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));
}
}
