Implementing GATT connection/disconnect on OS X.

device/bluetooth/bluetooth_adapter_mac.mm

 // 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 "device/bluetooth/bluetooth_adapter_mac.h"
 
 #import <IOBluetooth/objc/IOBluetoothDevice.h>
 #import <IOBluetooth/objc/IOBluetoothHostController.h>
 #include <stddef.h>
 
@@ skipping 75 matching lines @@
     low_energy_central_manager_.reset([[aClass alloc]
         initWithDelegate:low_energy_central_manager_delegate_.get()
                    queue:dispatch_get_main_queue()]);
     low_energy_discovery_manager_->SetCentralManager(
         low_energy_central_manager_.get());
   }
   DCHECK(classic_discovery_manager_.get());
 }
 
 BluetoothAdapterMac::~BluetoothAdapterMac() {
+  // Need to make sure the devices are destroyed before CBCentralManage is

[fbeaufortchromium, 2016/02/11 10:26:02]

s/CBCentralManage/CBCentralManager

[msarda, 2016/02/11 10:55:38]

I would kill this comment and just do what the comment says:
devices_clear();
low_energy_central_manager_.release();

[jlebel, 2016/02/19 11:02:35]

This is really important to do it explicitly. We could think this line is
useless since it will be done anyway once the destroy method is done. But there
is an issue for the disconnect.
Doing the 2 lines that you suggested looks like it is useless too.  But it is
important.
I don't mind adding low_energy_central_manager_.release();

+  // released.
+  devices_.clear();
 }
 
 std::string BluetoothAdapterMac::GetAddress() const {
   return address_;
 }
 
 std::string BluetoothAdapterMac::GetName() const {
   return name_;
 }
 
@@ skipping 109 matching lines @@
   DVLOG(1) << "Adapter registered a new connection from device with address: "
            << BluetoothClassicDeviceMac::GetDeviceAddress(device);
   ClassicDeviceAdded(device);
 }
 
 // static
 bool BluetoothAdapterMac::IsLowEnergyAvailable() {
   return base::mac::IsOSYosemiteOrLater();
 }
 
+void BluetoothAdapterMac::RemovePairingDelegateInternal(
+    BluetoothDevice::PairingDelegate* pairing_delegate) {}
+
 void BluetoothAdapterMac::SetCentralManagerForTesting(
     CBCentralManager* central_manager) {
   CHECK(BluetoothAdapterMac::IsLowEnergyAvailable());
-  [central_manager performSelector:@selector(setDelegate:)
-                        withObject:low_energy_central_manager_delegate_];
-  low_energy_central_manager_.reset(central_manager);
+  [central_manager setDelegate:low_energy_central_manager_delegate_];
+  low_energy_central_manager_.reset(central_manager,
+                                    base::scoped_policy::RETAIN);
   low_energy_discovery_manager_->SetCentralManager(
       low_energy_central_manager_.get());
 }
 
