bluetooth: First iteration of StopScan

StopScan just sends the command directly to the active adapter.
Future iterations will cause the state to change to kTransitioning,
and support concurrent calls.

Bug: 870192
Change-Id: I84e57147e7b1c82f3137841d615d7250bfac4cb9
Reviewed-on: https://chromium-review.googlesource.com/c/1295612
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Ovidio Henriquez <odejesush@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604383}

ash/system/bluetooth/tray_bluetooth_helper_experimental.cc

@@ -9,6 +9,7 @@
 
 #include "ash/shell.h"
 #include "ash/system/tray/system_tray_notifier.h"
+#include "base/bind_helpers.h"
 #include "services/device/public/mojom/constants.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "services/service_manager/public/cpp/identity.h"
@@ -58,7 +59,7 @@ void TrayBluetoothHelperExperimental::StartBluetoothDiscovering() {
 }
 
 void TrayBluetoothHelperExperimental::StopBluetoothDiscovering() {
-  NOTIMPLEMENTED();
+  bluetooth_system_ptr_->StopScan(base::DoNothing());
 }
 
 void TrayBluetoothHelperExperimental::ConnectToBluetoothDevice(

device/bluetooth/bluez/bluetooth_bluez_unittest.cc

@@ -982,10 +982,11 @@ TEST_F(BluetoothBlueZTest, UnexpectedChangesDuringMultipleDiscoverySessions) {
   // bluez::FakeBluetoothAdapterClient's count should be only 1 and a single
   // call to
   // bluez::FakeBluetoothAdapterClient::StopDiscovery should work.
-  fake_bluetooth_adapter_client_->StopDiscovery(
+  fake_bluetooth_adapter_client_->BluetoothAdapterClient::StopDiscovery(
       dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
-      GetCallback(), base::Bind(&BluetoothBlueZTest::DBusErrorCallback,
-                                base::Unretained(this)));
+      GetCallback(),
+      base::Bind(&BluetoothBlueZTest::DBusErrorCallback,
+                 base::Unretained(this)));
   base::RunLoop().Run();
   EXPECT_EQ(2, observer.discovering_changed_count());
   EXPECT_EQ(4, callback_count_);
@@ -1091,10 +1092,11 @@ TEST_F(BluetoothBlueZTest, UnexpectedChangesDuringMultipleDiscoverySessions) {
   // Stop discovery via D-Bus. The fake client's reference count will drop but
   // the discovery state won't change since our BluetoothAdapter also just
   // requested it via D-Bus.
-  fake_bluetooth_adapter_client_->StopDiscovery(
+  fake_bluetooth_adapter_client_->BluetoothAdapterClient::StopDiscovery(
       dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
-      GetCallback(), base::Bind(&BluetoothBlueZTest::DBusErrorCallback,
-                                base::Unretained(this)));
+      GetCallback(),
+      base::Bind(&BluetoothBlueZTest::DBusErrorCallback,
+                 base::Unretained(this)));
   base::RunLoop().Run();
   EXPECT_EQ(5, observer.discovering_changed_count());
   EXPECT_EQ(10, callback_count_);
@@ -1169,10 +1171,11 @@ TEST_F(BluetoothBlueZTest, InvalidatedDiscoverySessions) {
   // should become inactive, but more importantly, we shouldn't run into any
   // memory errors as the sessions that we explicitly deleted should get
   // cleaned up.
-  fake_bluetooth_adapter_client_->StopDiscovery(
+  fake_bluetooth_adapter_client_->BluetoothAdapterClient::StopDiscovery(
       dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
-      GetCallback(), base::Bind(&BluetoothBlueZTest::DBusErrorCallback,
-                                base::Unretained(this)));
+      GetCallback(),
+      base::Bind(&BluetoothBlueZTest::DBusErrorCallback,
+                 base::Unretained(this)));
   base::RunLoop().Run();
   EXPECT_EQ(2, observer.discovering_changed_count());
   EXPECT_EQ(4, callback_count_);

device/bluetooth/dbus/bluetooth_adapter_client.cc

@@ -5,6 +5,7 @@
 #include "device/bluetooth/dbus/bluetooth_adapter_client.h"
 
 #include <string>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ -254,25 +255,21 @@ class BluetoothAdapterClientImpl : public BluetoothAdapterClient,
 
   // BluetoothAdapterClient override.
   void StopDiscovery(const dbus::ObjectPath& object_path,
-                     const base::Closure& callback,
-                     ErrorCallback error_callback) override {
+                     ResponseCallback callback) override {
     dbus::MethodCall method_call(bluetooth_adapter::kBluetoothAdapterInterface,
                                  bluetooth_adapter::kStopDiscovery);
 
     dbus::ObjectProxy* object_proxy =
         object_manager_->GetObjectProxy(object_path);
     if (!object_proxy) {
-      std::move(error_callback).Run(kUnknownAdapterError, "");
+      std::move(callback).Run(Error(kUnknownAdapterError, ""));
       return;
     }
 
-    object_proxy->CallMethodWithErrorCallback(
+    object_proxy->CallMethodWithErrorResponse(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&BluetoothAdapterClientImpl::OnSuccess,
-                       weak_ptr_factory_.GetWeakPtr(), callback),
-        base::BindOnce(&BluetoothAdapterClientImpl::OnError,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       std::move(error_callback)));
+        base::BindOnce(&BluetoothAdapterClientImpl::OnResponse,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   // BluetoothAdapterClient override.
@@ -606,4 +603,11 @@ void BluetoothAdapterClient::StartDiscovery(const dbus::ObjectPath& object_path,
                                              std::move(error_callback)));
 }
 
+void BluetoothAdapterClient::StopDiscovery(const dbus::ObjectPath& object_path,
+                                           const base::Closure& callback,
+                                           ErrorCallback error_callback) {
+  StopDiscovery(object_path, base::BindOnce(&OnResponseAdapter, callback,
+                                            std::move(error_callback)));
+}
+
 }  // namespace bluez

device/bluetooth/dbus/bluetooth_adapter_client.h

@@ -172,8 +172,11 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterClient : public BluezDBusClient {
   // Cancels any previous device discovery on the adapter with object path
   // |object_path|.
   virtual void StopDiscovery(const dbus::ObjectPath& object_path,
-                             const base::Closure& callback,
-                             ErrorCallback error_callback) = 0;
+                             ResponseCallback callback) = 0;
+  // DEPRECATED: Use StopDiscovery() above.
+  void StopDiscovery(const dbus::ObjectPath& object_path,
+                     const base::Closure& callback,
+                     ErrorCallback error_callback);
 
   // Pauses all discovery sessions.
   virtual void PauseDiscovery(const dbus::ObjectPath& object_path,

device/bluetooth/dbus/fake_bluetooth_adapter_client.cc

@@ -158,25 +158,24 @@ void FakeBluetoothAdapterClient::StartDiscovery(
 
 void FakeBluetoothAdapterClient::StopDiscovery(
     const dbus::ObjectPath& object_path,
-    const base::Closure& callback,
-    ErrorCallback error_callback) {
+    ResponseCallback callback) {
   if (object_path != dbus::ObjectPath(kAdapterPath)) {
     PostDelayedTask(
-        base::BindOnce(std::move(error_callback), kNoResponseError, ""));
+        base::BindOnce(std::move(callback), Error(kNoResponseError, "")));
     return;
   }
 
   if (!discovering_count_) {
     LOG(WARNING) << "StopDiscovery called when not discovering";
     PostDelayedTask(
-        base::BindOnce(std::move(error_callback), kNoResponseError, ""));
+        base::BindOnce(std::move(callback), Error(kNoResponseError, "")));
     return;
   }
 
   --discovering_count_;
   VLOG(1) << "StopDiscovery: " << object_path.value() << ", "
           << "count is now " << discovering_count_;
-  PostDelayedTask(callback);
+  PostDelayedTask(base::BindOnce(std::move(callback), base::nullopt));
 
   if (discovering_count_ == 0) {
     FakeBluetoothDeviceClient* device_client =

device/bluetooth/dbus/fake_bluetooth_adapter_client.h

@@ -5,6 +5,7 @@
 #ifndef DEVICE_BLUETOOTH_DBUS_FAKE_BLUETOOTH_ADAPTER_CLIENT_H_
 #define DEVICE_BLUETOOTH_DBUS_FAKE_BLUETOOTH_ADAPTER_CLIENT_H_
 
+#include <map>
 #include <memory>
 #include <string>
 #include <vector>
@@ -51,8 +52,7 @@ class DEVICE_BLUETOOTH_EXPORT FakeBluetoothAdapterClient
   void StartDiscovery(const dbus::ObjectPath& object_path,
                       ResponseCallback callback) override;
   void StopDiscovery(const dbus::ObjectPath& object_path,
-                     const base::Closure& callback,
-                     ErrorCallback error_callback) override;
+                     ResponseCallback callback) override;
   void PauseDiscovery(const dbus::ObjectPath& object_path,
                       const base::Closure& callback,
                       ErrorCallback error_callback) override;

services/device/bluetooth/bluetooth_system.cc

@@ -9,6 +9,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/bind.h"
 #include "dbus/object_path.h"
 #include "device/bluetooth/dbus/bluetooth_adapter_client.h"
 #include "device/bluetooth/dbus/bluez_dbus_manager.h"
@@ -158,6 +159,24 @@ void BluetoothSystem::StartScan(StartScanCallback callback) {
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
+void BluetoothSystem::StopScan(StopScanCallback callback) {
+  switch (state_) {
+    case State::kUnsupported:
+    case State::kUnavailable:
+    case State::kPoweredOff:
+    case State::kTransitioning:
+      std::move(callback).Run(StopScanResult::kBluetoothUnavailable);
+      return;
+    case State::kPoweredOn:
+      break;
+  }
+
+  GetBluetoothAdapterClient()->StopDiscovery(
+      active_adapter_.value(),
+      base::BindOnce(&BluetoothSystem::OnStopDiscovery,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+}
+
 bluez::BluetoothAdapterClient* BluetoothSystem::GetBluetoothAdapterClient() {
   // Use AlternateBluetoothAdapterClient to avoid interfering with users of the
   // regular BluetoothAdapterClient.
@@ -208,4 +227,13 @@ void BluetoothSystem::OnStartDiscovery(
                                 : StartScanResult::kSuccess);
 }
 
+void BluetoothSystem::OnStopDiscovery(
+    StopScanCallback callback,
+    const base::Optional<bluez::BluetoothAdapterClient::Error>& error) {
+  // TODO(https://crbug.com/897996): Use the name and message in |error| to
+  // return more specific error codes.
+  std::move(callback).Run(error ? StopScanResult::kFailedUnknownReason
+                                : StopScanResult::kSuccess);
+}
+
 }  // namespace device

services/device/bluetooth/bluetooth_system.h

@@ -40,6 +40,7 @@ class BluetoothSystem : public mojom::BluetoothSystem,
   void SetPowered(bool powered, SetPoweredCallback callback) override;
   void GetScanState(GetScanStateCallback callback) override;
   void StartScan(StartScanCallback callback) override;
+  void StopScan(StopScanCallback callback) override;
 
  private:
   bluez::BluetoothAdapterClient* GetBluetoothAdapterClient();
@@ -53,6 +54,9 @@ class BluetoothSystem : public mojom::BluetoothSystem,
   void OnStartDiscovery(
       StartScanCallback callback,
       const base::Optional<bluez::BluetoothAdapterClient::Error>& error);
+  void OnStopDiscovery(
+      StopScanCallback callback,
+      const base::Optional<bluez::BluetoothAdapterClient::Error>& error);
 
   mojom::BluetoothSystemClientPtr client_ptr_;
 

services/device/public/mojom/bluetooth_system.mojom

@@ -92,6 +92,26 @@ interface BluetoothSystem {
   //     there is one in progress already.
   //  2. Return more detailed error codes.
   StartScan() => (StartScanResult result);
+
+  enum StopScanResult {
+    // Command successfully sent to BT Radio.
+    kSuccess,
+    // Unknown failure when sending the command to the BT Radio.
+    kFailedUnknownReason,
+    // Can't use Bluetooth right now e.g. BT radio is off, or not present.
+    kBluetoothUnavailable,
+    // TODO(https://crbug.com/897996): Add more specific error codes.
+  };
+
+  // Attempts to stop scanning for Bluetooth devices. Callback is run with
+  // `kSuccess` if the command was successfully sent to the BT Radio. Once the
+  // BT Radio actually stops scanning for devices,
+  // BluetoothSystemClient::OnScanStateChanged will be called.
+  // TODO(https://crbug.com/897996): This function is missing two features:
+  //  1. Support concurrent calls; currently BlueZ just drops other calls if
+  //     there is one in progress already.
+  //  2. Return more detailed error codes.
+  StopScan() => (StopScanResult result);
 };
 
 // Interface used by clients of BluetoothSystem to get notified of events