libweave: Add removing of weave::Command::Observer Not a problem yet, but this is a cleaner approach. BUG=none TEST='FEATURES=test emerge-gizmo buffet' Change-Id: I1889ee293a3e1db34aad14a7166b7ebbe32a8c10 Reviewed-on: https://chromium-review.googlesource.com/290161 Reviewed-by: Vitaly Buka <vitalybuka@chromium.org> Commit-Queue: Vitaly Buka <vitalybuka@chromium.org> Trybot-Ready: Vitaly Buka <vitalybuka@chromium.org> Tested-by: Vitaly Buka <vitalybuka@chromium.org>
diff --git a/buffet/dbus_command_dispatcher.cc b/buffet/dbus_command_dispatcher.cc index 39e106b..9bbb55e 100644 --- a/buffet/dbus_command_dispatcher.cc +++ b/buffet/dbus_command_dispatcher.cc
@@ -30,7 +30,10 @@ object_manager_.get(), object_manager_->GetBus(), command, buffet::kCommandServicePathPrefix + std::to_string(++next_id_))}; proxy->RegisterAsync(AsyncEventSequencer::GetDefaultCompletionAction()); - command->AddObserver(proxy.release()); + // DBusCommandProxy::DBusCommandProxy() subscribe itself to weave::Command + // notifications. When weave::Command is being destroyed it sends + // ::OnCommandDestroyed() and DBusCommandProxy deletes itself. + proxy.release(); } } // namespace buffet
diff --git a/buffet/dbus_command_proxy.cc b/buffet/dbus_command_proxy.cc index d53355a..f89ec52 100644 --- a/buffet/dbus_command_proxy.cc +++ b/buffet/dbus_command_proxy.cc
@@ -20,7 +20,9 @@ weave::Command* command, std::string object_path) : command_{command}, - dbus_object_{object_manager, bus, dbus::ObjectPath{object_path}} {} + dbus_object_{object_manager, bus, dbus::ObjectPath{object_path}} { + observer_.Add(command); +} void DBusCommandProxy::RegisterAsync( const AsyncEventSequencer::CompletionAction& completion_callback) {
diff --git a/buffet/dbus_command_proxy.h b/buffet/dbus_command_proxy.h index 86d9c6e..7be3938 100644 --- a/buffet/dbus_command_proxy.h +++ b/buffet/dbus_command_proxy.h
@@ -8,6 +8,7 @@ #include <string> #include <base/macros.h> +#include <base/scoped_observer.h> #include <chromeos/dbus/data_serialization.h> #include <chromeos/dbus/dbus_object.h> @@ -59,6 +60,8 @@ org::chromium::Buffet::CommandAdaptor dbus_adaptor_{this}; chromeos::dbus_utils::DBusObject dbus_object_; + ScopedObserver<weave::Command, weave::Command::Observer> observer_{this}; + friend class DBusCommandProxyTest; friend class DBusCommandDispacherTest; DISALLOW_COPY_AND_ASSIGN(DBusCommandProxy);
diff --git a/libweave/include/weave/command.h b/libweave/include/weave/command.h index 58a63fe..0533afa 100644 --- a/libweave/include/weave/command.h +++ b/libweave/include/weave/command.h
@@ -30,18 +30,22 @@ // This interface lets the command to notify clients about changes. class Observer { public: - virtual ~Observer() = default; - virtual void OnResultsChanged() = 0; virtual void OnStatusChanged() = 0; virtual void OnProgressChanged() = 0; virtual void OnCommandDestroyed() = 0; + + protected: + virtual ~Observer() = default; }; // Adds an observer for this command. The observer object is not owned by this // class. virtual void AddObserver(Observer* observer) = 0; + // Removes an observer for this command. + virtual void RemoveObserver(Observer* observer) = 0; + // Returns the full command ID. virtual const std::string& GetID() const = 0;
diff --git a/libweave/include/weave/mock_command.h b/libweave/include/weave/mock_command.h index def903d..8addfea 100644 --- a/libweave/include/weave/mock_command.h +++ b/libweave/include/weave/mock_command.h
@@ -22,6 +22,7 @@ ~MockCommand() override = default; MOCK_METHOD1(AddObserver, void(Observer*)); + MOCK_METHOD1(RemoveObserver, void(Observer*)); MOCK_CONST_METHOD0(GetID, const std::string&()); MOCK_CONST_METHOD0(GetName, const std::string&()); MOCK_CONST_METHOD0(GetCategory, const std::string&());
diff --git a/libweave/src/commands/cloud_command_proxy.cc b/libweave/src/commands/cloud_command_proxy.cc index a6685fc..b277802 100644 --- a/libweave/src/commands/cloud_command_proxy.cc +++ b/libweave/src/commands/cloud_command_proxy.cc
@@ -28,6 +28,7 @@ callback_token_ = state_change_queue_->AddOnStateUpdatedCallback( base::Bind(&CloudCommandProxy::OnDeviceStateUpdated, weak_ptr_factory_.GetWeakPtr())); + observer_.Add(command_instance); } void CloudCommandProxy::OnResultsChanged() {
diff --git a/libweave/src/commands/cloud_command_proxy.h b/libweave/src/commands/cloud_command_proxy.h index db03885..5f7d21c 100644 --- a/libweave/src/commands/cloud_command_proxy.h +++ b/libweave/src/commands/cloud_command_proxy.h
@@ -13,6 +13,7 @@ #include <base/macros.h> #include <base/memory/ref_counted.h> #include <base/memory/weak_ptr.h> +#include <base/scoped_observer.h> #include <base/task_runner.h> #include <chromeos/backoff_entry.h> @@ -90,6 +91,8 @@ // successfully. UpdateID last_state_update_id_{0}; + ScopedObserver<Command, Command::Observer> observer_{this}; + base::WeakPtrFactory<CloudCommandProxy> backoff_weak_ptr_factory_{this}; base::WeakPtrFactory<CloudCommandProxy> weak_ptr_factory_{this}; DISALLOW_COPY_AND_ASSIGN(CloudCommandProxy);
diff --git a/libweave/src/commands/cloud_command_proxy_unittest.cc b/libweave/src/commands/cloud_command_proxy_unittest.cc index 7cc054e..f3e9b60 100644 --- a/libweave/src/commands/cloud_command_proxy_unittest.cc +++ b/libweave/src/commands/cloud_command_proxy_unittest.cc
@@ -157,7 +157,10 @@ &state_change_queue_, std::move(backoff), task_runner_}}; - command_instance_->AddObserver(proxy.release()); + // CloudCommandProxy::CloudCommandProxy() subscribe itself to weave::Command + // notifications. When weave::Command is being destroyed it sends + // ::OnCommandDestroyed() and CloudCommandProxy deletes itself. + proxy.release(); } StateChangeQueueInterface::UpdateID current_state_update_id_{0};
diff --git a/libweave/src/commands/command_instance.cc b/libweave/src/commands/command_instance.cc index 8c4b89a..4e89dca 100644 --- a/libweave/src/commands/command_instance.cc +++ b/libweave/src/commands/command_instance.cc
@@ -58,8 +58,7 @@ } CommandInstance::~CommandInstance() { - for (auto observer : observers_) - observer->OnCommandDestroyed(); + FOR_EACH_OBSERVER(Observer, observers_, OnCommandDestroyed()); } const std::string& CommandInstance::GetID() const { @@ -107,8 +106,7 @@ SetStatus(CommandStatus::kInProgress); if (obj != progress_) { progress_ = obj; - for (auto observer : observers_) - observer->OnProgressChanged(); + FOR_EACH_OBSERVER(Observer, observers_, OnProgressChanged()); } return true; } @@ -124,8 +122,7 @@ if (obj != results_) { results_ = obj; - for (auto observer : observers_) - observer->OnResultsChanged(); + FOR_EACH_OBSERVER(Observer, observers_, OnResultsChanged()); } return true; } @@ -251,7 +248,11 @@ } void CommandInstance::AddObserver(Observer* observer) { - observers_.push_back(observer); + observers_.AddObserver(observer); +} + +void CommandInstance::RemoveObserver(Observer* observer) { + observers_.RemoveObserver(observer); } void CommandInstance::Abort() { @@ -275,8 +276,7 @@ void CommandInstance::SetStatus(CommandStatus status) { if (status != status_) { status_ = status; - for (auto observer : observers_) - observer->OnStatusChanged(); + FOR_EACH_OBSERVER(Observer, observers_, OnStatusChanged()); } }
diff --git a/libweave/src/commands/command_instance.h b/libweave/src/commands/command_instance.h index 1b30d53..d709e2f 100644 --- a/libweave/src/commands/command_instance.h +++ b/libweave/src/commands/command_instance.h
@@ -11,6 +11,7 @@ #include <vector> #include <base/macros.h> +#include <base/observer_list.h> #include <chromeos/errors/error.h> #include "libweave/src/commands/prop_values.h" @@ -42,6 +43,7 @@ // Command overrides. std::unique_ptr<base::DictionaryValue> ToJson() const override; void AddObserver(Observer* observer) override; + void RemoveObserver(Observer* observer) override; const std::string& GetID() const override; const std::string& GetName() const override; const std::string& GetCategory() const override; @@ -109,8 +111,8 @@ ValueMap results_; // Current command status. CommandStatus status_ = CommandStatus::kQueued; - // Command observer for the command. - std::vector<Observer*> observers_; + // Command observers. + base::ObserverList<Observer> observers_; // Pointer to the command queue this command instance is added to. // The queue owns the command instance, so it outlives this object. CommandQueue* queue_ = nullptr;
diff --git a/libweave/src/device_registration_info.cc b/libweave/src/device_registration_info.cc index 1eddc21..ff35f5c 100644 --- a/libweave/src/device_registration_info.cc +++ b/libweave/src/device_registration_info.cc
@@ -1012,10 +1012,13 @@ << "' arrived, ID: " << command_instance->GetID(); std::unique_ptr<chromeos::BackoffEntry> backoff_entry{ new chromeos::BackoffEntry{cloud_backoff_policy_.get()}}; - std::unique_ptr<Command::Observer> cloud_proxy{new CloudCommandProxy{ + std::unique_ptr<CloudCommandProxy> cloud_proxy{new CloudCommandProxy{ command_instance.get(), this, state_manager_->GetStateChangeQueue(), std::move(backoff_entry), task_runner_}}; - command_instance->AddObserver(cloud_proxy.release()); + // CloudCommandProxy::CloudCommandProxy() subscribe itself to weave::Command + // notifications. When weave::Command is being destroyed it sends + // ::OnCommandDestroyed() and CloudCommandProxy deletes itself. + cloud_proxy.release(); command_manager_->AddCommand(std::move(command_instance)); } }