bluetooth: Refactor DBus clients to use CallMethodWithErrorResponse

Currently Bluetooth DBus clients use CallMethodWithErrorCallback which
takes two callbacks one for success and one for failure.

This patterns is hard to use for clients that implement a Mojo
interface for example BluetoothSystem, so this CL deprecates the methods
that take two callbacks and introduces one that takes a single callback.

BluetoothSystem will soon replace all other clients of these DBus
clients so it makes sense to move away from the two callbacks pattern and
to the single callback pattern which BluetoothSystem can more easily
use.

Bug: 870192
Change-Id: I703a8b8ed277bf94de6adbef6cb954ac66f2b602
Reviewed-on: https://chromium-review.googlesource.com/c/1293292Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604026}

device/bluetooth/dbus/bluetooth_adapter_client.cc

@@ -95,6 +95,31 @@ void WriteAttribute(dbus::MessageWriter* writer,
   writer->CloseContainer(&struct_writer);
 }
 
+BluetoothAdapterClient::Error ErrorResponseToError(
+    dbus::ErrorResponse* response) {
+  BluetoothAdapterClient::Error error(BluetoothAdapterClient::kNoResponseError,
+                                      "");
+  if (response) {
+    dbus::MessageReader reader(response);
+    error.name = response->GetErrorName();
+    reader.PopString(&error.message);
+  }
+
+  return error;
+}
+
+void OnResponseAdapter(
+    const base::Closure& callback,
+    BluetoothAdapterClient::ErrorCallback error_callback,
+    const base::Optional<BluetoothAdapterClient::Error>& error) {
+  if (!error) {
+    callback.Run();
+    return;
+  }
+
+  std::move(error_callback).Run(error->name, error->message);
+}
+
 }  // namespace
 
 BluetoothAdapterClient::DiscoveryFilter::DiscoveryFilter() = default;
@@ -124,6 +149,10 @@ void BluetoothAdapterClient::DiscoveryFilter::CopyFrom(
     uuids.reset();
 }
 
+BluetoothAdapterClient::Error::Error(const std::string& name,
+                                     const std::string& message)
+    : name(name), message(message) {}
+
 const char BluetoothAdapterClient::kNoResponseError[] =
     "org.chromium.Error.NoResponse";
 const char BluetoothAdapterClient::kUnknownAdapterError[] =
@@ -206,25 +235,21 @@ class BluetoothAdapterClientImpl : public BluetoothAdapterClient,
 
   // BluetoothAdapterClient override.
   void StartDiscovery(const dbus::ObjectPath& object_path,
-                      const base::Closure& callback,
-                      ErrorCallback error_callback) override {
+                      ResponseCallback callback) override {
     dbus::MethodCall method_call(bluetooth_adapter::kBluetoothAdapterInterface,
                                  bluetooth_adapter::kStartDiscovery);
 
     dbus::ObjectProxy* object_proxy =
         object_manager_->GetObjectProxy(object_path);
     if (!object_proxy) {
-      std::move(error_callback).Run(kUnknownAdapterError, "");
+      std::move(callback).Run(Error(kUnknownAdapterError, ""));
       return;
     }
 
-    object_proxy->CallMethodWithErrorCallback(
+    object_proxy->CallMethodWithErrorResponse(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&BluetoothAdapterClientImpl::OnSuccess,
-                       weak_ptr_factory_.GetWeakPtr(), callback),
-        base::BindOnce(&BluetoothAdapterClientImpl::OnError,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       std::move(error_callback)));
+        base::BindOnce(&BluetoothAdapterClientImpl::OnResponse,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   // BluetoothAdapterClient override.
@@ -541,6 +566,17 @@ class BluetoothAdapterClientImpl : public BluetoothAdapterClient,
     std::move(error_callback).Run(error_name, error_message);
   }
 
+  void OnResponse(ResponseCallback callback,
+                  dbus::Response* response,
+                  dbus::ErrorResponse* error_response) {
+    if (response) {
+      std::move(callback).Run(base::nullopt);
+      return;
+    }
+
+    std::move(callback).Run(ErrorResponseToError(error_response));
+  }
+
   dbus::ObjectManager* object_manager_;
 
   // List of observers interested in event notifications from us.
@@ -563,4 +599,11 @@ BluetoothAdapterClient* BluetoothAdapterClient::Create() {
   return new BluetoothAdapterClientImpl;
 }
 
+void BluetoothAdapterClient::StartDiscovery(const dbus::ObjectPath& object_path,
+                                            const base::Closure& callback,
+                                            ErrorCallback error_callback) {
+  StartDiscovery(object_path, base::BindOnce(&OnResponseAdapter, callback,
+                                             std::move(error_callback)));
+}
+
 }  // namespace bluez

