Add GetConnectionInfo function for BluetoothDevice, replacing the existing

chromeos/dbus/bluetooth_device_client.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/dbus/bluetooth_device_client.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_manager.h"
 #include "dbus/object_path.h"
 #include "dbus/object_proxy.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 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[] =
     "org.chromium.Error.UnknownDevice";
 
 BluetoothDeviceClient::Properties::Properties(
     dbus::ObjectProxy* object_proxy,
     const std::string& interface_name,
     const PropertyChangedCallback& callback)
     : dbus::PropertySet(object_proxy, interface_name, callback) {
   RegisterProperty(bluetooth_device::kAddressProperty, &address);
   RegisterProperty(bluetooth_device::kNameProperty, &name);
   RegisterProperty(bluetooth_device::kIconProperty, &icon);
   RegisterProperty(bluetooth_device::kClassProperty, &bluetooth_class);
   RegisterProperty(bluetooth_device::kAppearanceProperty, &appearance);
   RegisterProperty(bluetooth_device::kUUIDsProperty, &uuids);
   RegisterProperty(bluetooth_device::kPairedProperty, &paired);
   RegisterProperty(bluetooth_device::kConnectedProperty, &connected);
   RegisterProperty(bluetooth_device::kTrustedProperty, &trusted);
   RegisterProperty(bluetooth_device::kBlockedProperty, &blocked);
   RegisterProperty(bluetooth_device::kAliasProperty, &alias);
   RegisterProperty(bluetooth_device::kAdapterProperty, &adapter);
   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() {
 }
 
 
 // The BluetoothDeviceClient implementation used in production.
 class BluetoothDeviceClientImpl
     : public BluetoothDeviceClient,
       public dbus::ObjectManager::Interface {
@@ skipping 208 matching lines @@
     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 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);
+  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);
-    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);
-
-    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),
+        &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:
   virtual void Init(dbus::Bus* bus) override {
     object_manager_ = bus->GetObjectManager(
         bluetooth_object_manager::kBluetoothObjectManagerServiceName,
         dbus::ObjectPath(
             bluetooth_object_manager::kBluetoothObjectManagerServicePath));
     object_manager_->RegisterInterface(
         bluetooth_device::kBluetoothDeviceInterface, this);
@@ skipping 25 matching lines @@
                       DevicePropertyChanged(object_path, property_name));
   }
 
   // Called when a response for successful method call is received.
   void OnSuccess(const base::Closure& callback,
                  dbus::Response* response) {
     DCHECK(response);
     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) {
     // Error response has optional error message argument.
     std::string error_name;
     std::string error_message;
     if (response) {
       dbus::MessageReader reader(response);
       error_name = response->GetErrorName();
       reader.PopString(&error_message);
@@ skipping 22 matching lines @@
 }
 
 BluetoothDeviceClient::~BluetoothDeviceClient() {
 }
 
 BluetoothDeviceClient* BluetoothDeviceClient::Create() {
   return new BluetoothDeviceClientImpl();
 }
 
 }  // namespace chromeos