bluetooth: mac: Improve classic device discovery and update

device/bluetooth/bluetooth_discovery_manager_mac.mm

 // 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_discovery_manager_mac.h"
 
 #import <IOBluetooth/objc/IOBluetoothDevice.h>
 #import <IOBluetooth/objc/IOBluetoothDeviceInquiry.h>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/mac/sdk_forward_declarations.h"
 #include "base/macros.h"
+#include "base/timer/timer.h"
 
 namespace device {
 
 class BluetoothDiscoveryManagerMacClassic;
 
 }  // namespace device
 
 // IOBluetoothDeviceInquiryDelegate implementation.
 @interface BluetoothDeviceInquiryDelegate
     : NSObject<IOBluetoothDeviceInquiryDelegate> {
  @private
   device::BluetoothDiscoveryManagerMacClassic* manager_;  // weak
 }
 
 - (id)initWithManager:(device::BluetoothDiscoveryManagerMacClassic*)manager;
 
 @end
 
 namespace device {
 
+// 1 second between cache clean ups is a good compromise between latency and
+// performance.
+const base::TimeDelta kCleanCacheDelay = base::TimeDelta::FromSeconds(1);

[Jeffrey Yasskin, 2016/09/14 20:35:57]

s/const/constexpr/ to make it clear to readers that this is a static rather than
a dynamic initialization.

[ortuno, 2016/09/20 04:31:33]

Done.

+
 // ementation of BluetoothDiscoveryManagerMac for Bluetooth classic device
 // discovery, using the IOBluetooth framework.
 class BluetoothDiscoveryManagerMacClassic
     : public BluetoothDiscoveryManagerMac {
  public:
   explicit BluetoothDiscoveryManagerMacClassic(Observer* observer)
       : BluetoothDiscoveryManagerMac(observer),
         should_do_discovery_(false),
         inquiry_running_(false),
         inquiry_delegate_(
             [[BluetoothDeviceInquiryDelegate alloc] initWithManager:this]),
         inquiry_([[IOBluetoothDeviceInquiry alloc]
-            initWithDelegate:inquiry_delegate_]) {}
+            initWithDelegate:inquiry_delegate_]),
+        clean_cache_timer_(

[Jeffrey Yasskin, 2016/09/14 20:35:57]

I'm nervous about periodic tasks that call into MacOS's bluetooth stack, after
https://crbug.com/482141. If you've already thought about jank and concluded
this won't cause any, could you comment how you know that? Otherwise, please
think about it. :)

[ortuno, 2016/09/20 04:31:33]

I chose 1 second in case cleaning the cache took too long. I'm also not that
worried about it because this only happens during an active scan. The other one
happened ALL the time. We could add some tracers to make sure it doesn't take
too long.

That said maybe I'm trying to force the classic API to be too similar to the LE
API, that is, maybe there is no need to be notified of each time we see a
classic device. We could just clean the cache before a client starts a discovery
that would allow new discovery sessions to find old devices and avoid the timer.
WDYT?

[Jeffrey Yasskin, 2016/09/23 22:25:21]

Having a tracer sounds like a good idea. Just around the whole
CleanFoundDevicesCache() body? Or ignore this if you take the timer out as you
say below.
Yeah, Classic inquiry is done by the inquirer sending a broadcast and listening
for responses, rather than LE's style of listening for spontaneous broadcasts.
It probably makes sense to only discover each Classic device once per inquiry.

[ortuno, 2016/09/26 08:45:28]

Removed timer.

+            FROM_HERE,
+            kCleanCacheDelay,
+            base::Bind(
+                &BluetoothDiscoveryManagerMacClassic::CleanFoundDevicesCache,
+                // base::Timer guarantees it won't call back after its
+                // destructor starts.
+                base::Unretained(this)),
+            true /* is_repeating */) {}
 
   ~BluetoothDiscoveryManagerMacClassic() override {}
 
   // BluetoothDiscoveryManagerMac override.
   bool IsDiscovering() const override { return should_do_discovery_; }
 
   // BluetoothDiscoveryManagerMac override.
   bool StartDiscovery() override {
     DVLOG(1) << "Bluetooth Classic: StartDiscovery";
     DCHECK(!should_do_discovery_);
@@ skipping 10 matching lines @@
     if ([inquiry_ start] != kIOReturnSuccess) {
       DVLOG(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;
     }
 
+    CleanFoundDevicesCache();
+    clean_cache_timer_.Reset();
     DVLOG(1) << "Device inquiry start was successful";
     return true;
   }
 
   // BluetoothDiscoveryManagerMac override.
   bool StopDiscovery() override {
     DVLOG(1) << "Bluetooth Classic: StopDiscovery";
     DCHECK(should_do_discovery_);
 
     should_do_discovery_ = false;
 
     if (!inquiry_running_) {
       DVLOG(1) << "No device inquiry running; discovery stopped";
       return true;
     }
 
     DVLOG(1) << "Requesting to stop device inquiry";
+    CleanFoundDevicesCache();
+    clean_cache_timer_.Stop();
+
     IOReturn status = [inquiry_ stop];
     if (status == kIOReturnSuccess) {
       DVLOG(1) << "Device inquiry stop was successful";
       return true;
     }
 
     if (status == kIOReturnNotPermitted) {
       DVLOG(1) << "Device inquiry was already stopped";
       return true;
     }
@@ skipping 55 matching lines @@
       return;
     }
 
     DVLOG(1) << "Failed to restart discovery";
     should_do_discovery_ = false;
     DCHECK(observer_);
     observer_->ClassicDiscoveryStopped(true /* unexpected */);
   }
 
  private:
+  void CleanFoundDevicesCache() { [inquiry_ clearFoundDevices]; }
+
   // 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_;
 
+  // Timer to clean up the cache. That way we get notified every time a device
+  // is seen.
+  base::Timer clean_cache_timer_;
+
   DISALLOW_COPY_AND_ASSIGN(BluetoothDiscoveryManagerMacClassic);
 };
 
 BluetoothDiscoveryManagerMac::BluetoothDiscoveryManagerMac(
     Observer* observer) : observer_(observer) {
   DCHECK(observer);
 }
 
 BluetoothDiscoveryManagerMac::~BluetoothDiscoveryManagerMac() {
 }
@@ skipping 25 matching lines @@
   manager_->DeviceFound(sender, device);
 }
 
 - (void)deviceInquiryComplete:(IOBluetoothDeviceInquiry*)sender
                         error:(IOReturn)error
                       aborted:(BOOL)aborted {
   manager_->DeviceInquiryComplete(sender, error, aborted);
 }
 
 @end