device/bluetooth: Clean up classic discovery in BluetoothAdapterMac.

device/bluetooth/bluetooth_mac_discovery_manager.mm

Index: device/bluetooth/bluetooth_mac_discovery_manager.mm
diff --git a/device/bluetooth/bluetooth_mac_discovery_manager.mm b/device/bluetooth/bluetooth_mac_discovery_manager.mm
new file mode 100644
index 0000000000000000000000000000000000000000..836fd62f26bcaaa3a015d988d14e11e4d36d6f55
--- /dev/null
+++ b/device/bluetooth/bluetooth_mac_discovery_manager.mm
@@ -0,0 +1,241 @@
+// Copyright 2014 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_mac_discovery_manager.h"
+
+#import <IOBluetooth/objc/IOBluetoothDevice.h>
+#import <IOBluetooth/objc/IOBluetoothDeviceInquiry.h>
+
+#include "base/mac/scoped_nsobject.h"
+
+namespace device {
+
+class BluetoothMacDiscoveryManagerClassicImpl;
+
+}  // namespace device
+
+// Replicate specific 10.7 SDK declarations for building with prior SDKs.
+#if !defined(MAC_OS_X_VERSION_10_7) || \
+    MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
+
+@protocol IOBluetoothDeviceInquiryDelegate
+- (void)deviceInquiryStarted:(IOBluetoothDeviceInquiry*)sender;
+- (void)deviceInquiryDeviceFound:(IOBluetoothDeviceInquiry*)sender
+                          device:(IOBluetoothDevice*)device;
+- (void)deviceInquiryComplete:(IOBluetoothDeviceInquiry*)sender
+                        error:(IOReturn)error
+                      aborted:(BOOL)aborted;
+@end
+
+#endif  // MAC_OS_X_VERSION_10_7

[Ilya Sherman, 2014/06/10 01:25:13]

I discovered while rebasing a CL today that apparently this has moved to
base/mac/sdk_forward_declarations.h

[armansito, 2014/06/10 21:56:50]

Done.

