[Merge] lp:~mandel/location-service/espoo-delayed-provider into lp:location-service
Alfonso Sanchez-Beato
alfonso.sanchez-beato at canonical.com
Thu Jun 18 06:35:40 UTC 2015
Review: Needs Fixing
LGTM, just have some minor nits. See comments below.
Diff comments:
> === modified file 'include/location_service/com/ubuntu/location/provider.h'
> --- include/location_service/com/ubuntu/location/provider.h 2015-06-16 10:50:38 +0000
> +++ include/location_service/com/ubuntu/location/provider.h 2015-06-16 10:50:39 +0000
> @@ -241,7 +241,7 @@
> * @brief States the booting state of the provider.
> * @return The boot state of the provider.
> */
> - virtual BootState boot_state() const;
> + virtual BootState boot_state();
>
> /**
> * @brief Indicates to the provider that it should start the boot process in order to be able to
>
> === modified file 'src/location_service/com/ubuntu/location/provider.cpp'
> --- src/location_service/com/ubuntu/location/provider.cpp 2015-06-16 10:50:38 +0000
> +++ src/location_service/com/ubuntu/location/provider.cpp 2015-06-16 10:50:39 +0000
> @@ -142,6 +142,7 @@
>
> void cul::Provider::Controller::on_provider_booted(bool was_booted)
> {
> + LOG(INFO) << "Provider was booted " << was_booted;
> if (!was_booted)
> return;
>
> @@ -194,7 +195,7 @@
> return false;
> }
>
> -cul::Provider::BootState cul::Provider::boot_state() const
> +cul::Provider::BootState cul::Provider::boot_state()
Why did you remove the "const" modifier? The implementation has not changed.
> {
> return cul::Provider::BootState::BOOTED;
> }
>
> === modified file 'src/location_service/com/ubuntu/location/providers/remote/provider.cpp'
> --- src/location_service/com/ubuntu/location/providers/remote/provider.cpp 2015-04-14 18:54:00 +0000
> +++ src/location_service/com/ubuntu/location/providers/remote/provider.cpp 2015-06-16 10:50:39 +0000
> @@ -22,7 +22,9 @@
>
> #include <com/ubuntu/location/logging.h>
>
> +#include <core/dbus/dbus.h>
> #include <core/dbus/object.h>
> +#include <core/dbus/service_watcher.h>
> #include <core/dbus/signal.h>
> #include <core/dbus/asio/executor.h>
>
> @@ -30,6 +32,8 @@
>
> #include <boost/asio.hpp>
>
> +#include <functional>
> +#include <mutex>
> #include <thread>
>
> namespace cul = com::ubuntu::location;
> @@ -39,6 +43,123 @@
>
> namespace
> {
> +// NotYet is a provider decoration that waits for a remote service to come up, before instantiating
> +// the actual stub instance.
> +class NotYet : public cul::Provider
> +{
> +public:
> + // Creates a new instance, watching fo the given service_name on the given bus, invoking the
> + // when_ready functor for constructing the provider instance whenever the service becomes ready.
> + NotYet(const core::dbus::Bus::Ptr& bus, const std::string& service_name, const std::function<cul::Provider::Ptr()>& when_ready)
> + : dbus{bus}, service_watcher{dbus.make_service_watcher(service_name)},
> + service_registered
> + {
> + service_watcher->service_registered().connect(std::bind(&NotYet::on_service_registered, this, when_ready))
> + }
> + {
> + }
> +
> +
> + void on_service_registered(const std::function<cul::Provider::Ptr()>& when_ready)
> + {
> + LOG(INFO) << "Remove location service appeared.";
Should not be "Remote" here? Also, it would be nice to log some information so we know which concrete provider type has appeared.
> + impl = when_ready();
> + set_boot_state(cul::Provider::BootState::BOOTED);
> + // notify the system we are booted so that we get the current state (started etc..)
> + booted_signal()(true);
> + LOG(INFO) << "Emitting booted signal.";
> + }
> +
> + bool matches_criteria(const cul::Criteria& criteria) override
> + {
> + if (impl) return impl->matches_criteria(criteria);
> +
> + return false;
> + }
> +
> + bool supports(const cul::Provider::Features& f) const override
> + {
> + if (impl) return impl->supports(f);
> + return false;
> + }
> +
> + bool requires(const cul::Provider::Requirements& r) const override
> + {
> + if (impl) return impl->requires(r);
> + return true;
> + }
> +
> + void on_wifi_and_cell_reporting_state_changed(cul::WifiAndCellIdReportingState state) override
> + {
> + if (impl) impl->on_wifi_and_cell_reporting_state_changed(state);
> + }
> +
> + void on_reference_location_updated(const cul::Update<cul::Position>& position) override
> + {
> + if (impl) impl->on_reference_location_updated(position);
> + }
> +
> + void on_reference_velocity_updated(const cul::Update<cul::Velocity>& velocity) override
> + {
> + if (impl) impl->on_reference_velocity_updated(velocity);
> + }
> +
> + void on_reference_heading_updated(const cul::Update<cul::Heading>& heading) override
> + {
> + if (impl) impl->on_reference_heading_updated(heading);
> + }
> +
> + void start_position_updates() override
> + {
> + if (impl) impl->state_controller()->start_position_updates();
> + }
> +
> + void stop_position_updates() override
> + {
> + if (impl) impl->state_controller()->stop_position_updates();
> + }
> +
> + void start_heading_updates() override
> + {
> + if (impl) impl->state_controller()->start_heading_updates();
> + }
> +
> + void stop_heading_updates() override
> + {
> + if (impl) impl->state_controller()->stop_heading_updates();
> + }
> +
> + void start_velocity_updates() override
> + {
> + if (impl) impl->state_controller()->start_velocity_updates();
> + }
> +
> + void stop_velocity_updates() override
> + {
> + if (impl) impl->state_controller()->stop_velocity_updates();
> + }
> +
> + BootState boot_state()
> + {
> + std::lock_guard<std::mutex> guard(state_mutex);
> + return state;
> + }
> +
> + void set_boot_state(BootState new_state)
> + {
> + std::lock_guard<std::mutex> guard(state_mutex);
> + state = new_state;
> + }
> +
> +private:
> + core::dbus::DBus dbus;
> + std::unique_ptr<core::dbus::ServiceWatcher> service_watcher;
> + core::ScopedConnection service_registered;
> + cul::Provider::Ptr impl;
> + cul::Provider::BootState state = cul::Provider::BootState::BOOTING;
> + std::mutex state_mutex;
> +};
> +
> struct Runtime
> {
> static Runtime& instance()
> @@ -191,7 +312,19 @@
> auto service = dbus::Service::use_service(bus, name);
> auto object = service->object_for_path(path);
>
> - return create_instance_with_config(remote::stub::Configuration{object});
> + try
> + {
> + auto result = create_instance_with_config(remote::stub::Configuration{object});
> + } catch (const std::runtime_error& e)
> + {
> + std::cerr << "Error creating remote provider, creating facade for watching the service and delayed initialization" << std::endl;
Again, it would be nice to log some information so we know the provider type that has not appeared yet. Besides, the line is too long. I know it is better not to split strings, but you could have wrapped std::endl.
> + }
> +
> + return std::make_shared<NotYet>(bus, name, [bus, name, path]()
> + {
> + auto object = dbus::Service::use_service(bus, name)->object_for_path(path);
> + return create_instance_with_config(remote::stub::Configuration{object});
> + });
> }
>
> cul::Provider::Ptr remote::Provider::Stub::create_instance_with_config(const remote::stub::Configuration& config)
>
> === modified file 'src/location_service/com/ubuntu/location/providers/remote/provider.h'
> --- src/location_service/com/ubuntu/location/providers/remote/provider.h 2015-01-26 19:11:46 +0000
> +++ src/location_service/com/ubuntu/location/providers/remote/provider.h 2015-06-16 10:50:39 +0000
> @@ -40,7 +40,8 @@
> {
> class Stub : public com::ubuntu::location::Provider, public std::enable_shared_from_this<Stub>
> {
> - public:
> + public:
There is a whitespace to the right of "public:"
> +
> // For integration with the Provider factory.
> static std::string class_name();
>
>
> === modified file 'tests/mock_delayed_provider.h'
> --- tests/mock_delayed_provider.h 2015-06-16 10:50:38 +0000
> +++ tests/mock_delayed_provider.h 2015-06-16 10:50:39 +0000
> @@ -63,7 +63,7 @@
> MOCK_METHOD0(stop_velocity_updates, void());
>
> MOCK_CONST_METHOD0(has_delayed_boot, bool());
> - MOCK_CONST_METHOD0(boot_state, BootState());
> + MOCK_METHOD0(boot_state, BootState());
> MOCK_CONST_METHOD0(boot, void());
>
>
>
--
https://code.launchpad.net/~mandel/location-service/espoo-delayed-provider/+merge/262056
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service.
More information about the Ubuntu-reviews
mailing list