Bluetooth: Add a "connectable" property to the BluetoothDevice.

device/bluetooth/bluetooth_device_chromeos.cc

 // Copyright (c) 2012 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 "device/bluetooth/bluetooth_device_chromeos.h"
 
 #include <map>
 #include <string>
 #include <vector>
 
@@ skipping 28 matching lines @@
 using device::BluetoothSocket;
 
 namespace chromeos {
 
 BluetoothDeviceChromeOS::BluetoothDeviceChromeOS(
     BluetoothAdapterChromeOS* adapter)
   : BluetoothDevice(),
     adapter_(adapter),
     pairing_delegate_(NULL),
     connecting_applications_counter_(0),
+    service_records_loaded_(false),
     weak_ptr_factory_(this) {
 }
 
 BluetoothDeviceChromeOS::~BluetoothDeviceChromeOS() {
 }
 
 bool BluetoothDeviceChromeOS::IsPaired() const {
   return !object_path_.value().empty();
 }
 
 const BluetoothDevice::ServiceList&
 BluetoothDeviceChromeOS::GetServices() const {
   return service_uuids_;
 }
 
 void BluetoothDeviceChromeOS::GetServiceRecords(
     const ServiceRecordsCallback& callback,
     const ErrorCallback& error_callback) {
+  if (service_records_loaded_) {
+    callback.Run(service_records_);
+  } else {
+    UpdateServiceRecords(callback, error_callback);
+  }

[keybuk, 2013/02/21 21:08:58]

One of the reasons that GetServiceRecords() always calls DiscoverServices() is
that the service list can be dynamic (e.g. services only published on Android
while the app is actually running)

This would seem to break that by caching the values and never updating. Is there
a way to alter this so that it always attempts a refresh, and if that fails,
falls back to the cache instead of giving an error?

[deymo, 2013/02/22 23:09:09]

Done.

+}
+
+void BluetoothDeviceChromeOS::UpdateServiceRecords(
+    const ServiceRecordsCallback& callback,
+    const ErrorCallback& error_callback) {
   DBusThreadManager::Get()->GetBluetoothDeviceClient()->
       DiscoverServices(
           object_path_,
           "",  // empty pattern to browse all services
           base::Bind(&BluetoothDeviceChromeOS::CollectServiceRecordsCallback,
                      weak_ptr_factory_.GetWeakPtr(),
                      callback,
                      error_callback));
 }
 
@@ skipping 287 matching lines @@
   // we can connect after rebooting. This information is part of the
   // pairing information of the device, and is unique to the combination
   // of our bluetooth address and the device's bluetooth address. A
   // different host needs a new pairing, so it's not useful to sync.
   DBusThreadManager::Get()->GetBluetoothDeviceClient()->
       GetProperties(object_path_)->trusted.Set(
           true,
           base::Bind(&BluetoothDeviceChromeOS::OnSetTrusted,
                      weak_ptr_factory_.GetWeakPtr(),
                      callback,
-                     base::Bind(error_callback,
-                                ERROR_UNKNOWN)));
-  // TODO(deymo): Replace ERROR_UNKNOWN with an appropriate new constant.
+                     error_callback));
 
   // Connect application-layer protocols.
   ConnectApplications(callback, error_callback);
 }
 
 void BluetoothDeviceChromeOS::OnCreateDeviceError(
     const ConnectErrorCallback& error_callback,
     const std::string& error_name,
     const std::string& error_message) {
   // The default |error_code| is an unknown error.
@@ skipping 22 matching lines @@
     const ServiceRecordsCallback& callback,
     const ErrorCallback& error_callback,
     const dbus::ObjectPath& device_path,
     const BluetoothDeviceClient::ServiceMap& service_map,
     bool success) {
   if (!success) {
     error_callback.Run();
     return;
   }
 
-  ScopedVector<BluetoothServiceRecord> records;
+  // Update the cache.
+  service_records_loaded_ = true;
+  service_records_.clear();

[keybuk, 2013/02/21 21:08:58]

This gives a time period where another thread would see an empty service records
list before it's repopulated?

[deymo, 2013/02/22 23:09:09]

As discussed, this is ok.

[keybuk, 2013/02/22 23:50:03]

after discussion, this seems okay - since all BDCOs code is run on the same
thread

+  // TODO(deymo): Perhaps don't update the cache if the new SDP information is
+  // empty and we had something before. Some devices only answer this
+  // information while paired. This requires more investigation.
   for (BluetoothDeviceClient::ServiceMap::const_iterator i =
       service_map.begin(); i != service_map.end(); ++i) {
-    records.push_back(
+    service_records_.push_back(
         new BluetoothServiceRecordChromeOS(address(), i->second));
   }
-  callback.Run(records);
+  callback.Run(service_records_);
 }
 
-void BluetoothDeviceChromeOS::OnSetTrusted(const base::Closure& callback,
-                                           const ErrorCallback& error_callback,
-                                           bool success) {
+void BluetoothDeviceChromeOS::OnSetTrusted(
+    const base::Closure& callback,
+    const ConnectErrorCallback& error_callback,
+    bool success) {
   if (success) {
-    callback.Run();
+    UpdateServiceRecords(
+        base::Bind(&BluetoothDeviceChromeOS::OnServicesUpdatedAfterPair,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   callback),
+        base::Bind(error_callback, ERROR_SDP_FAILED));

[keybuk, 2013/02/21 21:08:58]

Could failure be handled better here? This adds another user error case where we
could perhaps have a "default behavior" and just log the error.

[deymo, 2013/02/22 23:09:09]

Not having the SDP information is not a "fatal" connect error. Changed this to
call callback anyway.

   } else {
     LOG(WARNING) << "Failed to set device as trusted: " << address_;
-    error_callback.Run();
+    error_callback.Run(ERROR_UNKNOWN);
+    // TODO(deymo): Replace ERROR_UNKNOWN with an appropriate new constant.
   }
 }
 
+void BluetoothDeviceChromeOS::OnServicesUpdatedAfterPair(
+    const base::Closure& callback,
+    const ServiceRecordList& list) {
+  // Update the BluetoothDevice::connectable_ property.

[keybuk, 2013/02/21 21:08:58]

nit: a comment explaining the boolean logic would be nice for future
maintainers, e.g. "in the cases of multiple HID profiles, all must be normally
connectable for the property to be true"

[deymo, 2013/02/22 23:09:09]

We have a simplification of the device here. We have only one device object with
its connected and connectable status, but many profiles (and we just try to
connect to all of them). I added a note about this.

+  bool hid_normally_connectable = true;
+  bool hid_reconnect_initiate = true;
+  for (ServiceRecordList::const_iterator it = list.begin();
+      it != list.end(); ++it) {
+    hid_normally_connectable =
+        hid_normally_connectable && (*it)->hid_normally_connectable();
+    hid_reconnect_initiate =
+        hid_reconnect_initiate && (*it)->hid_reconnect_initiate();
+  }
+  connectable_ = hid_normally_connectable || !hid_reconnect_initiate;
+
+  callback.Run();
+}
+
 void BluetoothDeviceChromeOS::ConnectApplications(
     const base::Closure& callback,
     const ConnectErrorCallback& error_callback) {
   // Introspect the device object to determine supported applications.
   DBusThreadManager::Get()->GetIntrospectableClient()->
       Introspect(bluetooth_device::kBluetoothDeviceServiceName,
                  object_path_,
                  base::Bind(&BluetoothDeviceChromeOS::OnIntrospect,
                             weak_ptr_factory_.GetWeakPtr(),
                             callback,
@@ skipping 293 matching lines @@
 }
 
 
 // static
 BluetoothDeviceChromeOS* BluetoothDeviceChromeOS::Create(
     BluetoothAdapterChromeOS* adapter) {
   return new BluetoothDeviceChromeOS(adapter);
 }
 
 }  // namespace chromeos