[Bluetooth] Improve the adapter's awareness of connected devices.

device/bluetooth/bluetooth_adapter_mac.mm

Index: device/bluetooth/bluetooth_adapter_mac.mm
diff --git a/device/bluetooth/bluetooth_adapter_mac.mm b/device/bluetooth/bluetooth_adapter_mac.mm
index c4f9e39ea2a4863ce82b37a0844ed3314d407cb5..19bd893319bd4bebffd77197c043f000a6f5b4dd 100644
--- a/device/bluetooth/bluetooth_adapter_mac.mm
+++ b/device/bluetooth/bluetooth_adapter_mac.mm
@@ -138,6 +138,10 @@ void BluetoothAdapterMac::CreateL2capService(
 
 void BluetoothAdapterMac::DeviceFound(BluetoothDiscoveryManagerMac* manager,
                                       IOBluetoothDevice* device) {
+  // TODO(isherman): The list of discovered devices is never reset. This should
+  // probably key off of |devices_| instead. Currently, if a device is paired,
+  // then unpaired, then paired again, the app would never hear about the second
+  // pairing.

[armansito, 2014/06/17 14:56:43]

I agree with this, since to be consistent with the other platforms, devices that
are signalled through DeviceAdded should be available later via
BluetoothAdapter::GetDevices. Though since we don't seem to have a reliable
"removed" event on Mac, we can only detect this through the polling below and it
looks like it will now effectively end up calling DeviceRemoved on devices that
aren't connected or paired.

So if discovery is running and DeviceFound and PollAdapter run one after
another, we'll call DeviceAdded and then immediately DeviceRemoved on a device
which is technically still available to the system?

[Ilya Sherman, 2014/06/17 21:43:38]

Yeah, all of this logic is kinda wrong.  It's not actually clear to me when
exactly DeviceRemoved should be called.  Suppose a device is paired, but not
connected, and we haven't recently pinged it to check whether it's currently
within range.  The device might be off.  When should DeviceRemoved be called for
this device?  (What does ChromeOS do?)

Maybe we should report devices as removed if they are not connected and their
-getLastInquiryUpdate: and -recentAccessDate: are both a while ago?  I'd need to
investigate whether that does something reasonable.  I'd prefer to punt this
work to a separate CL.  For this CL, I'm primarily interested in making sure
that connected devices are listed among the |devices_|, so that we don't try to
dereference a NULL pointer.  Would you prefer that I hold off on adding
DeviceRemoved notifications until they are more sane?

[keybuk, 2014/06/17 21:48:50]

CrOS for a device that was found by discovery, and has not been paired, keeps
the device object around for 30s-3m (depending on device type) before removing
it

The idea being that if we don't get an inquiry or advertising packet in that
time, the device isn't in range anymore

[armansito, 2014/06/17 22:07:22]

Yes, let's not include the DeviceRemoved notifications yet, at least until we
have a better way to report them. As Scott said, on Chrome OS bluetoothd
automatically removes devices if it hasn't heard from them for a period of time.
So perhaps we try to simulate that here (if we can)? Ideally we wouldn't
condition this at all on whether or not the device is paired and/or connected.

Please investigate if the API calls that you mentioned give us a reliable result
overall. It's fine if you choose to do this in another CL, just make sure to add
a TODO here detailing the behavior that we're aiming for.

[Ilya Sherman, 2014/06/17 22:15:55]

Done.

   std::string device_address = BluetoothDeviceMac::GetDeviceAddress(device);
   if (discovered_devices_.find(device_address) == discovered_devices_.end()) {
     BluetoothDeviceMac device_mac(device);
@@ -161,6 +165,22 @@ void BluetoothAdapterMac::DiscoveryStopped(
                     AdapterDiscoveringChanged(this, false));
 }
 
+void BluetoothAdapterMac::DeviceConnected(IOBluetoothDevice* device) {
+  std::string device_address = BluetoothDeviceMac::GetDeviceAddress(device);
+  DVLOG(1) << "Adapter registered a new connection from device with address: "
+           << device_address;
+
+  // Only notify once per device.
+  if (devices_.count(device_address))
+    return;
+
+  scoped_ptr<BluetoothDeviceMac> device_mac(new BluetoothDeviceMac(device));
+  FOR_EACH_OBSERVER(BluetoothAdapter::Observer,
+                    observers_,
+                    DeviceAdded(this, device_mac.get()));
+  devices_[device_address] = device_mac.release();
+}
+
 void BluetoothAdapterMac::AddDiscoverySession(
     const base::Closure& callback,
     const ErrorCallback& error_callback) {
@@ -262,6 +282,7 @@ void BluetoothAdapterMac::PollAdapter() {
                       AdapterPoweredChanged(this, powered_));
   }
 
+  // TODO(isherman): This doesn't detect when a device is unpaired.
   IOBluetoothDevice* recent_device =
       [[IOBluetoothDevice recentDevices:1] lastObject];
   NSDate* access_timestamp = [recent_device recentAccessDate];
@@ -269,7 +290,7 @@ void BluetoothAdapterMac::PollAdapter() {
       access_timestamp == nil ||
       [recently_accessed_device_timestamp_ compare:access_timestamp] ==
           NSOrderedAscending) {
-    UpdateDevices([IOBluetoothDevice pairedDevices]);
+    UpdateDevices();
     recently_accessed_device_timestamp_.reset([access_timestamp copy]);
   }
 
@@ -280,15 +301,42 @@ void BluetoothAdapterMac::PollAdapter() {
       base::TimeDelta::FromMilliseconds(kPollIntervalMs));
 }
 
-void BluetoothAdapterMac::UpdateDevices(NSArray* devices) {
-  // TODO(armansito): This code never calls
-  // BluetoothAdapter::Observer::DeviceRemoved. It should, if a device
-  // no longer exists.
-  STLDeleteValues(&devices_);
-  for (IOBluetoothDevice* device in devices) {
+void BluetoothAdapterMac::UpdateDevices() {
+  // Snapshot the devices observers were previously notified of.
+  // Note that the code below is careful to take ownership of any values that
+  // are erased from the map, since the map owns the memory for all its mapped
+  // devices.
+  DevicesMap old_devices = devices_;
+
+  // Add all the paired devices.
+  devices_.clear();
+  for (IOBluetoothDevice* device in [IOBluetoothDevice pairedDevices]) {
     std::string device_address = BluetoothDeviceMac::GetDeviceAddress(device);
-    devices_[device_address] = new BluetoothDeviceMac(device);
+    scoped_ptr<BluetoothDevice> device_mac(old_devices[device_address]);
+    if (!device_mac)
+      device_mac.reset(new BluetoothDeviceMac(device));
+    devices_[device_address] = device_mac.release();
+    old_devices.erase(device_address);
+  }
+
+  // Add any unpaired connected devices.
+  for (const auto& old_device : old_devices) {

[armansito, 2014/06/17 14:56:43]

C++11 is allowed now?

[Ilya Sherman, 2014/06/17 21:43:38]

It is in Mac-only files :D

+    if (!old_device.second->IsConnected())
+      continue;
+
+    const std::string& device_address = old_device.first;
+    DCHECK(!devices_.count(device_address));
+    devices_[device_address] = old_device.second;
+    old_devices.erase(device_address);
+  }
+
+  // Notify observers of any devices that are no longer paired nor connected.
+  for (const auto& removed_device : old_devices) {
+    FOR_EACH_OBSERVER(BluetoothAdapter::Observer,
+                      observers_,
+                      DeviceRemoved(this, removed_device.second));

[armansito, 2014/06/17 14:56:43]

Good catch on realizing that this was never called. On Chrome OS, devices are
available via BluetoothAdapter::GetDevices even if they aren't connected or
paired, as long as they have been discovered and are discoverable and within
range, which is ideally what we want here as well (see my comment above in
::DeviceFound).

I wonder which one of [IOBluetoothDevice pairedDevices], [IOBluetoothDevice
recentDevices:], and [IOBluetoothDeviceInquiry foundDevices] correctly returns
what we want in the end. If there is no way to obtain a list of all devices
currently within range, then maybe that's why DeviceRemoved was actually never
called before?

   }
+  STLDeleteValues(&old_devices);
 }
 
 }  // namespace device