Adding support for Low Energy device discovery to BluetoothAdapterMac

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 ae557d0314b3395ce6d8ff4790fa15a19be63056..b1a026a46512ce5144087a77132381bf53b4263e 100644
--- a/device/bluetooth/bluetooth_adapter_mac.mm
+++ b/device/bluetooth/bluetooth_adapter_mac.mm
@@ -22,6 +22,7 @@
 #include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "device/bluetooth/bluetooth_device_mac.h"
+#include "device/bluetooth/bluetooth_discovery_session.h"
 #include "device/bluetooth/bluetooth_socket_mac.h"
 #include "device/bluetooth/bluetooth_uuid.h"
 
@@ -57,6 +58,8 @@ BluetoothAdapterMac::BluetoothAdapterMac()
       num_discovery_sessions_(0),
       classic_discovery_manager_(
           BluetoothDiscoveryManagerMac::CreateClassic(this)),
+      low_energy_discovery_manager_(
+          BluetoothLowEnergyDiscoveryManagerMac::Create(this)),
       weak_ptr_factory_(this) {
   DCHECK(classic_discovery_manager_.get());
 }
@@ -109,7 +112,8 @@ void BluetoothAdapterMac::SetDiscoverable(
 }
 
 bool BluetoothAdapterMac::IsDiscovering() const {
-  return classic_discovery_manager_->IsDiscovering();
+  return (classic_discovery_manager_->IsDiscovering() ||
+          low_energy_discovery_manager_->IsDiscovering());
 }
 
 void BluetoothAdapterMac::CreateRfcommService(
@@ -148,11 +152,11 @@ void BluetoothAdapterMac::RegisterAdvertisement(
   error_callback.Run(BluetoothAdvertisement::ERROR_UNSUPPORTED_PLATFORM);
 }
 
-void BluetoothAdapterMac::DeviceFound(IOBluetoothDevice* device) {
-  DeviceAdded(device);
+void BluetoothAdapterMac::ClassicDeviceFound(IOBluetoothDevice* device) {
+  ClassicDeviceAdded(device);
 }
 
-void BluetoothAdapterMac::DiscoveryStopped(bool unexpected) {
+void BluetoothAdapterMac::ClassicDiscoveryStopped(bool unexpected) {
   if (unexpected) {
     DVLOG(1) << "Discovery stopped unexpectedly";
     num_discovery_sessions_ = 0;
@@ -168,7 +172,32 @@ void BluetoothAdapterMac::DeviceConnected(IOBluetoothDevice* device) {
   // to +registerForConnectNotifications:selector:.
   DVLOG(1) << "Adapter registered a new connection from device with address: "
            << BluetoothDeviceMac::GetDeviceAddress(device);
-  DeviceAdded(device);
+  ClassicDeviceAdded(device);
+}
+
+bool BluetoothAdapterMac::StartDiscovery(

[scheib, 2015/06/05 18:10:20]

Move to after SetDiscoveryFilter. "Method definitions in the corresponding .cc
file should be the same as the declaration order, as much as possible."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

[krstnmnlsn, 2015/06/09 01:08:10]

Right.  I went ahead and moved RemovePairingDelegateInteral as well.

+    BluetoothDiscoveryFilter* discovery_filter) {
+  // Default to dual discovery if |discovery_filter| is NULL.
+  BluetoothDiscoveryFilter::TransportMask transport =
+      BluetoothDiscoveryFilter::Transport::TRANSPORT_DUAL;
+  if (discovery_filter)
+    transport = discovery_filter->GetTransport();
+
+  if ((transport & BluetoothDiscoveryFilter::Transport::TRANSPORT_CLASSIC) &&

[scheib, 2015/06/05 18:10:20]

We should find a mix of classic and LE devices and test that starting discovery
on both APIs at the same time will work. I can help with that a bit. 

Or, if there's more info already known about this we should document it here in
comments.

[krstnmnlsn, 2015/06/09 01:08:10]

Manually tested and added a short comment (cannot find any information online).

+      !classic_discovery_manager_->IsDiscovering()) {
+    // TODO(krstnmnlsn): If a classic discovery session is already running then
+    // we should update its filter.
+    if (!classic_discovery_manager_->StartDiscovery()) {
+      DVLOG(1) << "Failed to add a classic discovery session";
+      return false;
+    }
+  }
+  if (transport & BluetoothDiscoveryFilter::Transport::TRANSPORT_LE) {
+    // Begin a low energy discovery session or update it if one is already
+    // running.
+    low_energy_discovery_manager_->StartDiscovery(BluetoothDevice::UUIDList());
+  }
+  return true;
 }
 
 void BluetoothAdapterMac::AddDiscoverySession(
@@ -179,14 +208,19 @@ void BluetoothAdapterMac::AddDiscoverySession(
   if (num_discovery_sessions_ > 0) {
     DCHECK(IsDiscovering());
     num_discovery_sessions_++;
+    // We are already running a discovery session, notify the system if the
+    // filter has changed.
+    if (!StartDiscovery(discovery_filter)) {

[scheib, 2015/06/06 05:09:48]

Without seeing the plans for how differing filters will be handled I'm uncertain
about this. Previously if multiple AddDiscoverSessions happen the system was
only asked to scan once. I don't know if the OSX APIs care, but I also don't see
a need to call them more than once unless we have a reason to.

Adding Classic and LE simultaneously gives us a reason to do something if first
a discovery session requested only one, but another AddDiscoverySession
requested another one. Perhaps num_discovery_sessions_ should be duplicated for
both Classic and LE counts?

If filters later try to support scanning for differing sets of service UUIDs in
LE, we'll need e.g. merging logic for the filer, or postponing or failing a new
discovery session if one is already under way.

After clearing up the design intent (put it in the design doc, and add comments
here explaining anything (especially the 'why' that isn't obvious from code))
Then add tests for the different scenarios of multiple AddDiscoverySessions.

[krstnmnlsn, 2015/06/09 01:08:09]

Recap/continuing of offline discussion: 
The plans for handling of differing filters have mostly already been made in the
BluetoothAdapter class (see: GetMergedDiscoveryFilter).  We can handle merging
(and editing) the set of filters, and notifying the system of a filter change
will be consistent with Chromeos and what (I was interpreting to be) the desired
cross-platform behavior.

The LE API does not mind repeated calls to start bluetooth/peripheral discovery,
the classic API might.  That is why the classic part of StartDiscovery currently
does nothing on a repeated call (this was the previous behavior as well). 
Issue: crbug.com/498056

+      error_callback.Run();
+      return;
+    }
     callback.Run();
     return;
   }
 
   DCHECK_EQ(0, num_discovery_sessions_);
 
-  if (!classic_discovery_manager_->StartDiscovery()) {
-    DVLOG(1) << "Failed to add a discovery session";
+  if (!StartDiscovery(discovery_filter)) {
     error_callback.Run();
     return;
   }
@@ -219,10 +253,21 @@ void BluetoothAdapterMac::RemoveDiscoverySession(
     return;
   }
 
-  if (!classic_discovery_manager_->StopDiscovery()) {
-    DVLOG(1) << "Failed to stop discovery";
-    error_callback.Run();
-    return;
+  // Default to dual discovery if |discovery_filter| is NULL.
+  BluetoothDiscoveryFilter::TransportMask transport =
+      BluetoothDiscoveryFilter::Transport::TRANSPORT_DUAL;
+  if (discovery_filter)
+    transport = discovery_filter->GetTransport();
+
+  if (transport & BluetoothDiscoveryFilter::Transport::TRANSPORT_CLASSIC) {
+    if (!classic_discovery_manager_->StopDiscovery()) {
+      DVLOG(1) << "Failed to stop classic discovery";
+      error_callback.Run();
+      return;
+    }
+  }
+  if (transport & BluetoothDiscoveryFilter::Transport::TRANSPORT_LE) {
+    low_energy_discovery_manager_->StopDiscovery();
   }
 
   DVLOG(1) << "Discovery stopped";
@@ -323,7 +368,7 @@ void BluetoothAdapterMac::PollAdapter() {
       base::TimeDelta::FromMilliseconds(kPollIntervalMs));
 }
 
-void BluetoothAdapterMac::DeviceAdded(IOBluetoothDevice* device) {
+void BluetoothAdapterMac::ClassicDeviceAdded(IOBluetoothDevice* device) {
   std::string device_address = BluetoothDeviceMac::GetDeviceAddress(device);
 
   // Only notify observers once per device.
@@ -336,6 +381,17 @@ void BluetoothAdapterMac::DeviceAdded(IOBluetoothDevice* device) {
                     DeviceAdded(this, devices_[device_address]));
 }
 
+// TODO(krstnmnlsn): This method to be implemented as soon as UpdateDevices can
+// handle instances of LowEnergyBluetoothDevice in |devices_|.

[scheib, 2015/06/05 18:10:20]

Add a reference to the issue number to do that work. Just a crbug.com/000000
after the sentence. 

[krstnmnlsn, 2015/06/09 01:08:09]

Done.

+void BluetoothAdapterMac::LowEnergyDeviceUpdated(
+    CBPeripheral* peripheral,
+    NSDictionary* advertisementData,
+    int rssi) {
+  NOTIMPLEMENTED();

[scheib, 2015/06/05 18:10:20]

We expect LE sessions will be started, so we can't throw an error here because
we haven't implemented this yet. Just remove NOTIMPLEMENTED for now.

[krstnmnlsn, 2015/06/09 01:08:09]

Done.

+}
+
+// TODO(krstnmnlsn): This method assumes all BluetoothDevices in devices_ are
+// instances of BluetoothDeviceMac.  Add support for low energy devices.

[scheib, 2015/06/05 18:10:20]

issue number.

[krstnmnlsn, 2015/06/09 01:08:09]

Done.

 void BluetoothAdapterMac::UpdateDevices() {
   // Notify observers if any previously seen devices are no longer available,
   // i.e. if they are no longer paired, connected, nor recently discovered via
@@ -365,7 +421,7 @@ void BluetoothAdapterMac::UpdateDevices() {
 
   // Add any new paired devices.
   for (IOBluetoothDevice* device in [IOBluetoothDevice pairedDevices]) {
-    DeviceAdded(device);
+    ClassicDeviceAdded(device);
   }
 }