Get rid of Callback from CrosDisksClient part 1.

This CL removes Callback usage for D-Bus signals in
CrosDisksClient.
For the interface, Observer is used following the guideline.
For the implementation, OnceCallback and RepeatingCallback are
used.

BUG=739622
TEST=Ran on bots.

Change-Id: I453dd2f4f6148adad074e915500cc651090cefd8
Reviewed-on: https://chromium-review.googlesource.com/817714
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522840}

chromeos/dbus/cros_disks_client.cc

@@ -15,6 +15,7 @@
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/sys_info.h"
@@ -94,7 +95,17 @@ bool ReadMountEntryFromDbus(dbus::MessageReader* reader, MountEntry* entry) {
 // The CrosDisksClient implementation.
 class CrosDisksClientImpl : public CrosDisksClient {
  public:
-  CrosDisksClientImpl() : proxy_(NULL), weak_ptr_factory_(this) {}
+  CrosDisksClientImpl() : proxy_(nullptr), weak_ptr_factory_(this) {}
+
+  // CrosDisksClient override.
+  void AddObserver(Observer* observer) override {
+    observer_list_.AddObserver(observer);
+  }
+
+  // CrosDisksClient override.
+  void RemoveObserver(Observer* observer) override {
+    observer_list_.RemoveObserver(observer);
+  }
 
   // CrosDisksClient override.
   void Mount(const std::string& source_path,
@@ -223,74 +234,50 @@ class CrosDisksClientImpl : public CrosDisksClient {
                        error_callback));
   }
 
-  // CrosDisksClient override.
-  void SetMountEventHandler(
-      const MountEventHandler& mount_event_handler) override {
-    static const SignalEventTuple kSignalEventTuples[] = {
-      { cros_disks::kDeviceAdded, CROS_DISKS_DEVICE_ADDED },
-      { cros_disks::kDeviceScanned, CROS_DISKS_DEVICE_SCANNED },
-      { cros_disks::kDeviceRemoved, CROS_DISKS_DEVICE_REMOVED },
-      { cros_disks::kDiskAdded, CROS_DISKS_DISK_ADDED },
-      { cros_disks::kDiskChanged, CROS_DISKS_DISK_CHANGED },
-      { cros_disks::kDiskRemoved, CROS_DISKS_DISK_REMOVED },
-    };
-    const size_t kNumSignalEventTuples = arraysize(kSignalEventTuples);
+ protected:
+  void Init(dbus::Bus* bus) override {
+    proxy_ = bus->GetObjectProxy(
+        cros_disks::kCrosDisksServiceName,
+        dbus::ObjectPath(cros_disks::kCrosDisksServicePath));
 
-    for (size_t i = 0; i < kNumSignalEventTuples; ++i) {
+    // Register handlers for D-Bus signals.
+    constexpr SignalEventTuple kSignalEventTuples[] = {
+        {cros_disks::kDeviceAdded, CROS_DISKS_DEVICE_ADDED},
+        {cros_disks::kDeviceScanned, CROS_DISKS_DEVICE_SCANNED},
+        {cros_disks::kDeviceRemoved, CROS_DISKS_DEVICE_REMOVED},
+        {cros_disks::kDiskAdded, CROS_DISKS_DISK_ADDED},
+        {cros_disks::kDiskChanged, CROS_DISKS_DISK_CHANGED},
+        {cros_disks::kDiskRemoved, CROS_DISKS_DISK_REMOVED},
+    };
+    for (const auto& entry : kSignalEventTuples) {
       proxy_->ConnectToSignal(
-          cros_disks::kCrosDisksInterface,
-          kSignalEventTuples[i].signal_name,
-          base::Bind(&CrosDisksClientImpl::OnMountEvent,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     kSignalEventTuples[i].event_type,
-                     mount_event_handler),
-          base::Bind(&CrosDisksClientImpl::OnSignalConnected,
-                     weak_ptr_factory_.GetWeakPtr()));
+          cros_disks::kCrosDisksInterface, entry.signal_name,
+          base::BindRepeating(&CrosDisksClientImpl::OnMountEvent,
+                              weak_ptr_factory_.GetWeakPtr(), entry.event_type),
+          base::BindOnce(&CrosDisksClientImpl::OnSignalConnected,
+                         weak_ptr_factory_.GetWeakPtr()));
     }
-  }
 
-  // CrosDisksClient override.
-  void SetMountCompletedHandler(
-      const MountCompletedHandler& mount_completed_handler) override {
     proxy_->ConnectToSignal(
-        cros_disks::kCrosDisksInterface,
-        cros_disks::kMountCompleted,
-        base::Bind(&CrosDisksClientImpl::OnMountCompleted,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   mount_completed_handler),
-        base::Bind(&CrosDisksClientImpl::OnSignalConnected,
-                   weak_ptr_factory_.GetWeakPtr()));
-  }
+        cros_disks::kCrosDisksInterface, cros_disks::kMountCompleted,
+        base::BindRepeating(&CrosDisksClientImpl::OnMountCompleted,
+                            weak_ptr_factory_.GetWeakPtr()),
+        base::BindOnce(&CrosDisksClientImpl::OnSignalConnected,
+                       weak_ptr_factory_.GetWeakPtr()));
 
-  // CrosDisksClient override.
-  void SetFormatCompletedHandler(
-      const FormatCompletedHandler& format_completed_handler) override {
     proxy_->ConnectToSignal(
-        cros_disks::kCrosDisksInterface,
-        cros_disks::kFormatCompleted,
-        base::Bind(&CrosDisksClientImpl::OnFormatCompleted,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   format_completed_handler),
-        base::Bind(&CrosDisksClientImpl::OnSignalConnected,
-                   weak_ptr_factory_.GetWeakPtr()));
-  }
+        cros_disks::kCrosDisksInterface, cros_disks::kFormatCompleted,
+        base::BindRepeating(&CrosDisksClientImpl::OnFormatCompleted,
+                            weak_ptr_factory_.GetWeakPtr()),
+        base::BindOnce(&CrosDisksClientImpl::OnSignalConnected,
+                       weak_ptr_factory_.GetWeakPtr()));
 
