Adding support for service scan on OS X

device/bluetooth/bluetooth_low_energy_device_mac.mm

Index: device/bluetooth/bluetooth_low_energy_device_mac.mm
diff --git a/device/bluetooth/bluetooth_low_energy_device_mac.mm b/device/bluetooth/bluetooth_low_energy_device_mac.mm
index 03981d77d1f15848b9ddcfadd71be1aa41f36548..580dbbbeda762d871367b98d57d5ad2bce0550d9 100644
--- a/device/bluetooth/bluetooth_low_energy_device_mac.mm
+++ b/device/bluetooth/bluetooth_low_energy_device_mac.mm
@@ -14,6 +14,8 @@
 #include "base/strings/sys_string_conversions.h"
 #include "device/bluetooth/bluetooth_adapter_mac.h"
 #include "device/bluetooth/bluetooth_device.h"
+#include "device/bluetooth/bluetooth_low_energy_peripheral_delegate.h"
+#include "device/bluetooth/bluetooth_remote_gatt_service_mac.h"
 
 using device::BluetoothDevice;
 using device::BluetoothLowEnergyDeviceMac;
@@ -27,6 +29,9 @@ BluetoothLowEnergyDeviceMac::BluetoothLowEnergyDeviceMac(
       peripheral_(peripheral, base::scoped_policy::RETAIN) {
   DCHECK(BluetoothAdapterMac::IsLowEnergyAvailable());
   DCHECK(peripheral_.get());
+  peripheral_delegate_.reset([[BluetoothLowEnergyPeripheralDelegate alloc]
+      initWithBluetoothLowEnergyDeviceMac:this]);
+  [peripheral_ setDelegate:peripheral_delegate_];
   identifier_ = GetPeripheralIdentifier(peripheral);
   hash_address_ = GetPeripheralHashAddress(peripheral);
   Update(advertisement_data, rssi);
@@ -226,10 +231,52 @@ void BluetoothLowEnergyDeviceMac::CreateGattConnectionImpl() {
   }
 }
 
+void BluetoothLowEnergyDeviceMac::DidConnectGatt() {
+  [GetPeripheral() discoverServices:nil];

[ortuno, 2016/05/06 16:03:33]

Could this be called from BluetoothAdapterMac::DidConnectPeripheral? That way
you don't need to override this function.

[jlebel, 2016/05/07 00:16:10]

Yes, of course, it can be done there. But it doesn't be logical. This is a call
to deal with the peripheral, that's why I wanted to do in
BluetoothLowEnergyDeviceMac.

[ortuno, 2016/05/09 19:54:00]

Ah, good point. Up to you.

+  BluetoothDeviceMac::DidConnectGatt();
+}
+
 void BluetoothLowEnergyDeviceMac::DisconnectGatt() {
   GetMacAdapter()->DisconnectGatt(this);
 }
 
+void BluetoothLowEnergyDeviceMac::DidDiscoverPrimaryServices(NSError* error) {
+  if (error) {

[ortuno, 2016/05/06 16:03:33]

I think for now let's just VLOG it or UMA it to match other platforms. I opened
an issue to decide what's the behavior we want. I'm leaning towards this though.
http://crbug.com/609844

[jlebel, 2016/05/07 00:16:10]

Done.

+    // TODO(http://crbug.com/609320): Need to pass the error.
+    DisconnectGatt();
+    return;
+  }
+  for (CBService* cb_service in GetPeripheral().services) {
+    BluetoothRemoteGattServiceMac* gatt_service =
+        GetBluetoothRemoteGattService(cb_service);
+    if (!gatt_service) {
+      gatt_service = new BluetoothRemoteGattServiceMac(this, cb_service, true);

[ortuno, 2016/05/06 16:03:33]

To make the code easier to read, you should comment bools and ints whose meaning
is not clear, in this case:

true /* is_primary */

[jlebel, 2016/05/07 00:16:11]

Done.

+      gatt_services_.add(

[ortuno, 2016/05/06 16:03:33]

nit: I would save the result from here and then DCHECK the value was actually
inserted.

[jlebel, 2016/05/07 00:16:11]

a DCHECK like:
DCHECK(GetPeripheralIdentifier(cb_service == gatt_service);
?

[ortuno, 2016/05/09 19:54:00]

More like:

auto result_iter = gatt_services_.add(
    gatt_service->GetIdentifier(),
    base::WrapUnique(gatt_service));

DCHECK(result_iter->second);

+          gatt_service->GetIdentifier(),
+          std::unique_ptr<BluetoothRemoteGattService>(gatt_service));

[ortuno, 2016/05/06 16:03:33]

nit: maybe you can just use base::WrapUnique() instead of std::unique_ptr<....>.

[jlebel, 2016/05/07 00:16:10]

Done.

+      adapter_->NotifyGattServiceAdded(gatt_service);

[ortuno, 2016/05/06 16:03:33]

It's a bit dangerous to be passing the raw pointer here because adding the
unique_ptr to the map could have deleted the object. But I think you are good if
you DCHECK that the value was actually added above.

Also fyi, Chrome OS calls this after all characteristics and descriptors have
been discovered for the service but Windows and Bluez do it as soon as the
service is discovered. I opened an issue so that we can define what the correct
behavior is http://crbug.com/609642. You don't need to do anything for this
patch though.

[jlebel, 2016/05/07 00:16:11]

It is nicer to create a service so I can track which services has fully
discovered and which one are still pending. So I would definitely prefer the
windows/bluez version.

+    }
+  }
+  SetGattServicesDiscoveryComplete(true);
+  adapter_->NotifyGattServicesDiscovered(this);

[ortuno, 2016/05/06 16:03:33]

As discussed, this should be called only after all
services/characteristics/descriptors have been discovered. But I do think it's
OK to call it from here until we add support for the other objects. I would add
a TODO to remove this so that we don't forget.

[jlebel, 2016/05/07 00:16:10]

Done.

+}
+
+void BluetoothLowEnergyDeviceMac::DidModifyServices(

[ortuno, 2016/05/06 16:03:33]

Does this get called when disconnecting?

[jlebel, 2016/05/07 00:16:10]

No. It is called when the services are modified.

+    NSArray* invalidatedServices) {
+  for (CBService* cb_service in invalidatedServices) {
+    BluetoothRemoteGattServiceMac* gatt_service =
+        GetBluetoothRemoteGattService(cb_service);
+    DCHECK(gatt_service);
+    if ([GetPeripheral().services containsObject:cb_service]) {

[ortuno, 2016/05/06 16:03:33]

Based on [1] it seems we can no longer use the *any* of the services in the
invalidatedServices list. So I think you need to call NotifyGattServiceRemoved
for each of the services in invalidatedServices.

Also keep in mind that when you call NotifyGattServiceRemoved you need to pass
in a pointer to the service and that service can no longer be in the map. You
can look at the Linux code to see how they achieve that.[2]

[1]
https://developer.apple.com/library/ios/documentation/CoreBluetooth/Reference...:
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...
Edit

[jlebel, 2016/05/07 00:16:10]

Done.

+      adapter_->NotifyGattServiceChanged(gatt_service);
+    } else {
+      gatt_services_.take_and_erase(gatt_service->GetIdentifier());
+    }
+  }
+  SetGattServicesDiscoveryComplete(false);
+  [GetPeripheral() discoverServices:nil];
+}
+
 // static
 std::string BluetoothLowEnergyDeviceMac::GetPeripheralIdentifier(
     CBPeripheral* peripheral) {
@@ -258,8 +305,32 @@ CBPeripheral* BluetoothLowEnergyDeviceMac::GetPeripheral() {
   return peripheral_;
 }
 
+device::BluetoothRemoteGattServiceMac*
+BluetoothLowEnergyDeviceMac::GetBluetoothRemoteGattService(
+    CBService* cb_service) const {
+  for (GattServiceMap::const_iterator it = gatt_services_.begin();
+       it != gatt_services_.end(); ++it) {
+    device::BluetoothRemoteGattService* gatt_service = it->second;
+    device::BluetoothRemoteGattServiceMac* gatt_service_mac =
+        static_cast<BluetoothRemoteGattServiceMac*>(gatt_service);
+    if (gatt_service_mac->GetService() == cb_service)
+      return gatt_service_mac;
+  }
+  return NULL;

[ortuno, 2016/05/06 16:03:33]

nit: return nullptr;

[jlebel, 2016/05/07 00:16:11]

Done.

+}
+
 void BluetoothLowEnergyDeviceMac::DidDisconnectPeripheral(
     BluetoothDevice::ConnectErrorCode error_code) {
+  SetGattServicesDiscoveryComplete(false);
+  // Explicitly take and erase GATT services one by one to ensure that calling
+  // GetGattService on removed service in GattServiceRemoved returns null.
+  std::vector<std::string> service_keys;
+  for (const auto& gatt_service : gatt_services_) {
+    service_keys.push_back(gatt_service.first);
+  }
+  for (const auto& key : service_keys) {
+    gatt_services_.take_and_erase(key);
+  }

[ortuno, 2016/05/06 16:03:33]

You could also swap it with a local map and erase from there:


GattServiceMap gatt_services_swapped;
gatt_services_swapped.swap(gatt_services_);
for (const auto& pair : gatt_services_swapped) {
  gatt_services_swapped.take_and_erase(pair.first);
}

[jlebel, 2016/05/07 00:16:11]

I have a crash on the second service removed. 

[ortuno, 2016/05/09 19:54:00]

Hmm really? I just tried it with the tests and it seemed to work. Did you have
any problems with real devices?

   if (create_gatt_connection_error_callbacks_.empty()) {
     // TODO(http://crbug.com/585897): Need to pass the error.
     DidDisconnectGatt();