+
+// IOBluetoothDeviceInquiryDelegate implementation.
+@interface BluetoothDeviceInquiryDelegate
+    : NSObject<IOBluetoothDeviceInquiryDelegate> {
+ @private
+  device::BluetoothMacDiscoveryManagerClassicImpl* manager_;  // weak
+}
+
+- (id)initWithManager:(device::BluetoothMacDiscoveryManagerClassicImpl*)manager;
+
+@end
+
+namespace device {
+
+// Implementation of BluetoothMacDiscoveryManager for Bluetooth classic device
+// discovery, using the IOBluetooth framework.
+class BluetoothMacDiscoveryManagerClassicImpl

[Ilya Sherman, 2014/06/10 01:10:59]

nit: "Impl" seems unnecessary, since there's no non-impl version of the class
(other than the superclass which I assume is intended to be the superclass for
both Classic and LE).

[armansito, 2014/06/10 21:56:50]

You assumed correctly. Done.

+    : public BluetoothMacDiscoveryManager {
+ public:
+  BluetoothMacDiscoveryManagerClassicImpl()
+      : should_do_discovery_(false),
+        inquiry_running_(false),
+        inquiry_delegate_(
+            [[BluetoothDeviceInquiryDelegate alloc] initWithManager:this]),
+        inquiry_([[IOBluetoothDeviceInquiry alloc]
+            initWithDelegate:inquiry_delegate_]) {}
+
+  virtual ~BluetoothMacDiscoveryManagerClassicImpl() {}
+
+  // BluetoothMacDiscoveryManager override.
+  virtual bool IsDiscovering() const OVERRIDE { return should_do_discovery_; }
+
+  // BluetoothMacDiscoveryManager override.
+  virtual bool StartDiscovery() OVERRIDE {
+    VLOG(1) << "Bluetooth Classic: StartDiscovery";
+    if (should_do_discovery_) {
+      VLOG(1) << "Already discovering";
+      return true;
+    }

[Ilya Sherman, 2014/06/10 01:11:00]

Could this be a DCHECK instead?

[armansito, 2014/06/10 21:56:50]

Done.

+
+    VLOG(1) << "Discovery requested";
+    should_do_discovery_ = true;
+
+    if (inquiry_running_) {
+      VLOG(1) << "Device inquiry already running";
+      return true;
+    }

[Ilya Sherman, 2014/06/10 01:10:59]

Why is this reachable?  Could this be a DCHECK instead?

[armansito, 2014/06/10 21:56:51]

No, please see my comment below.

+
+    VLOG(1) << "Requesting to start device inquiry";
+    if ([inquiry_ start] != kIOReturnSuccess) {
+      VLOG(1) << "Failed to start device inquiry";
+
+      // Set |should_do_discovery_| to false here. Since we're reporting an
+      // error, we're indicating that the adapter call StartDiscovery again
+      // if needed.
+      should_do_discovery_ = false;
+      return false;
+    }
+
+    VLOG(1) << "Device inquiry start was successful";
+    return true;
+  }
+
+  // BluetoothMacDiscoveryManager override.
+  virtual bool StopDiscovery() OVERRIDE {
+    VLOG(1) << "Bluetooth Classic: StopDiscovery";
+    if (!should_do_discovery_) {
+      VLOG(1) << "Discovery already stopped";
+      return true;
+    }

[Ilya Sherman, 2014/06/10 01:10:59]

Ditto.

[armansito, 2014/06/10 21:56:50]

Done.

+
+    should_do_discovery_ = false;
+
+    if (!inquiry_running_) {
+      VLOG(1) << "No device inquiry running; discovery stopped";
+      return true;
+    }

[Ilya Sherman, 2014/06/10 01:10:59]

This seems wrong, since inquiry_running_ can be set asynchronously.  It seems
like calling StopDiscovery() immediately after StartDiscovery() would currently
not actually stop the discovery.  Or am I missing something?

[armansito, 2014/06/10 21:56:51]

[inquiry_ start] and [inquiry_ stop] are synchronous, so it shouldn't be
possible to call StopDiscovery before StartDiscovery has returned successfully,
since I assume all of these operations are running on the same thread (the UI
thread?).  With that assumption, if StopDiscovery gets called within 10 seconds
after the call to StartDiscovery, |inquiry_running_| should be true. If
StopDiscovery is called exactly after 10 seconds and we get a race where the
system notifies us that inquiry has stopped and sets |inquiry_running_| to
false, since this method here sets |should_do_discovery_| to false, the inquiry
won't be restarted.

If we ever hit a case where we end up restarting the inquiry, and this method
returns while |inquiry_running_| is still false, then yes, we will be in a
situation where inquiry will keep running for another 10 seconds. Even so, we
won't restart inquiry as long as |should_do_discovery_| is false when inquiry
ends. I think this behavior is OK. Us returning success from StopDiscovery
basically means that we have requested the underlying system to stop discovering
new devices and the request went through successfully, and it will eventually
stop.

This race condition is also the reason why we don't want to have a
DCHECK(!inquiry_running_) in StartDiscovery. If inquiry is still running during
a call to StartDiscovery, we just return true and set |should_do_discovery_| to
true, so that when inquiry ends, it gets automatically restarted.

P.S.: I just tested this by running startDiscovery and stopDiscovery in a loop
from JS, and things seem to work as expected.

+
+    VLOG(1) << "Requesting to stop device inquiry";
+    IOReturn status = [inquiry_ stop];
+    if (status == kIOReturnSuccess) {
+      VLOG(1) << "Device inquiry stop was successful";
+      return true;
+    }
+
+    if (status == kIOReturnNotPermitted) {
+      VLOG(1) << "Device inquiry was already stopped";
+      return true;
+    }
+
+    LOG(WARNING) << "Failed to stop device inquiry";
+    return false;
+  }
+
+  // Called by BluetoothDeviceInquiryDelegate.
+  void DeviceInquiryStarted(IOBluetoothDeviceInquiry* inquiry) {
+    DCHECK(!inquiry_running_);
+
+    VLOG(1) << "Device inquiry started!";
+
+    // If discovery was requested to stop in the mean time, stop the inquiry.
+    if (!should_do_discovery_) {
+      VLOG(1) << "Discovery stop was requested earlier. Stopping inquiry";
+      [inquiry stop];
+      return;
+    }
+
+    inquiry_running_ = true;
+  }
+
+  void DeviceFound(IOBluetoothDeviceInquiry* inquiry,
+                   IOBluetoothDevice* device) {
+    FOR_EACH_OBSERVER(Observer, observers_, DeviceFound(this, device));
+  }
+
+  void DeviceInquiryComplete(IOBluetoothDeviceInquiry* inquiry,
+                             IOReturn error,
+                             bool aborted) {
+    DCHECK_EQ(inquiry_, inquiry);
+    VLOG(1) << "Device inquiry complete";
+    inquiry_running_ = false;
+
+    // Automatically restart device inquiry if discovery is still desired.

[Ilya Sherman, 2014/06/10 01:10:59]

nit: This comment seems either misplaced, or like an "Otherwise, ..." addition
could help clarify.

[armansito, 2014/06/10 21:56:51]

Done.

+    if (!should_do_discovery_) {
+      FOR_EACH_OBSERVER(
+          Observer, observers_, DiscoveryStopped(this, false /* unexpected */));
+      return;
+    }
+
+    if (error != kIOReturnSuccess) {
+      VLOG(1) << "Inquiry has stopped with an error: " << error;
+      should_do_discovery_ = false;
+      FOR_EACH_OBSERVER(
+          Observer, observers_, DiscoveryStopped(this, true /* unexpected */));
+      return;
+    }
+
+    VLOG(1) << "Restarting device inquiry";
+
+    if ([inquiry_ start] == kIOReturnSuccess) {
+      VLOG(1) << "Device inquiry restart was successful";
+      return;
+    }
+
+    VLOG(1) << "Failed to restart discovery";
+    should_do_discovery_ = false;
+    FOR_EACH_OBSERVER(
+        Observer, observers_, DiscoveryStopped(this, true /* unexpected */));
+  }
+
+ private:
+  // The requested discovery state.
+  bool should_do_discovery_;
+
+  // The current inquiry state.
+  bool inquiry_running_;
+
+  // Objective-C objects for running and tracking device inquiry.
+  base::scoped_nsobject<BluetoothDeviceInquiryDelegate> inquiry_delegate_;
+  base::scoped_nsobject<IOBluetoothDeviceInquiry> inquiry_;
+
+  DISALLOW_COPY_AND_ASSIGN(BluetoothMacDiscoveryManagerClassicImpl);
+};
+
+BluetoothMacDiscoveryManager::BluetoothMacDiscoveryManager() {
+}
+BluetoothMacDiscoveryManager::~BluetoothMacDiscoveryManager() {
+}
+
+void BluetoothMacDiscoveryManager::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void BluetoothMacDiscoveryManager::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+// static
+BluetoothMacDiscoveryManager* BluetoothMacDiscoveryManager::CreateClassic() {
+  return new BluetoothMacDiscoveryManagerClassicImpl();
+}
+
+}  // namespace device
+
+@implementation BluetoothDeviceInquiryDelegate
+
+- (id)initWithManager:
+          (device::BluetoothMacDiscoveryManagerClassicImpl*)manager {
+  if ((self = [super init]))
+    manager_ = manager;
+
+  return self;
+}
+
+- (void)deviceInquiryStarted:(IOBluetoothDeviceInquiry*)sender {
+  manager_->DeviceInquiryStarted(sender);
+}
+
+- (void)deviceInquiryDeviceFound:(IOBluetoothDeviceInquiry*)sender
+                          device:(IOBluetoothDevice*)device {
+  manager_->DeviceFound(sender, device);
+}
+
+- (void)deviceInquiryComplete:(IOBluetoothDeviceInquiry*)sender
+                        error:(IOReturn)error
+                      aborted:(BOOL)aborted {
+  manager_->DeviceInquiryComplete(sender, error, aborted);
+}
+
+@end