-  // CrosDisksClient override.
-  void SetRenameCompletedHandler(
-      const RenameCompletedHandler& rename_completed_handler) override {
     proxy_->ConnectToSignal(
         cros_disks::kCrosDisksInterface, cros_disks::kRenameCompleted,
-        base::Bind(&CrosDisksClientImpl::OnRenameCompleted,
-                   weak_ptr_factory_.GetWeakPtr(), rename_completed_handler),
-        base::Bind(&CrosDisksClientImpl::OnSignalConnected,
-                   weak_ptr_factory_.GetWeakPtr()));
-  }
-
- protected:
-  void Init(dbus::Bus* bus) override {
-    proxy_ = bus->GetObjectProxy(
-        cros_disks::kCrosDisksServiceName,
-        dbus::ObjectPath(cros_disks::kCrosDisksServicePath));
+        base::BindRepeating(&CrosDisksClientImpl::OnRenameCompleted,
+                            weak_ptr_factory_.GetWeakPtr()),
+        base::BindOnce(&CrosDisksClientImpl::OnSignalConnected,
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
  private:
@@ -429,52 +416,62 @@ class CrosDisksClientImpl : public CrosDisksClient {
     callback.Run(disk);
   }
 
-  // Handles mount event signals and calls |handler|.
-  void OnMountEvent(MountEventType event_type,
-                    MountEventHandler handler,
-                    dbus::Signal* signal) {
+  // Handles mount event signals and notifies observers.
+  void OnMountEvent(MountEventType event_type, dbus::Signal* signal) {
     dbus::MessageReader reader(signal);
     std::string device;
     if (!reader.PopString(&device)) {
       LOG(ERROR) << "Invalid signal: " << signal->ToString();
       return;
     }
-    handler.Run(event_type, device);
+
+    for (auto& observer : observer_list_)
+      observer.OnMountEvent(event_type, device);
   }
 
-  // Handles MountCompleted signal and calls |handler|.
-  void OnMountCompleted(MountCompletedHandler handler, dbus::Signal* signal) {
+  // Handles MountCompleted signal and notifies observers.
+  void OnMountCompleted(dbus::Signal* signal) {
     dbus::MessageReader reader(signal);
     MountEntry entry;
     if (!ReadMountEntryFromDbus(&reader, &entry)) {
       LOG(ERROR) << "Invalid signal: " << signal->ToString();
       return;
     }
-    handler.Run(entry);
+
+    for (auto& observer : observer_list_)
+      observer.OnMountCompleted(entry);
   }
 
-  // Handles FormatCompleted signal and calls |handler|.
-  void OnFormatCompleted(FormatCompletedHandler handler, dbus::Signal* signal) {
+  // Handles FormatCompleted signal and notifies observers.
+  void OnFormatCompleted(dbus::Signal* signal) {
     dbus::MessageReader reader(signal);
     uint32_t error_code = 0;
     std::string device_path;
     if (!reader.PopUint32(&error_code) || !reader.PopString(&device_path)) {
       LOG(ERROR) << "Invalid signal: " << signal->ToString();
       return;
     }
-    handler.Run(static_cast<FormatError>(error_code), device_path);
+
+    for (auto& observer : observer_list_) {
+      observer.OnFormatCompleted(static_cast<FormatError>(error_code),
+                                 device_path);
+    }
   }
 
-  // Handles RenameCompleted signal and calls |handler|.
-  void OnRenameCompleted(RenameCompletedHandler handler, dbus::Signal* signal) {
+  // Handles RenameCompleted signal and notifies observers.
+  void OnRenameCompleted(dbus::Signal* signal) {
     dbus::MessageReader reader(signal);
     uint32_t error_code = 0;
     std::string device_path;
     if (!reader.PopUint32(&error_code) || !reader.PopString(&device_path)) {
       LOG(ERROR) << "Invalid signal: " << signal->ToString();
       return;
     }
-    handler.Run(static_cast<RenameError>(error_code), device_path);
+
+    for (auto& observer : observer_list_) {
+      observer.OnRenameCompleted(static_cast<RenameError>(error_code),
+                                 device_path);
+    }
   }
 
   // Handles the result of signal connection setup.
@@ -487,6 +484,8 @@ class CrosDisksClientImpl : public CrosDisksClient {
 
   dbus::ObjectProxy* proxy_;
 
+  base::ObserverList<Observer> observer_list_;
+
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<CrosDisksClientImpl> weak_ptr_factory_;

chromeos/dbus/cros_disks_client.h

@@ -280,32 +280,35 @@ class CHROMEOS_EXPORT CrosDisksClient : public DBusClient {
   typedef base::Callback<void(const DiskInfo& disk_info)>
       GetDevicePropertiesCallback;
 
-  // A callback to handle MountCompleted signal.
-  typedef base::Callback<void(const MountEntry& entry)> MountCompletedHandler;
-
-  // A callback to handle FormatCompleted signal.
-  // The first argument is the error code.
-  // The second argument is the device path.
-  typedef base::Callback<void(FormatError error_code,
-                              const std::string& device_path)>
-      FormatCompletedHandler;
-
-  // A callback to handle RenameCompleted signal.
-  // The first argument is the error code.
-  // The second argument is the device path.
-  typedef base::Callback<void(RenameError error_code,
-                              const std::string& device_path)>
-      RenameCompletedHandler;
-
-  // A callback to handle mount events.
-  // The first argument is the event type.
-  // The second argument is the device path.
-  typedef base::Callback<void(MountEventType event_type,
-                              const std::string& device_path)>
-      MountEventHandler;
+  class Observer {
+   public:
+    // Called when a mount event signal is received.
+    virtual void OnMountEvent(MountEventType event_type,
+                              const std::string& device_path) = 0;
+
+    // Called when a MountCompleted signal is received.
+    virtual void OnMountCompleted(const MountEntry& entry) = 0;
+
+    // Called when a FormatCompleted signal is received.
+    virtual void OnFormatCompleted(FormatError error_code,
+                                   const std::string& device_path) = 0;
+
+    // Called when a RenameCompleted signal is received.
+    virtual void OnRenameCompleted(RenameError error_code,
+                                   const std::string& device_path) = 0;
+
+   protected:
+    virtual ~Observer() = default;
+  };
 
   ~CrosDisksClient() override;
 
+  // Registers the given |observer| to listen D-Bus signals.
+  virtual void AddObserver(Observer* observer) = 0;
+
+  // Unregisters the |observer| from this instance.
+  virtual void RemoveObserver(Observer* observer) = 0;
+
   // Calls Mount method.  |callback| is called after the method call succeeds,
   // otherwise, |error_callback| is called.
   // When mounting an archive, caller may set two optional arguments:
@@ -367,26 +370,6 @@ class CHROMEOS_EXPORT CrosDisksClient : public DBusClient {
                                    const GetDevicePropertiesCallback& callback,
                                    const base::Closure& error_callback) = 0;
 
-  // Registers |mount_event_handler| as a callback to be invoked when a mount
-  // event signal is received.
-  virtual void SetMountEventHandler(
-      const MountEventHandler& mount_event_handler) = 0;
-
-  // Registers |mount_completed_handler| as a callback to be invoked when a
-  // MountCompleted signal is received.
-  virtual void SetMountCompletedHandler(
-      const MountCompletedHandler& mount_completed_handler) = 0;
-
-  // Registers |format_completed_handler| as a callback to be invoked when a
-  // FormatCompleted signal is received.
-  virtual void SetFormatCompletedHandler(
-      const FormatCompletedHandler& format_completed_handler) = 0;
-
-  // Registers |rename_completed_handler| as a callback to be invoked when a
-  // RenameCompleted signal is received.
-  virtual void SetRenameCompletedHandler(
-      const RenameCompletedHandler& rename_completed_handler) = 0;
-
   // Factory function, creates a new instance and returns ownership.
   // For normal usage, access the singleton via DBusThreadManager::Get().
   static CrosDisksClient* Create(DBusClientImplementationType type);