device/bluetooth/dbus/bluetooth_adapter_client.h

@@ -12,6 +12,7 @@
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "dbus/object_path.h"
 #include "dbus/property.h"
 #include "device/bluetooth/bluetooth_export.h"
@@ -47,6 +48,14 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterClient : public BluezDBusClient {
     DISALLOW_COPY_AND_ASSIGN(DiscoveryFilter);
   };
 
+  // Represent an error sent through DBus.
+  struct Error {
+    Error(const std::string& name, const std::string& message);
+
+    std::string name;
+    std::string message;
+  };
+
   // Structure of properties associated with bluetooth adapters.
   struct Properties : public dbus::PropertySet {
     // The Bluetooth device address of the adapter. Read-only.
@@ -147,10 +156,18 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterClient : public BluezDBusClient {
                                   const std::string& error_message)>
       ErrorCallback;
 
+  // Callback used by adapter methods to indicate that a response was
+  // received with an optional Error in case an error occurred.
+  using ResponseCallback =
+      base::OnceCallback<void(const base::Optional<Error>&)>;
+
   // Starts a device discovery on the adapter with object path |object_path|.
   virtual void StartDiscovery(const dbus::ObjectPath& object_path,
+                              ResponseCallback callback) = 0;
+  // DEPRECATED: Use StartDiscovery() above.
+  void StartDiscovery(const dbus::ObjectPath& object_path,
                       const base::Closure& callback,
-                              ErrorCallback error_callback) = 0;
+                      ErrorCallback error_callback);
 
   // Cancels any previous device discovery on the adapter with object path
   // |object_path|.

device/bluetooth/dbus/fake_bluetooth_adapter_client.cc

@@ -134,18 +134,17 @@ FakeBluetoothAdapterClient::GetProperties(const dbus::ObjectPath& object_path) {
 
 void FakeBluetoothAdapterClient::StartDiscovery(
     const dbus::ObjectPath& object_path,
-    const base::Closure& callback,
-    ErrorCallback error_callback) {
+    ResponseCallback callback) {
   if (object_path != dbus::ObjectPath(kAdapterPath)) {
     PostDelayedTask(
-        base::BindOnce(std::move(error_callback), kNoResponseError, ""));
+        base::BindOnce(std::move(callback), Error(kNoResponseError, "")));
     return;
   }
 
   ++discovering_count_;
   VLOG(1) << "StartDiscovery: " << object_path.value() << ", "
           << "count is now " << discovering_count_;
-  PostDelayedTask(callback);
+  PostDelayedTask(base::BindOnce(std::move(callback), base::nullopt));
 
   if (discovering_count_ == 1) {
     properties_->discovering.ReplaceValue(true);

device/bluetooth/dbus/fake_bluetooth_adapter_client.h

@@ -49,8 +49,7 @@ class DEVICE_BLUETOOTH_EXPORT FakeBluetoothAdapterClient
   std::vector<dbus::ObjectPath> GetAdapters() override;
   Properties* GetProperties(const dbus::ObjectPath& object_path) override;
   void StartDiscovery(const dbus::ObjectPath& object_path,
-                      const base::Closure& callback,
-                      ErrorCallback error_callback) override;
+                      ResponseCallback callback) override;
   void StopDiscovery(const dbus::ObjectPath& object_path,
                      const base::Closure& callback,
                      ErrorCallback error_callback) override;

services/device/bluetooth/bluetooth_system_unittest.cc

@@ -230,8 +230,7 @@ class DEVICE_BLUETOOTH_EXPORT TestBluetoothAdapterClient
   }
 
   void StartDiscovery(const dbus::ObjectPath& object_path,
-                      const base::Closure& callback,
-                      ErrorCallback error_callback) override {
+                      ResponseCallback callback) override {
     NOTIMPLEMENTED();
   }