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

device/bluetooth/bluetooth_device_chromeos.cc

Index: device/bluetooth/bluetooth_device_chromeos.cc
diff --git a/device/bluetooth/bluetooth_device_chromeos.cc b/device/bluetooth/bluetooth_device_chromeos.cc
index 996fb23a0c9b83a2e67fd57f82dcac1a7f171662..b3fa8d073ff18921c7fa1fbcfa512ce443e4eb18 100644
--- a/device/bluetooth/bluetooth_device_chromeos.cc
+++ b/device/bluetooth/bluetooth_device_chromeos.cc
@@ -46,6 +46,7 @@ BluetoothDeviceChromeOS::BluetoothDeviceChromeOS(
     adapter_(adapter),
     pairing_delegate_(NULL),
     connecting_applications_counter_(0),
+    service_records_loaded_(false),
     weak_ptr_factory_(this) {
 }
 
@@ -64,6 +65,16 @@ BluetoothDeviceChromeOS::GetServices() const {
 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_,
@@ -371,9 +382,7 @@ void BluetoothDeviceChromeOS::OnCreateDevice(
           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);
@@ -416,26 +425,55 @@ void BluetoothDeviceChromeOS::CollectServiceRecordsCallback(
     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) {