-void BluetoothAdapterMac::RemovePairingDelegateInternal(
-    BluetoothDevice::PairingDelegate* pairing_delegate) {
+CBCentralManager* BluetoothAdapterMac::GetCentralManagerForTesting() {
+  return low_energy_central_manager_;
 }
 
 void BluetoothAdapterMac::AddDiscoverySession(
     BluetoothDiscoveryFilter* discovery_filter,
     const base::Closure& callback,
     const DiscoverySessionErrorCallback& error_callback) {
   DVLOG(1) << __func__;
   if (num_discovery_sessions_ > 0) {
     DCHECK(IsDiscovering());
     num_discovery_sessions_++;
@@ skipping 202 matching lines @@
   BluetoothDevice* device_classic = new BluetoothClassicDeviceMac(this, device);
   devices_.set(device_address, make_scoped_ptr(device_classic));
   FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
                     DeviceAdded(this, device_classic));
 }
 
 void BluetoothAdapterMac::LowEnergyDeviceUpdated(
     CBPeripheral* peripheral,
     NSDictionary* advertisement_data,
     int rssi) {
-  std::string device_address =
-      BluetoothLowEnergyDeviceMac::GetPeripheralHashAddress(peripheral);
-  // Try to find device from |devices_| with key |device_address|,
+  BluetoothLowEnergyDeviceMac* device_mac =
+      GetBluetoothLowEnergyDeviceMac(peripheral);

[fbeaufortchromium, 2016/02/11 10:26:02]

Was the comment line below removed by accident?

// Try to find device from |devices_| with key |device_address|,

Or should we simply remove the comment?

[jlebel, 2016/02/19 11:02:35]

Done.

[msarda, 2016/02/22 14:01:06]

This was not Done. Please put back the line.

   // if has no entry in the map, create new device and insert into |devices_|,
   // otherwise update the existing device.
-  DevicesMap::const_iterator iter = devices_.find(device_address);
-  if (iter == devices_.end()) {
+  if (!device_mac) {
     VLOG(1) << "LowEnergyDeviceUpdated new device";
     // A new device has been found.
-    BluetoothLowEnergyDeviceMac* device_mac = new BluetoothLowEnergyDeviceMac(
-        this, peripheral, advertisement_data, rssi);
+    device_mac = new BluetoothLowEnergyDeviceMac(this, peripheral,
+                                                 advertisement_data, rssi);
+    std::string device_address =
+        BluetoothLowEnergyDeviceMac::GetPeripheralHashAddress(peripheral);
     devices_.add(device_address, scoped_ptr<BluetoothDevice>(device_mac));
     FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
                       DeviceAdded(this, device_mac));
     return;
   }
 
-  BluetoothLowEnergyDeviceMac* device_mac =
-      static_cast<BluetoothLowEnergyDeviceMac*>(iter->second);
   std::string stored_device_id = device_mac->GetIdentifier();
   std::string updated_device_id =
       BluetoothLowEnergyDeviceMac::GetPeripheralIdentifier(peripheral);
   if (stored_device_id != updated_device_id) {
     VLOG(1) << "LowEnergyDeviceUpdated stored_device_id != updated_device_id: "
             << std::endl
             << "  " << stored_device_id << std::endl
             << "  " << updated_device_id;
     // Collision, two identifiers map to the same hash address.  With a 48 bit
     // hash the probability of this occuring with 10,000 devices
@@ skipping 42 matching lines @@
   }
 }
 
 void BluetoothAdapterMac::AddPairedDevices() {
   // Add any new paired devices.
   for (IOBluetoothDevice* device in [IOBluetoothDevice pairedDevices]) {
     ClassicDeviceAdded(device);
   }
 }
 
+void BluetoothAdapterMac::CreateGattConnection(
+    BluetoothLowEnergyDeviceMac* device_mac) {
+  [low_energy_central_manager_ connectPeripheral:device_mac->peripheral_
+                                         options:nil];
+}
+
+void BluetoothAdapterMac::DisconnectGatt(
+    BluetoothLowEnergyDeviceMac* device_mac) {
+  [low_energy_central_manager_
+      cancelPeripheralConnection:device_mac->peripheral_];
+}
+
+void BluetoothAdapterMac::DidConnectPeripheral(CBPeripheral* peripheral) {
+  BluetoothLowEnergyDeviceMac* device_mac =
+      GetBluetoothLowEnergyDeviceMac(peripheral);
+  // Try to find device from |devices_| with key |device_address|,

[msarda, 2016/02/11 10:55:38]

s/find device/find the device
s/,/.
s/if has no entry in the map, disconnect the peripheral./If there is no entry in
the map then disconnect the peripheral.

[jlebel, 2016/02/19 11:02:35]

Done.

+  // if has no entry in the map, disconnect the peripheral.
+  if (!device_mac) {
+    [low_energy_central_manager_ cancelPeripheralConnection:peripheral];
+    return;
+  }
+  device_mac->GattConnected();
+}
+
+void BluetoothAdapterMac::DidFailToConnectPeripheral(CBPeripheral* peripheral,
+                                                     NSError* error) {
+  BluetoothLowEnergyDeviceMac* device_mac =
+      GetBluetoothLowEnergyDeviceMac(peripheral);
+  if (!device_mac) {

[msarda, 2016/02/11 10:55:38]

This code is duplicated in 3 functions. Consider refactoring it to a helper
method.

[jlebel, 2016/02/19 11:02:35]

I'm not sure we will gain a lot of lines by adding an helper. I will have
problems to find the right name, and the |if| will still be needed.

+    [low_energy_central_manager_ cancelPeripheralConnection:peripheral];
+    return;
+  }
+  // TODO(http://crbug.com/585894): Need to convert the error.

[fbeaufortchromium, 2016/02/11 10:26:02]

Can we VLOG the error there?

[jlebel, 2016/02/19 11:02:35]

Done.

+  device_mac->DidFailToConnectGatt(BluetoothClassicDeviceMac::ERROR_UNKNOWN);
+}
+
+void BluetoothAdapterMac::DidDisconnectPeripheral(CBPeripheral* peripheral,
+                                                  NSError* error) {
+  BluetoothLowEnergyDeviceMac* device_mac =
+      GetBluetoothLowEnergyDeviceMac(peripheral);
+  if (!device_mac) {
+    [low_energy_central_manager_ cancelPeripheralConnection:peripheral];
+    return;
+  }
+  // TODO(http://crbug.com/585897): Need to pass the error.
+  device_mac->DidDisconnectPeripheral();
+}
+
+BluetoothLowEnergyDeviceMac*
+BluetoothAdapterMac::GetBluetoothLowEnergyDeviceMac(CBPeripheral* peripheral) {
+  std::string device_address =
+      BluetoothLowEnergyDeviceMac::GetPeripheralHashAddress(peripheral);
+  // Try to find device from |devices_| with key |device_address|,

[msarda, 2016/02/11 10:55:38]

I do not understand the comment. Where do you disconnect the peripheral?

[jlebel, 2016/02/19 11:02:35]

Done.

+  // if has no entry in the map, disconnect the peripheral.
+  DevicesMap::const_iterator iter = devices_.find(device_address);
+  if (iter == devices_.end()) {
+    return nil;
+  }
+  return static_cast<BluetoothLowEnergyDeviceMac*>(iter->second);
+}
+
 }  // namespace device