Add GetConnectionInfo function for BluetoothDevice, replacing the existing

chromeos/dbus/bluetooth_device_client.cc

Index: chromeos/dbus/bluetooth_device_client.cc
diff --git a/chromeos/dbus/bluetooth_device_client.cc b/chromeos/dbus/bluetooth_device_client.cc
index 777b1aa1cfb9307f36cbd9ec4240bdc997a1af80..00702105b4082f60f15e72c3c390b2e526d2338f 100644
--- a/chromeos/dbus/bluetooth_device_client.cc
+++ b/chromeos/dbus/bluetooth_device_client.cc
@@ -16,6 +16,13 @@
 
 namespace chromeos {
 
+namespace {
+
+// Value returned for the the RSSI or TX power if it cannot be read.
+const int kUnknownPower = 127;
+
+}  // namespace
+
 const char BluetoothDeviceClient::kNoResponseError[] =
     "org.chromium.Error.NoResponse";
 const char BluetoothDeviceClient::kUnknownDeviceError[] =
@@ -41,10 +48,6 @@ BluetoothDeviceClient::Properties::Properties(
   RegisterProperty(bluetooth_device::kLegacyPairingProperty, &legacy_pairing);
   RegisterProperty(bluetooth_device::kModaliasProperty, &modalias);
   RegisterProperty(bluetooth_device::kRSSIProperty, &rssi);
-  RegisterProperty(bluetooth_device::kConnectionRSSI, &connection_rssi);
-  RegisterProperty(bluetooth_device::kConnectionTXPower, &connection_tx_power);
-  RegisterProperty(bluetooth_device::kConnectionTXPowerMax,
-                   &connection_tx_power_max);
 }
 
 BluetoothDeviceClient::Properties::~Properties() {
@@ -273,37 +276,12 @@ class BluetoothDeviceClientImpl
   }
 
   // BluetoothDeviceClient override.
-  virtual void StartConnectionMonitor(
-      const dbus::ObjectPath& object_path,
-      const base::Closure& callback,
-      const ErrorCallback& error_callback) override {
-    dbus::MethodCall method_call(bluetooth_device::kBluetoothDeviceInterface,
-                                 bluetooth_device::kStartConnectionMonitor);
-
-    dbus::ObjectProxy* object_proxy =
-        object_manager_->GetObjectProxy(object_path);
-    if (!object_proxy) {
-      error_callback.Run(kUnknownDeviceError, "");
-      return;
-    }
-    object_proxy->CallMethodWithErrorCallback(
-        &method_call,
-        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::Bind(&BluetoothDeviceClientImpl::OnSuccess,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   callback),
-        base::Bind(&BluetoothDeviceClientImpl::OnError,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   error_callback));
-  }
-
-  // BluetoothDeviceClient override.
-  virtual void StopConnectionMonitor(
-      const dbus::ObjectPath& object_path,
-      const base::Closure& callback,
-      const ErrorCallback& error_callback) override {
-    dbus::MethodCall method_call(bluetooth_device::kBluetoothDeviceInterface,
-                                 bluetooth_device::kStopConnectionMonitor);
+  void GetConnInfo(const dbus::ObjectPath& object_path,
+                   const ConnInfoCallback& callback,
+                   const ErrorCallback& error_callback) override {
+    dbus::MethodCall method_call(
+        bluetooth_plugin_device::kBluetoothPluginInterface,
+        bluetooth_plugin_device::kGetConnInfo);
 
     dbus::ObjectProxy* object_proxy =
         object_manager_->GetObjectProxy(object_path);
@@ -312,14 +290,11 @@ class BluetoothDeviceClientImpl
       return;
     }
     object_proxy->CallMethodWithErrorCallback(
-        &method_call,
-        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::Bind(&BluetoothDeviceClientImpl::OnSuccess,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   callback),
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&BluetoothDeviceClientImpl::OnGetConnInfoSuccess,
+                   weak_ptr_factory_.GetWeakPtr(), callback),
         base::Bind(&BluetoothDeviceClientImpl::OnError,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   error_callback));
+                   weak_ptr_factory_.GetWeakPtr(), error_callback));
   }
 
  protected:
@@ -365,6 +340,26 @@ class BluetoothDeviceClientImpl
     callback.Run();
   }
 
+  // Called when a response for the GetConnInfo method is received.
+  void OnGetConnInfoSuccess(const ConnInfoCallback& callback,
+                            dbus::Response* response) {
+    int16 rssi = kUnknownPower;
+    int16 transmit_power = kUnknownPower;
+    int16 max_transmit_power = kUnknownPower;
+
+    if (response) {
+      dbus::MessageReader reader(response);
+      bool success = reader.PopInt16(&rssi);
+      success &= reader.PopInt16(&transmit_power);
+      success &= reader.PopInt16(&max_transmit_power);

[armansito, 2015/01/06 20:36:07]

I find this a bit difficult to read. It might be a little more clear to place
this within an if statement and short circuit in case of failure? Maybe do:

  if (!response)
    LOG(ERROR) << ...
  else if (!reader.PopInt16(&rssi) || !reader.PopInt16(..) || !reader...)
    LOG(WARNING) << "Received invalid response";

  callback.Run(..)

This way you would also avoid the DCHECK, since you wouldn't be asserting on the
behavior of a message received from a different process. Just a suggestion.

[Tim Song, 2015/01/06 22:30:50]

Done. I don't think it's defined to create a MessageReader with a null response,
so I kept the outer if statement.

[armansito, 2015/01/06 22:39:29]

You can also do an early return if response is NULL. I prefer not to have many
nested levels of if statements since the code just ends up becoming confusing.
So maybe just run the callback and return if response is NULL, otherwise you
create a MessageReader and parse the response. I'd rather run the callback in
two different places than have many levels of ifs.

[Tim Song, 2015/01/06 23:02:17]

Done.

+      DCHECK(success) << "Arguments for GetConnInfo invalid.";
+    } else {
+      LOG(ERROR) << "GetConnInfo suceeded, but no response recieved.";
+    }
+
+    callback.Run(rssi, transmit_power, max_transmit_power);
+  }
+
   // Called when a response for a failed method call is received.
   void OnError(const ErrorCallback& error_callback,
                dbus::ErrorResponse* response) {