buffet: Multiple listeners of add/remove new command.

Replace CommandDispatchInterface with callbacks.
Lists of callbacks in CommandQueue.

BUG=brillo:697
TEST=FEATURE=test emerge-gizmo buffet

Change-Id: I3c164c8c7c2cb098b896aa8d5c9a99d856b05172
Reviewed-on: https://chromium-review.googlesource.com/270350
Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>
Tested-by: Vitaly Buka <vitalybuka@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/commands/command_dispatch_interface.h b/buffet/commands/command_dispatch_interface.h
deleted file mode 100644
index d1a727d..0000000
--- a/buffet/commands/command_dispatch_interface.h
+++ /dev/null
@@ -1,29 +0,0 @@
-// Copyright 2014 The Chromium OS Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef BUFFET_COMMANDS_COMMAND_DISPATCH_INTERFACE_H_
-#define BUFFET_COMMANDS_COMMAND_DISPATCH_INTERFACE_H_
-
-#include <string>
-
-namespace buffet {
-
-class CommandInstance;
-
-// This is an abstract base interface that a command dispatcher will implement.
-// It allows to abstract out the actual transport layer, such as D-Bus, from
-// the rest of command registration and delivery subsystems.
-class CommandDispachInterface {
- public:
-  virtual ~CommandDispachInterface() = default;
-  // Callback invoked by CommandQueue when a new command is added to the queue.
-  virtual void OnCommandAdded(CommandInstance* command_instance) = 0;
-  // Callback invoked by CommandQueue when a new command is removed from
-  // the queue.
-  virtual void OnCommandRemoved(CommandInstance* command_instance) = 0;
-};
-
-}  // namespace buffet
-
-#endif  // BUFFET_COMMANDS_COMMAND_DISPATCH_INTERFACE_H_
diff --git a/buffet/commands/command_manager.cc b/buffet/commands/command_manager.cc
index de2125b..cbaaeca 100644
--- a/buffet/commands/command_manager.cc
+++ b/buffet/commands/command_manager.cc
@@ -16,18 +16,16 @@
 
 namespace buffet {
 
-CommandManager::CommandManager() {
-  command_queue_.SetCommandDispachInterface(&command_dispatcher_);
+CommandManager::CommandManager()
+    : CommandManager(base::WeakPtr<ExportedObjectManager>{}) {
 }
 
 CommandManager::CommandManager(
     const base::WeakPtr<ExportedObjectManager>& object_manager)
-    : command_dispatcher_(object_manager->GetBus(), object_manager.get()) {
-  command_queue_.SetCommandDispachInterface(&command_dispatcher_);
-}
-
-CommandManager::CommandManager(CommandDispachInterface* dispatch_interface) {
-  command_queue_.SetCommandDispachInterface(dispatch_interface);
+    : command_dispatcher_(object_manager) {
+  command_queue_.AddOnCommandAddedCallback(
+      base::Bind(&DBusCommandDispacher::OnCommandAdded,
+                 base::Unretained(&command_dispatcher_)));
 }
 
 const CommandDictionary& CommandManager::GetCommandDictionary() const {
diff --git a/buffet/commands/command_manager.h b/buffet/commands/command_manager.h
index b2b9eba..88ebca7 100644
--- a/buffet/commands/command_manager.h
+++ b/buffet/commands/command_manager.h
@@ -38,8 +38,6 @@
   explicit CommandManager(
       const base::WeakPtr<chromeos::dbus_utils::ExportedObjectManager>&
           object_manager);
-  // Special constructor to help mock out command dispatcher for testing.
-  explicit CommandManager(CommandDispachInterface* dispatch_interface);
 
   // Sets callback which is called when command definitions is changed.
   void AddOnCommandDefChanged(const base::Closure& callback) {
@@ -103,8 +101,8 @@
  private:
   CommandDictionary base_dictionary_;  // Base/std command definitions/schemas.
   CommandDictionary dictionary_;  // Command definitions/schemas.
-  CommandQueue command_queue_;
   DBusCommandDispacher command_dispatcher_;
+  CommandQueue command_queue_;
   std::vector<base::Callback<void()>> on_command_changed_;
 
   DISALLOW_COPY_AND_ASSIGN(CommandManager);
diff --git a/buffet/commands/command_queue.cc b/buffet/commands/command_queue.cc
index 62d8c20..614261e 100644
--- a/buffet/commands/command_queue.cc
+++ b/buffet/commands/command_queue.cc
@@ -4,16 +4,23 @@
 
 #include "buffet/commands/command_queue.h"
 
+#include <base/bind.h>
 #include <base/time/time.h>
 
-#include "buffet/commands/command_dispatch_interface.h"
-
 namespace buffet {
 
 namespace {
 const int kRemoveCommandDelayMin = 5;
 }
 
+void CommandQueue::AddOnCommandAddedCallback(const Callback& callback) {
+  on_command_added_.push_back(callback);
+}
+
+void CommandQueue::AddOnCommandRemovedCallback(const Callback& callback) {
+  on_command_removed_.push_back(callback);
+}
+
 void CommandQueue::Add(std::unique_ptr<CommandInstance> instance) {
   std::string id = instance->GetID();
   LOG_IF(FATAL, id.empty()) << "Command has no ID";
@@ -21,8 +28,8 @@
   auto pair = map_.insert(std::make_pair(id, std::move(instance)));
   LOG_IF(FATAL, !pair.second) << "Command with ID '" << id
                               << "' is already in the queue";
-  if (dispatch_interface_)
-    dispatch_interface_->OnCommandAdded(pair.first->second.get());
+  for (const auto& cb : on_command_added_)
+    cb.Run(pair.first->second.get());
   Cleanup();
 }
 
@@ -43,8 +50,8 @@
   std::unique_ptr<CommandInstance> instance{std::move(p->second)};
   instance->SetCommandQueue(nullptr);
   map_.erase(p);
-  if (dispatch_interface_)
-    dispatch_interface_->OnCommandRemoved(instance.get());
+  for (const auto& cb : on_command_removed_)
+    cb.Run(instance.get());
   return true;
 }
 
diff --git a/buffet/commands/command_queue.h b/buffet/commands/command_queue.h
index 09339d9..9178c8a 100644
--- a/buffet/commands/command_queue.h
+++ b/buffet/commands/command_queue.h
@@ -10,25 +10,25 @@
 #include <queue>
 #include <string>
 #include <utility>
+#include <vector>
 
+#include <base/callback.h>
 #include <base/macros.h>
 
 #include "buffet/commands/command_instance.h"
 
 namespace buffet {
 
-class CommandDispachInterface;
-
 class CommandQueue final {
  public:
+  using Callback = base::Callback<void(CommandInstance*)>;
   CommandQueue() = default;
 
-  // Sets a command dispatch notifications for changes in command queue.
-  // |dispatch_interface| must outlive the CommandQueue object instance
-  // or be nullptr.
-  void SetCommandDispachInterface(CommandDispachInterface* dispatch_interface) {
-    dispatch_interface_ = dispatch_interface;
-  }
+  // Adds notifications callback for a new command is added to the queue.
+  void AddOnCommandAddedCallback(const Callback& callback);
+
+  // Adds notifications callback for a command is removed from the queue.
+  void AddOnCommandRemovedCallback(const Callback& callback);
 
   // Checks if the command queue is empty.
   bool IsEmpty() const { return map_.empty(); }
@@ -75,8 +75,9 @@
   // Queue of commands to be removed.
   std::queue<std::pair<base::Time, std::string>> remove_queue_;
 
-  // Callback interface for command dispatch, if provided.
-  CommandDispachInterface* dispatch_interface_ = nullptr;
+  using CallbackList = std::vector<Callback>;
+  CallbackList on_command_added_;
+  CallbackList on_command_removed_;
 
   DISALLOW_COPY_AND_ASSIGN(CommandQueue);
 };
diff --git a/buffet/commands/command_queue_unittest.cc b/buffet/commands/command_queue_unittest.cc
index 8829404..f29b03e 100644
--- a/buffet/commands/command_queue_unittest.cc
+++ b/buffet/commands/command_queue_unittest.cc
@@ -8,11 +8,12 @@
 #include <string>
 #include <vector>
 
+#include <base/bind.h>
+#include <base/memory/weak_ptr.h>
 #include <chromeos/strings/string_utils.h>
 #include <gtest/gtest.h>
 
 #include "buffet/commands/command_definition.h"
-#include "buffet/commands/command_dispatch_interface.h"
 #include "buffet/commands/object_schema.h"
 
 namespace buffet {
@@ -44,22 +45,30 @@
                                         ObjectSchema::Create()};
 };
 
-// Fake implementation of CommandDispachInterface.
-// Just keeps track of commands being added to and removed from the queue_.
+// Keeps track of commands being added to and removed from the queue_.
 // Aborts if duplicate commands are added or non-existent commands are removed.
-class FakeDispatchInterface : public CommandDispachInterface {
+class FakeDispatcher {
  public:
-  void OnCommandAdded(CommandInstance* command_instance) override {
+  explicit FakeDispatcher(CommandQueue* queue) {
+    queue->AddOnCommandAddedCallback(
+        base::Bind(&FakeDispatcher::OnCommandAdded,
+                   weak_ptr_factory_.GetWeakPtr()));
+    queue->AddOnCommandRemovedCallback(
+        base::Bind(&FakeDispatcher::OnCommandRemoved,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void OnCommandAdded(CommandInstance* command_instance) {
     CHECK(ids_.insert(command_instance->GetID()).second)
         << "Command ID already exists: " << command_instance->GetID();
     CHECK(commands_.insert(command_instance).second)
         << "Command instance already exists";
   }
 
-  void OnCommandRemoved(CommandInstance* command_instance) override {
-    CHECK_EQ(1, ids_.erase(command_instance->GetID()))
+  void OnCommandRemoved(CommandInstance* command_instance) {
+    CHECK_EQ(1u, ids_.erase(command_instance->GetID()))
         << "Command ID not found: " << command_instance->GetID();
-    CHECK_EQ(1, commands_.erase(command_instance))
+    CHECK_EQ(1u, commands_.erase(command_instance))
         << "Command instance not found";
   }
 
@@ -73,6 +82,7 @@
  private:
   std::set<std::string> ids_;
   std::set<CommandInstance*> commands_;
+  base::WeakPtrFactory<FakeDispatcher> weak_ptr_factory_{this};
 };
 
 TEST_F(CommandQueueTest, Empty) {
@@ -123,8 +133,7 @@
 }
 
 TEST_F(CommandQueueTest, Dispatch) {
-  FakeDispatchInterface dispatch;
-  queue_.SetCommandDispachInterface(&dispatch);
+  FakeDispatcher dispatch(&queue_);
   const std::string id1 = "id1";
   const std::string id2 = "id2";
   queue_.Add(CreateDummyCommandInstance("base.reboot", id1));
@@ -137,7 +146,6 @@
   EXPECT_EQ(id2, dispatch.GetIDs());
   Remove(id2);
   EXPECT_EQ("", dispatch.GetIDs());
-  queue_.SetCommandDispachInterface(nullptr);
 }
 
 TEST_F(CommandQueueTest, Find) {
diff --git a/buffet/commands/dbus_command_dispatcher.cc b/buffet/commands/dbus_command_dispatcher.cc
index 457ea88..2f5333c 100644
--- a/buffet/commands/dbus_command_dispatcher.cc
+++ b/buffet/commands/dbus_command_dispatcher.cc
@@ -7,6 +7,7 @@
 #include <chromeos/dbus/exported_object_manager.h>
 
 #include "buffet/commands/command_instance.h"
+#include "buffet/commands/dbus_command_proxy.h"
 #include "buffet/dbus_constants.h"
 
 using chromeos::dbus_utils::AsyncEventSequencer;
@@ -15,30 +16,18 @@
 namespace buffet {
 
 DBusCommandDispacher::DBusCommandDispacher(
-    const scoped_refptr<dbus::Bus>& bus,
-    ExportedObjectManager* object_manager)
-    : bus_(bus), next_id_(0) {
-  if (object_manager)
-    object_manager_ = object_manager->AsWeakPtr();
+    const base::WeakPtr<ExportedObjectManager>& object_manager)
+    : object_manager_{object_manager} {
 }
 
 void DBusCommandDispacher::OnCommandAdded(CommandInstance* command_instance) {
-  auto proxy = CreateDBusCommandProxy(command_instance);
+  if (!object_manager_)
+    return;
+  std::unique_ptr<DBusCommandProxy> proxy{new DBusCommandProxy(
+      object_manager_.get(), object_manager_->GetBus(), command_instance,
+      dbus_constants::kCommandServicePathPrefix + std::to_string(++next_id_))};
   proxy->RegisterAsync(AsyncEventSequencer::GetDefaultCompletionAction());
   command_instance->AddProxy(std::move(proxy));
 }
 
-void DBusCommandDispacher::OnCommandRemoved(CommandInstance* command_instance) {
-}
-
-std::unique_ptr<DBusCommandProxy> DBusCommandDispacher::CreateDBusCommandProxy(
-      CommandInstance* command_instance) {
-  return std::unique_ptr<DBusCommandProxy>(
-      new DBusCommandProxy(object_manager_.get(),
-                           bus_,
-                           command_instance,
-                           dbus_constants::kCommandServicePathPrefix +
-                           std::to_string(++next_id_)));
-}
-
 }  // namespace buffet
diff --git a/buffet/commands/dbus_command_dispatcher.h b/buffet/commands/dbus_command_dispatcher.h
index 35d258b..ab63a6c 100644
--- a/buffet/commands/dbus_command_dispatcher.h
+++ b/buffet/commands/dbus_command_dispatcher.h
@@ -10,10 +10,6 @@
 
 #include <base/macros.h>
 #include <base/memory/weak_ptr.h>
-#include <dbus/bus.h>
-
-#include "buffet/commands/command_dispatch_interface.h"
-#include "buffet/commands/dbus_command_proxy.h"
 
 namespace chromeos {
 namespace dbus_utils {
@@ -23,37 +19,29 @@
 
 namespace buffet {
 
-// Implements D-Bus dispatch of commands. When OnCommandAdded is called over
-// CommandDispachInterface, DBusCommandDispacher creates an instance of
-// DBusCommandProxy object and advertises it through ExportedObjectManager on
-// D-Bus. Command handling processes can watch the new D-Bus object appear
-// and communicate with it to update the command handling progress.
-// Once command is handled, DBusCommandProxy::Done() is called and the command
-// is removed from the command queue and D-Bus ExportedObjectManager.
-class DBusCommandDispacher : public CommandDispachInterface {
+class CommandInstance;
+
+// Implements D-Bus dispatch of commands. When OnCommandAdded is called,
+// DBusCommandDispacher creates an instance of DBusCommandProxy object and
+// advertises it through ExportedObjectManager on D-Bus. Command handling
+// processes can watch the new D-Bus object appear and communicate with it to
+// update the command handling progress. Once command is handled,
+// DBusCommandProxy::Done() is called and the command is removed from the
+// command queue and D-Bus ExportedObjectManager.
+class DBusCommandDispacher {
  public:
-  DBusCommandDispacher(
-      const scoped_refptr<dbus::Bus>& bus,
-      chromeos::dbus_utils::ExportedObjectManager* object_manager = nullptr);
-  virtual ~DBusCommandDispacher() = default;
+  explicit DBusCommandDispacher(const base::WeakPtr<
+      chromeos::dbus_utils::ExportedObjectManager>& object_manager);
 
-  // CommandDispachInterface overrides. Called by CommandQueue.
-  void OnCommandAdded(CommandInstance* command_instance) override;
-  void OnCommandRemoved(CommandInstance* command_instance) override;
-
- protected:
-  virtual std::unique_ptr<DBusCommandProxy> CreateDBusCommandProxy(
-      CommandInstance* command_instance);
+  void OnCommandAdded(CommandInstance* command_instance);
 
  private:
-  scoped_refptr<dbus::Bus> bus_;
   base::WeakPtr<chromeos::dbus_utils::ExportedObjectManager> object_manager_;
-  int next_id_;
+  int next_id_{0};
 
   // Default constructor is used in special circumstances such as for testing.
   DBusCommandDispacher() = default;
 
-  friend class DBusCommandDispacherTest;
   friend class CommandManager;
   DISALLOW_COPY_AND_ASSIGN(DBusCommandDispacher);
 };
diff --git a/buffet/commands/dbus_command_dispatcher_unittest.cc b/buffet/commands/dbus_command_dispatcher_unittest.cc
index 2391bb1..d9d91bc 100644
--- a/buffet/commands/dbus_command_dispatcher_unittest.cc
+++ b/buffet/commands/dbus_command_dispatcher_unittest.cc
@@ -16,6 +16,7 @@
 
 #include "buffet/commands/command_dictionary.h"
 #include "buffet/commands/command_queue.h"
+#include "buffet/commands/dbus_command_proxy.h"
 #include "buffet/commands/unittest_utils.h"
 #include "buffet/dbus_constants.h"
 
@@ -60,9 +61,10 @@
     om_.reset(new chromeos::dbus_utils::ExportedObjectManager(
         bus_.get(), kExportedObjectManagerPath));
     om_->RegisterAsync(AsyncEventSequencer::GetDefaultCompletionAction());
-    command_dispatcher_.reset(
-        new DBusCommandDispacher(om_->GetBus(), om_.get()));
-    command_queue_.SetCommandDispachInterface(command_dispatcher_.get());
+    command_dispatcher_.reset(new DBusCommandDispacher(om_->AsWeakPtr()));
+    command_queue_.AddOnCommandAddedCallback(
+        base::Bind(&DBusCommandDispacher::OnCommandAdded,
+                   base::Unretained(command_dispatcher_.get())));
     // Use a mock exported object for command proxy.
     mock_exported_command_proxy_ = new dbus::MockExportedObject(
         bus_.get(), kCmdObjPath);
@@ -133,8 +135,8 @@
   scoped_refptr<dbus::MockExportedObject> mock_exported_command_proxy_;
   std::unique_ptr<chromeos::dbus_utils::ExportedObjectManager> om_;
   CommandDictionary dictionary_;
-  CommandQueue command_queue_;
   std::unique_ptr<DBusCommandDispacher> command_dispatcher_;
+  CommandQueue command_queue_;
 };
 
 TEST_F(DBusCommandDispacherTest, Test_Command_Base_Shutdown) {