bluetooth: macOS: Support for extra didConnectPeripheral event from macOS.

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 cff6ee533c6ecba3f9d00664ee6a6913532e4be5..0844b76acb2e327709e831810f845e9635d621ad 100644
--- a/device/bluetooth/bluetooth_low_energy_device_mac.mm
+++ b/device/bluetooth/bluetooth_low_energy_device_mac.mm
@@ -28,6 +28,7 @@ BluetoothLowEnergyDeviceMac::BluetoothLowEnergyDeviceMac(
     CBPeripheral* peripheral)
     : BluetoothDeviceMac(adapter),
       peripheral_(peripheral, base::scoped_policy::RETAIN),
+      connected_(false),
       discovery_pending_count_(0) {
   DCHECK(BluetoothAdapterMac::IsLowEnergyAvailable());
   DCHECK(peripheral_.get());
@@ -96,7 +97,11 @@ bool BluetoothLowEnergyDeviceMac::IsConnected() const {
 }
 
 bool BluetoothLowEnergyDeviceMac::IsGattConnected() const {
-  return ([peripheral_ state] == CBPeripheralStateConnected);
+  // |connected_| can be false while |[peripheral_ state]| is
+  // |CBPeripheralStateConnected|. This happens
+  // BluetoothAdapterMac::DidConnectPeripheral() is called and
+  // BluetoothLowEnergyDeviceMac::DidConnectGatt() has not been called yet.
+  return connected_;
 }
 
 bool BluetoothLowEnergyDeviceMac::IsConnectable() const {
@@ -186,6 +191,18 @@ void BluetoothLowEnergyDeviceMac::DisconnectGatt() {
   GetMacAdapter()->DisconnectGatt(this);
 }
 
+void BluetoothLowEnergyDeviceMac::DidConnectGatt() {

[ortuno, 2017/05/02 01:24:23]

Why can't we do this in DidConnectPeripheral before calling DidConnectGatt?

[jlebel, 2017/05/02 19:54:46]

That's a behavior of the device, no?

[ortuno, 2017/05/02 23:36:45]

Hmm I'm not sure I understand what you mean. DidConnectGatt is supposed to be a
non-virtual cross platform function that implementations use once a device
finishes connection. I think it makes sense to set connected_ before
DidConnectGatt and dispatch service discovery there as well.

[jlebel, 2017/05/03 21:16:19]

I'm not sure how to the same in BluetoothAdapterMac::DidConnectPeripheral()
First I need to update |connected_|, this can be done only in
BluetoothLowEnergyDeviceMac.
Plus, DiscoverPrimaryServices() seems to be more logical to be called in
BluetoothLowEnergyDeviceMac.

[ortuno, 2017/05/04 00:31:03]

Ah. I missed the fact that DidConnectPeripheral is in the Adapter.

What do you think about having BluetoothAdapterMac just forward the event to the
device. Like we do for characteristics in mac[1] or for connection events in
Android[2]:

// bluetooth_adapter_mac.mm
void BluetoothAdapterMac::DidConnectPeripheral(CBPeripheral* peripheral) {
  BluetoothLowEnergyDeviceMac* device_mac =
      GetBluetoothLowEnergyDeviceMac(peripheral);
  if (!device_mac) {
    [low_energy_central_manager_ cancelPeripheralConnection:peripheral];
    return;
  }
  device_mac->DidConnectPeripheral();
}

// bluetooth_low_energy_device_mac.mm
void BluetoothLowEnergyDeviceMac::DidConnectPeripheral() {
  const bool should_discover_services = !connected_;
  connected_ = true;
  if (should_discover_services) DiscoverPrimaryServices();
  DidConnectGatt();
}

This is very similar to what you propose with the added benefit of keeping
DidConnectGatt() truly cross platform.

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_low_energy_de...
[2]
https://cs.chromium.org/chromium/src/device/bluetooth/android/java/src/org/ch...

+  VLOG(1) << *this << ": Gatt connected.";
+  bool should_discover_services = !connected_;
+  connected_ = true;
+  BluetoothDeviceMac::DidConnectGatt();
+  if (should_discover_services) {
+    DiscoverPrimaryServices();
+  } else {
+    VLOG(1) << *this << ": Already connected, no need to discover services.";
+  }
+}
+
 void BluetoothLowEnergyDeviceMac::DidDiscoverPrimaryServices(NSError* error) {
   --discovery_pending_count_;
   if (discovery_pending_count_ < 0) {
@@ -450,6 +467,7 @@ BluetoothLowEnergyDeviceMac::GetBluetoothRemoteGattDescriptor(
 }
 
 void BluetoothLowEnergyDeviceMac::DidDisconnectPeripheral(NSError* error) {
+  connected_ = false;
   VLOG(1) << *this << ": Disconnected from peripheral.";
   if (error) {
     VLOG(1) << *this
@@ -484,7 +502,10 @@ std::ostream& operator<<(std::ostream& out,
   // BluetoothLowEnergyDeviceMac::GetNameForDisplay() instead.
   base::Optional<std::string> name = device.GetName();
   const char* name_cstr = name ? name->c_str() : "";

[ortuno, 2017/05/02 01:24:23]

optional. You might be able to avoid this:

<< name->value_or("Unnamed device") << ...

[jlebel, 2017/05/02 19:54:46]

Done.

+  const char* is_gatt_connected =
+      device.IsGattConnected() ? "Gatt connected" : "Gatt disconnected";
   return out << "<BluetoothLowEnergyDeviceMac " << device.GetAddress() << "/"
-             << &device << ", \"" << name_cstr << "\">";
+             << &device << ", " << is_gatt_connected << "\"" << name_cstr

[ortuno, 2017/05/02 01:24:23]

Would the following work?

<< &device << ", GATT Connected: " << device.IsGattConnected() << "\"" ...

[jlebel, 2017/05/02 19:54:46]

The result is:
GATT Connected: 0
GATT Connected: 1

[ortuno, 2017/05/02 23:36:45]

I think that's good enough. But up to you.

[jlebel, 2017/05/03 21:16:19]

I definitely prefer with english words to be more explicit.

+             << "\">";
 }
 }  // namespace device