[Merge] lp:~thomas-voss/location-service/enable-report-to-free-databases into lp:location-service
Manuel de la Peña
manuel.delapena at canonical.com
Thu Aug 14 21:08:31 UTC 2014
Review: Approve
I'm approving because the comments are not show stoppers and are really minor.
Diff comments:
> === modified file 'src/location_service/com/ubuntu/location/CMakeLists.txt'
> --- src/location_service/com/ubuntu/location/CMakeLists.txt 2014-08-08 07:33:49 +0000
> +++ src/location_service/com/ubuntu/location/CMakeLists.txt 2014-08-13 13:16:20 +0000
> @@ -29,6 +29,8 @@
> service/default_configuration.cpp
> service/default_permission_manager.cpp
> service/harvester.cpp
> + service/demultiplexing_reporter.h
> + service/demultiplexing_reporter.cpp
> service/trust_store_permission_manager.cpp
>
> service/implementation.cpp
>
> === modified file 'src/location_service/com/ubuntu/location/service/daemon.cpp'
> --- src/location_service/com/ubuntu/location/service/daemon.cpp 2014-08-01 12:51:25 +0000
> +++ src/location_service/com/ubuntu/location/service/daemon.cpp 2014-08-13 13:16:20 +0000
> @@ -18,6 +18,8 @@
> #include <com/ubuntu/location/provider_factory.h>
>
> #include <com/ubuntu/location/service/default_configuration.h>
> +#include <com/ubuntu/location/service/demultiplexing_reporter.h>
> +#include <com/ubuntu/location/service/ichnaea_reporter.h>
> #include <com/ubuntu/location/service/implementation.h>
> #include <com/ubuntu/location/service/stub.h>
>
> @@ -202,7 +204,28 @@
> location::service::Harvester::Configuration
> {
> location::connectivity::platform_default_manager(),
> - std::make_shared<NullReporter>()
> + // We submit location/wifi/cell measurements to both
> + // Mozilla's location service and to Ubuntu's own instance.
> + std::make_shared<service::DemultiplexingReporter>(
> + std::initializer_list<service::Harvester::Reporter::Ptr>
> + {
> + std::make_shared<service::ichnaea::Reporter>(
> + service::ichnaea::Reporter::Configuration
> + {
> + "https://location.services.mozilla.com",
What about moving this url to a const? Just so that is easier to find and modify.
> + "UbuntuLocationService",
> + "UbuntuLocationService"
> + }
> + ),
> + std::make_shared<service::ichnaea::Reporter>(
> + service::ichnaea::Reporter::Configuration
> + {
> + "https://162.213.35.107",
Same as the above, should we just move it to ubuntus_location_service_url or something of the kind.
> + "UbuntuLocationService",
> + "UbuntuLocationService"
> + }
> + )
> + })
> }
> };
>
>
> === added file 'src/location_service/com/ubuntu/location/service/demultiplexing_reporter.cpp'
> --- src/location_service/com/ubuntu/location/service/demultiplexing_reporter.cpp 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/service/demultiplexing_reporter.cpp 2014-08-13 13:16:20 +0000
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright © 2012-2013 Canonical Ltd.
> + *
Stupid error, but we need to use the correct dates here.
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +
> +#include <com/ubuntu/location/service/demultiplexing_reporter.h>
> +
> +namespace location = com::ubuntu::location;
> +namespace service = com::ubuntu::location::service;
> +
> +service::DemultiplexingReporter::DemultiplexingReporter(const std::set<service::Harvester::Reporter::Ptr>& reporters)
> + : reporters{reporters}
> +{
> +
> +}
> +
> +// Tell the reporters that it should start operating.
> +void service::DemultiplexingReporter::start()
> +{
> + std::lock_guard<std::mutex> lg{reporters_guard};
> + for(auto reporter : reporters)
> + reporter->start();
> +}
> +
> +// Tell the reporters to shut down its operation.
> +void service::DemultiplexingReporter::stop()
> +{
> + std::lock_guard<std::mutex> lg{reporters_guard};
> + for(auto reporter : reporters)
> + reporter->stop();
> +}
> +
> +// Triggers the reporters to send off the information.
> +void service::DemultiplexingReporter::report(
> + const location::Update<location::Position>& update,
> + const std::vector<location::connectivity::WirelessNetwork::Ptr>& wifis,
> + const std::vector<location::connectivity::RadioCell::Ptr>& cells)
> +{
> + std::lock_guard<std::mutex> lg{reporters_guard};
> + for(auto reporter : reporters)
> + reporter->report(update, wifis, cells);
> +}
>
> === added file 'src/location_service/com/ubuntu/location/service/demultiplexing_reporter.h'
> --- src/location_service/com/ubuntu/location/service/demultiplexing_reporter.h 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/service/demultiplexing_reporter.h 2014-08-13 13:16:20 +0000
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright © 2012-2013 Canonical Ltd.
Stupid error, but we need to use the correct dates here.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_DEMULTIPLEXING_REPORTER_H_
> +#define LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_DEMULTIPLEXING_REPORTER_H_
> +
> +#include <com/ubuntu/location/service/harvester.h>
> +
> +namespace com{namespace ubuntu{namespace location{namespace service
> +{
> +
> +// A simple demultiplexer, distributing updates to a set of
> +// actual reporter implementations.
> +class DemultiplexingReporter : public Harvester::Reporter
> +{
> +public:
> + DemultiplexingReporter(const std::set<Harvester::Reporter::Ptr>& reporters);
> +
> + // Tell the reporters that it should start operating.
> + void start() override;
> +
> + // Tell the reporters to shut down its operation.
> + void stop() override;
> +
> + // Triggers the reporters to send off the information.
> + void report(const Update<Position>& update,
> + const std::vector<connectivity::WirelessNetwork::Ptr>& wifis,
> + const std::vector<connectivity::RadioCell::Ptr>& cells) override;
> +private:
> + std::mutex reporters_guard;
> + std::set<Harvester::Reporter::Ptr> reporters;
> +};
> +}}}}
> +
> +#endif // LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_DEMULTIPLEXING_REPORTER_H_
>
> === modified file 'src/location_service/com/ubuntu/location/service/implementation.cpp'
> --- src/location_service/com/ubuntu/location/service/implementation.cpp 2014-08-01 12:51:25 +0000
> +++ src/location_service/com/ubuntu/location/service/implementation.cpp 2014-08-13 13:16:20 +0000
> @@ -81,6 +81,11 @@
> = value ?
> cul::WifiAndCellIdReportingState::on :
> cul::WifiAndCellIdReportingState::off;
> +
> + if (value)
> + harvester.start();
> + else
> + harvester.stop();
> }),
> does_satellite_based_positioning().changed().connect(
> [this](bool value)
>
> === modified file 'tests/CMakeLists.txt'
> --- tests/CMakeLists.txt 2014-07-31 09:39:03 +0000
> +++ tests/CMakeLists.txt 2014-08-13 13:16:20 +0000
> @@ -77,6 +77,7 @@
> LOCATION_SERVICE_ADD_TEST(default_permission_manager_test default_permission_manager_test.cpp)
> LOCATION_SERVICE_ADD_TEST(engine_test engine_test.cpp)
> LOCATION_SERVICE_ADD_TEST(harvester_test harvester_test.cpp)
> +LOCATION_SERVICE_ADD_TEST(demultiplexing_reporter_test demultiplexing_reporter_test.cpp)
>
> if (NET_CPP_FOUND)
> LOCATION_SERVICE_ADD_TEST(ichnaea_reporter_test ichnaea_reporter_test.cpp)
>
> === added file 'tests/demultiplexing_reporter_test.cpp'
> --- tests/demultiplexing_reporter_test.cpp 1970-01-01 00:00:00 +0000
> +++ tests/demultiplexing_reporter_test.cpp 2014-08-13 13:16:20 +0000
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright © 2012-2013 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +
> +#include <com/ubuntu/location/service/demultiplexing_reporter.h>
> +
> +#include <gtest/gtest.h>
> +
> +#include "mock_reporter.h"
> +
> +namespace location = com::ubuntu::location;
> +namespace service = com::ubuntu::location::service;
> +
> +namespace
> +{
> +location::Update<location::Position> reference_position_update
> +{
> + {
> + location::wgs84::Latitude{9. * location::units::Degrees},
> + location::wgs84::Longitude{53. * location::units::Degrees},
> + location::wgs84::Altitude{-2. * location::units::Meters}
> + },
> + location::Clock::now()
> +};
> +}
> +TEST(DemultiplexingReporter, dispatches_calls_to_all_reporters)
> +{
> + using namespace ::testing;
> +
> + std::set<service::Harvester::Reporter::Ptr> reporters;
> +
> + for (unsigned int i = 0; i < 5; i++)
> + {
> + auto reporter = std::make_shared<MockReporter>();
> +
> + EXPECT_CALL(*reporter, start()).Times(1);
> + EXPECT_CALL(*reporter, stop()).Times(1);
> + EXPECT_CALL(*reporter, report(_, _, _)).Times(1);
A matcher can be used to check the expected values here. But is not a land stoper.
> +
> + reporters.insert(reporter);
> + }
> +
> + service::DemultiplexingReporter reporter{reporters};
> +
> + reporter.start();
> + reporter.report(reference_position_update,{}, {});
> + reporter.stop();
> +}
>
> === modified file 'tests/harvester_test.cpp'
> --- tests/harvester_test.cpp 2014-06-12 15:08:23 +0000
> +++ tests/harvester_test.cpp 2014-08-13 13:16:20 +0000
> @@ -19,6 +19,7 @@
> #include <com/ubuntu/location/service/harvester.h>
>
> #include "mock_connectivity_manager.h"
> +#include "mock_reporter.h"
> #include "null_provider_selection_policy.h"
>
> #include <gmock/gmock.h>
> @@ -28,26 +29,6 @@
>
> namespace
> {
> -struct MockReporter : public location::service::Harvester::Reporter
> -{
> - MockReporter() = default;
> -
> - /** @brief Tell the reporter that it should start operating. */
> - MOCK_METHOD0(start, void());
> -
> - /** @brief Tell the reporter to shut down its operation. */
> - MOCK_METHOD0(stop, void());
> -
> - /**
> - * @brief Triggers the reporter to send off the information.
> - */
> - MOCK_METHOD3(report,
> - void(
> - const location::Update<location::Position>&,
> - const std::vector<location::connectivity::WirelessNetwork::Ptr>&,
> - const std::vector<location::connectivity::RadioCell::Ptr>&));
> -};
> -
> location::Update<location::Position> reference_position_update
> {
> {
>
> === added file 'tests/mock_reporter.h'
> --- tests/mock_reporter.h 1970-01-01 00:00:00 +0000
> +++ tests/mock_reporter.h 2014-08-13 13:16:20 +0000
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright © 2012-2013 Canonical Ltd.
Stupid error, but we need to use the correct dates here.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +#ifndef MOCK_REPORTER_H_
> +#define MOCK_REPORTER_H_
> +
> +#include <com/ubuntu/location/service/harvester.h>
> +
> +#include <gmock/gmock.h>
> +
> +struct MockReporter : public com::ubuntu::location::service::Harvester::Reporter
> +{
> + MockReporter() = default;
> +
> + /** @brief Tell the reporter that it should start operating. */
Do we need to document the mock?
> + MOCK_METHOD0(start, void());
> +
> + /** @brief Tell the reporter to shut down its operation. */
> + MOCK_METHOD0(stop, void());
> +
> + /**
> + * @brief Triggers the reporter to send off the information.
> + */
> + MOCK_METHOD3(report,
> + void(
> + const com::ubuntu::location::Update<com::ubuntu::location::Position>&,
> + const std::vector<com::ubuntu::location::connectivity::WirelessNetwork::Ptr>&,
> + const std::vector<com::ubuntu::location::connectivity::RadioCell::Ptr>&));
> +};
> +
> +#endif // MOCK_REPORTER_H_
>
--
https://code.launchpad.net/~thomas-voss/location-service/enable-report-to-free-databases/+merge/230618
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service.
More information about the Ubuntu-reviews
mailing list