Migrate Chrome OS Bluetooth UI to the new discovery API.

This CL migrates BluetoothOptionsHandler and ash::SystemTrayDelegate to the
new discovery session API, removing calls to the now deprecated
device::BluetoothAdapter::Start|StopDiscovering.

BUG=346982
TEST=Tested that discovery logic works on Uber tray and chrome://settings. Opened
multiple tabs with chrome://settings/search#Bluetooth. Also tested that all
BluetoothDiscoverySession objects are marked as inactive when the adapter gets
powered down via bt_console.

Review URL: https://codereview.chromium.org/184953002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@254727 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/resources/options/chromeos/bluetooth_add_device_overlay.js

@@ -34,15 +34,13 @@ cr.define('options', function() {
       this.createDeviceList_();
 
       $('bluetooth-add-device-cancel-button').onclick = function(event) {
-        chrome.send('stopBluetoothDeviceDiscovery');
         OptionsPage.closeOverlay();
       };
 
       var self = this;
       $('bluetooth-add-device-apply-button').onclick = function(event) {
         var device = self.deviceList_.selectedItem;
         var address = device.address;
-        chrome.send('stopBluetoothDeviceDiscovery');
         OptionsPage.closeOverlay();
         device.pairing = 'bluetoothStartConnecting';
         options.BluetoothPairing.showDialog(device);
@@ -68,6 +66,11 @@ cr.define('options', function() {
       });
     },
 
+    /** @override */
+    didClosePage: function() {
+      chrome.send('stopBluetoothDeviceDiscovery');
+    },
+
     /**
      * Creates, decorates and initializes the bluetooth device list.
      * @private

chrome/browser/ui/ash/system_tray_delegate_chromeos.cc

@@ -251,6 +251,7 @@ SystemTrayDelegateChromeOS::SystemTrayDelegateChromeOS()
       screen_locked_(false),
       have_session_start_time_(false),
       have_session_length_limit_(false),
+      should_run_bluetooth_discovery_(false),
       volume_control_delegate_(new VolumeController()),
       device_settings_observer_(CrosSettings::Get()->AddSettingsObserver(
           kSystemUse24HourClock,
@@ -703,12 +704,28 @@ void SystemTrayDelegateChromeOS::GetAvailableBluetoothDevices(
 }
 
 void SystemTrayDelegateChromeOS::BluetoothStartDiscovering() {
-  bluetooth_adapter_->StartDiscovering(
-      base::Bind(&base::DoNothing), base::Bind(&BluetoothSetDiscoveringError));
+  if (bluetooth_discovery_session_.get() &&
+      bluetooth_discovery_session_->IsActive()) {
+    LOG(WARNING) << "Already have active Bluetooth device discovery session.";
+    return;
+  }
+  VLOG(1) << "Requesting new Bluetooth device discovery session.";
+  should_run_bluetooth_discovery_ = true;
+  bluetooth_adapter_->StartDiscoverySession(
+      base::Bind(&SystemTrayDelegateChromeOS::OnStartBluetoothDiscoverySession,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&BluetoothSetDiscoveringError));
 }
 
 void SystemTrayDelegateChromeOS::BluetoothStopDiscovering() {
-  bluetooth_adapter_->StopDiscovering(
+  should_run_bluetooth_discovery_ = false;
+  if (!bluetooth_discovery_session_.get() ||
+      !bluetooth_discovery_session_->IsActive()) {
+    LOG(WARNING) << "No active Bluetooth device discovery session.";
+    return;
+  }
+  VLOG(1) << "Stopping Bluetooth device discovery session.";
+  bluetooth_discovery_session_->Stop(
       base::Bind(&base::DoNothing), base::Bind(&BluetoothSetDiscoveringError));
 }
 
@@ -1287,6 +1304,17 @@ void SystemTrayDelegateChromeOS::DeviceRemoved(
   GetSystemTrayNotifier()->NotifyRefreshBluetooth();
 }
 
+void SystemTrayDelegateChromeOS::OnStartBluetoothDiscoverySession(
+    scoped_ptr<device::BluetoothDiscoverySession> discovery_session) {
+  // If the discovery session was returned after a request to stop discovery
+  // (e.g. the user dismissed the Bluetooth detailed view before the call
+  // returned), don't claim the discovery session and let it clean up.
+  if (!should_run_bluetooth_discovery_)
+    return;
+  VLOG(1) << "Claiming new Bluetooth device discovery session.";
+  bluetooth_discovery_session_ = discovery_session.Pass();
+}
+
 // Overridden from SystemKeyEventListener::CapsLockObserver.
 void SystemTrayDelegateChromeOS::OnCapsLockChange(bool enabled) {
   bool search_mapped_to_caps_lock = false;

chrome/browser/ui/ash/system_tray_delegate_chromeos.h

@@ -29,6 +29,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "device/bluetooth/bluetooth_adapter.h"
+#include "device/bluetooth/bluetooth_discovery_session.h"
 
 namespace chromeos {
 
@@ -224,6 +225,9 @@ class SystemTrayDelegateChromeOS
   virtual void DeviceRemoved(device::BluetoothAdapter* adapter,
                              device::BluetoothDevice* device) OVERRIDE;
 
+  void OnStartBluetoothDiscoverySession(
+      scoped_ptr<device::BluetoothDiscoverySession> discovery_session);
+
   // Overridden from SystemKeyEventListener::CapsLockObserver.
   virtual void OnCapsLockChange(bool enabled) OVERRIDE;
 
@@ -251,8 +255,10 @@ class SystemTrayDelegateChromeOS
   bool have_session_length_limit_;
   base::TimeDelta session_length_limit_;
   std::string enterprise_domain_;
+  bool should_run_bluetooth_discovery_;
 
   scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
+  scoped_ptr<device::BluetoothDiscoverySession> bluetooth_discovery_session_;
   scoped_ptr<ash::VolumeControlDelegate> volume_control_delegate_;
   scoped_ptr<CrosSettingsObserverSubscription> device_settings_observer_;
   scoped_ptr<AccessibilityStatusSubscription> accessibility_subscription_;

chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc

@@ -55,20 +55,14 @@ const int kInvalidEntered = 0xFFFF;
 namespace chromeos {
 namespace options {
 
-BluetoothOptionsHandler::BluetoothOptionsHandler() :
-    discovering_(false),
-    pairing_device_passkey_(1000000),
-    pairing_device_entered_(kInvalidEntered),
-    weak_ptr_factory_(this) {
+BluetoothOptionsHandler::BluetoothOptionsHandler()
+    : should_run_device_discovery_(false),
+      pairing_device_passkey_(1000000),
+      pairing_device_entered_(kInvalidEntered),
+      weak_ptr_factory_(this) {
 }
 
 BluetoothOptionsHandler::~BluetoothOptionsHandler() {
-  if (discovering_) {
-    adapter_->StopDiscovering(
-        base::Bind(&base::DoNothing),
-        base::Bind(&base::DoNothing));
-    discovering_ = false;
-  }
   if (adapter_.get())
     adapter_->RemoveObserver(this);
 }
@@ -235,13 +229,26 @@ void BluetoothOptionsHandler::EnableChangeError() {
 
 void BluetoothOptionsHandler::FindDevicesCallback(
     const base::ListValue* args) {
-  if (!discovering_) {
-    discovering_ = true;
-    adapter_->StartDiscovering(
-        base::Bind(&base::DoNothing),
-        base::Bind(&BluetoothOptionsHandler::FindDevicesError,
-                   weak_ptr_factory_.GetWeakPtr()));
+  if (discovery_session_.get() && discovery_session_->IsActive()) {
+    VLOG(1) << "Already have an active discovery session.";
+    return;
   }
+  should_run_device_discovery_ = true;
+  adapter_->StartDiscoverySession(
+      base::Bind(&BluetoothOptionsHandler::OnStartDiscoverySession,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&BluetoothOptionsHandler::FindDevicesError,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void BluetoothOptionsHandler::OnStartDiscoverySession(
+    scoped_ptr<device::BluetoothDiscoverySession> discovery_session) {
+  // If the discovery session was returned after a request to stop discovery
+  // (e.g. the "Add Device" dialog was dismissed), don't claim the discovery
+  // session and let it clean up.
+  if (!should_run_device_discovery_)
+    return;
+  discovery_session_ = discovery_session.Pass();
 }
 
 void BluetoothOptionsHandler::FindDevicesError() {
@@ -402,13 +409,15 @@ void BluetoothOptionsHandler::ForgetError(const std::string& address) {
 
 void BluetoothOptionsHandler::StopDiscoveryCallback(
     const base::ListValue* args) {
-  if (discovering_) {
-    adapter_->StopDiscovering(
-        base::Bind(&base::DoNothing),
-        base::Bind(&BluetoothOptionsHandler::StopDiscoveryError,
-                   weak_ptr_factory_.GetWeakPtr()));
-    discovering_ = false;
+  should_run_device_discovery_ = false;
+  if (!discovery_session_.get() || !discovery_session_->IsActive()) {
+    VLOG(1) << "No active discovery session.";
+    return;
   }
+  discovery_session_->Stop(
+      base::Bind(&base::DoNothing),
+      base::Bind(&BluetoothOptionsHandler::StopDiscoveryError,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BluetoothOptionsHandler::StopDiscoveryError() {

chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h

@@ -14,6 +14,7 @@
 #include "chrome/browser/ui/webui/options/options_ui.h"
 #include "device/bluetooth/bluetooth_adapter.h"
 #include "device/bluetooth/bluetooth_device.h"
+#include "device/bluetooth/bluetooth_discovery_session.h"
 
 namespace base {
 class DictionaryValue;
@@ -154,12 +155,17 @@ class BluetoothOptionsHandler
   // change the power status of the adapter.
   void EnableChangeError();
 
+  // Called by device::BluetoothAdapter in response to a successful request
+  // to initiate a discovery session.
+  void OnStartDiscoverySession(
+      scoped_ptr<device::BluetoothDiscoverySession> discovery_session);
+
   // Called by device::BluetoothAdapter in response to a failure to
-  // set the adapter into discovery mode.
+  // initiate a discovery session.
   void FindDevicesError();
 
   // Called by device::BluetoothAdapter in response to a failure to
-  // remove the adapter from discovery mode.
+  // terminate a discovery session.
   void StopDiscoveryError();
 
   // Called by device::BluetoothDevice on a successful pairing and connection
@@ -210,8 +216,15 @@ class BluetoothOptionsHandler
   // Default bluetooth adapter, used for all operations.
   scoped_refptr<device::BluetoothAdapter> adapter_;
 
-  // True while performing device discovery.
-  bool discovering_;
+  // True, if the UI has requested device discovery. False, if either no device
+  // discovery was requested or the dialog responsible for device discovery was
+  // dismissed.
+  bool should_run_device_discovery_;
+
+  // The current device discovery session. Only one active discovery session is
+  // kept at a time and the instance that |discovery_session_| points to gets
+  // replaced by a new one when a new discovery session is initiated.
+  scoped_ptr<device::BluetoothDiscoverySession> discovery_session_;
 
   // Cached information about the current pairing device, if any.
   std::string pairing_device_address_;

device/bluetooth/bluetooth_adapter_chromeos.cc

@@ -625,8 +625,10 @@ void BluetoothAdapterChromeOS::DiscoveringChanged(
     bool discovering) {
   // If the adapter stopped discovery due to a reason other than a request by
   // us, reset the count to 0.
+  VLOG(1) << "Discovering changed: " << discovering;
   if (!discovering && !discovery_request_pending_
       && num_discovery_sessions_ > 0) {
+    VLOG(1) << "Marking sessions as inactive.";
     num_discovery_sessions_ = 0;
     MarkDiscoverySessionsAsInactive();
   }
@@ -680,8 +682,8 @@ void BluetoothAdapterChromeOS::AddDiscoverySession(
     // The pending request is either to stop a previous session or to start a
     // new one. Either way, queue this one.
     DCHECK(num_discovery_sessions_ == 1 || num_discovery_sessions_ == 0);
-    VLOG(1) << "Pending request to initiate device discovery. Queueing request "
-            << "to start a new discovery session.";
+    VLOG(1) << "Pending request to start/stop device discovery. Queueing "
+            << "request to start a new discovery session.";
     discovery_request_queue_.push(std::make_pair(callback, error_callback));
     return;
   }
@@ -708,6 +710,7 @@ void BluetoothAdapterChromeOS::AddDiscoverySession(
                      callback),
           base::Bind(&BluetoothAdapterChromeOS::OnStartDiscoveryError,
                      weak_ptr_factory_.GetWeakPtr(),
+                     callback,
                      error_callback));
 }
 
@@ -726,8 +729,8 @@ void BluetoothAdapterChromeOS::RemoveDiscoverySession(
 
   // If there is a pending request to BlueZ, then queue this request.
   if (discovery_request_pending_) {
-    VLOG(1) << "Pending request to initiate device discovery. Queueing request "
-            << "to stop discovery session.";
+    VLOG(1) << "Pending request to start/stop device discovery. Queueing "
+            << "request to stop discovery session.";
     error_callback.Run();
     return;
   }
@@ -759,6 +762,7 @@ void BluetoothAdapterChromeOS::RemoveDiscoverySession(
 
 void BluetoothAdapterChromeOS::OnStartDiscovery(const base::Closure& callback) {
   // Report success on the original request and increment the count.
+  VLOG(1) << __func__;
   DCHECK(discovery_request_pending_);
   DCHECK(num_discovery_sessions_ == 0);
   discovery_request_pending_ = false;
@@ -770,6 +774,7 @@ void BluetoothAdapterChromeOS::OnStartDiscovery(const base::Closure& callback) {
 }
 
 void BluetoothAdapterChromeOS::OnStartDiscoveryError(
+    const base::Closure& callback,
     const ErrorCallback& error_callback,
     const std::string& error_name,
     const std::string& error_message) {
@@ -780,14 +785,25 @@ void BluetoothAdapterChromeOS::OnStartDiscoveryError(
   DCHECK(num_discovery_sessions_ == 0);
   DCHECK(discovery_request_pending_);
   discovery_request_pending_ = false;
-  error_callback.Run();
+
+  // Discovery request may fail if discovery was previously initiated by Chrome,
+  // but the session were invalidated due to the discovery state unexpectedly
+  // changing to false and then back to true. In this case, report success.
+  if (error_name == bluetooth_device::kErrorInProgress && IsDiscovering()) {
+    VLOG(1) << "Discovery previously initiated. Reporting success.";
+    num_discovery_sessions_++;
+    callback.Run();
+  } else {
+    error_callback.Run();
+  }
 
   // Try to add a new discovery session for each queued request.
   ProcessQueuedDiscoveryRequests();
 }
 
 void BluetoothAdapterChromeOS::OnStopDiscovery(const base::Closure& callback) {
   // Report success on the original request and decrement the count.
+  VLOG(1) << __func__;
   DCHECK(discovery_request_pending_);
   DCHECK(num_discovery_sessions_ == 1);
   discovery_request_pending_ = false;
@@ -817,6 +833,7 @@ void BluetoothAdapterChromeOS::OnStopDiscoveryError(
 
 void BluetoothAdapterChromeOS::ProcessQueuedDiscoveryRequests() {
   while (!discovery_request_queue_.empty()) {
+    VLOG(1) << "Process queued discovery request.";
     DiscoveryCallbackPair callbacks = discovery_request_queue_.front();
     discovery_request_queue_.pop();
     AddDiscoverySession(callbacks.first, callbacks.second);

device/bluetooth/bluetooth_adapter_chromeos.h

@@ -194,7 +194,8 @@ class BluetoothAdapterChromeOS
 
   // Called by dbus:: on completion of the D-Bus method call to start discovery.
   void OnStartDiscovery(const base::Closure& callback);
-  void OnStartDiscoveryError(const ErrorCallback& error_callback,
+  void OnStartDiscoveryError(const base::Closure& callback,
+                             const ErrorCallback& error_callback,
                              const std::string& error_name,
                              const std::string& error_message);
 

device/bluetooth/bluetooth_chromeos_unittest.cc

@@ -246,7 +246,7 @@ class BluetoothChromeOSTest : public testing::Test {
   }
 
   virtual void TearDown() {
-    if (last_discovery_session_.get() && last_discovery_session_->active()) {
+    if (last_discovery_session_.get() && last_discovery_session_->IsActive()) {
       callback_count_ = 0;
       last_discovery_session_->Stop(
           base::Bind(&BluetoothChromeOSTest::Callback,
@@ -1323,7 +1323,7 @@ TEST_F(BluetoothChromeOSTest, StartDiscoverySession) {
   EXPECT_TRUE(observer.last_discovering_);
   EXPECT_TRUE(adapter_->IsDiscovering());
   EXPECT_TRUE(last_discovery_session_.get());
-  EXPECT_TRUE(last_discovery_session_->active());
+  EXPECT_TRUE(last_discovery_session_->IsActive());
 
   // Start another session. A new one should be returned in the callback, which
   // in turn will destroy the previous session. Adapter should still be
@@ -1340,7 +1340,7 @@ TEST_F(BluetoothChromeOSTest, StartDiscoverySession) {
   EXPECT_TRUE(observer.last_discovering_);
   EXPECT_TRUE(adapter_->IsDiscovering());
   EXPECT_TRUE(last_discovery_session_.get());
-  EXPECT_TRUE(last_discovery_session_->active());
+  EXPECT_TRUE(last_discovery_session_->IsActive());
 
   // Hold on to the current discovery session to prevent it from getting
   // destroyed during the next call to StartDiscoverySession.
@@ -1360,7 +1360,7 @@ TEST_F(BluetoothChromeOSTest, StartDiscoverySession) {
   EXPECT_TRUE(observer.last_discovering_);
   EXPECT_TRUE(adapter_->IsDiscovering());
   EXPECT_TRUE(last_discovery_session_.get());
-  EXPECT_TRUE(last_discovery_session_->active());
+  EXPECT_TRUE(last_discovery_session_->IsActive());
   EXPECT_NE(last_discovery_session_.get(), discovery_session);
 
   // Stop the previous discovery session. The session should end but discovery
@@ -1377,8 +1377,8 @@ TEST_F(BluetoothChromeOSTest, StartDiscoverySession) {
   EXPECT_TRUE(observer.last_discovering_);
   EXPECT_TRUE(adapter_->IsDiscovering());
   EXPECT_TRUE(last_discovery_session_.get());
-  EXPECT_TRUE(last_discovery_session_->active());
-  EXPECT_FALSE(discovery_session->active());
+  EXPECT_TRUE(last_discovery_session_->IsActive());
+  EXPECT_FALSE(discovery_session->IsActive());
 
   // Delete the current active session. Discovery should eventually stop.
   last_discovery_session_.reset();