[bluetooth] Integrate BT serial class w/ GetPort

Currently, the BluetoothSerialPortImpl interface isn't being reached
from anywhere in the codebase. By updating SerialPortManagerImpl's
GetPort() function, we can create a self-owned instance of
BluetoothSerialPortImpl where a serial port can be hooked up and use
its interface implementations later on.

Bug: 1043300
Change-Id: I06911b4918dcba8c2b240a689454aeb8b9ae22e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363670
Commit-Queue: Vicky Min <vickymin@google.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801263}

services/device/public/mojom/serial.mojom

@@ -18,7 +18,7 @@ struct SerialPortInfo {
 
   // This member is used to identify whether the SerialPortInfo object is
   // converted from a Bluetooth serial device.
-  DeviceType type;
+  DeviceType type = PLATFORM_SERIAL;
 
   // On macOS a serial device may have two paths, one for the call-out device
   // and one for the dial-in device. The call-out device is preferred. If

services/device/serial/bluetooth_serial_device_enumerator.cc

@@ -34,7 +34,7 @@ void BluetoothSerialDeviceEnumerator::OnGotClassicAdapter(
     if (base::Contains(device_uuids, GetSerialPortProfileUUID())) {
       auto port = mojom::SerialPortInfo::New();
       port->token = base::UnguessableToken::Create();
-      port->path = base::FilePath::FromUTF8Unsafe(device->GetIdentifier());
+      port->path = base::FilePath::FromUTF8Unsafe(device->GetAddress());
       port->type = mojom::DeviceType::SPP_DEVICE;
       bluetooth_ports_.insert(
           std::make_pair(device->GetAddress(), port->token));
@@ -49,7 +49,7 @@ void BluetoothSerialDeviceEnumerator::DeviceAdded(BluetoothAdapter* adapter,
   if (base::Contains(device_uuids, GetSerialPortProfileUUID())) {
     auto port = mojom::SerialPortInfo::New();
     port->token = base::UnguessableToken::Create();
-    port->path = base::FilePath::FromUTF8Unsafe(device->GetIdentifier());
+    port->path = base::FilePath::FromUTF8Unsafe(device->GetAddress());
     port->type = mojom::DeviceType::SPP_DEVICE;
     bluetooth_ports_.insert(std::make_pair(device->GetAddress(), port->token));
     AddPort(std::move(port));
@@ -65,4 +65,18 @@ void BluetoothSerialDeviceEnumerator::DeviceRemoved(BluetoothAdapter* adapter,
   RemovePort(token);
 }
 
+scoped_refptr<BluetoothAdapter> BluetoothSerialDeviceEnumerator::GetAdapter() {
+  return adapter_;
+}
+
+base::Optional<std::string>
+BluetoothSerialDeviceEnumerator::GetAddressFromToken(
+    const base::UnguessableToken& token) {
+  for (const auto& entry : bluetooth_ports_) {
+    if (entry.second == token)
+      return entry.first;
+  }
+  return base::nullopt;
+}
+
 }  // namespace device

services/device/serial/bluetooth_serial_device_enumerator.h

@@ -29,6 +29,13 @@ class BluetoothSerialDeviceEnumerator : public BluetoothAdapter::Observer,
   void DeviceRemoved(BluetoothAdapter* adapter,
                      BluetoothDevice* device) override;
 
+  scoped_refptr<BluetoothAdapter> GetAdapter();
+
+  // This method will search the map of Bluetooth ports and find the
+  // address with the matching token.
+  base::Optional<std::string> GetAddressFromToken(
+      const base::UnguessableToken& token);
+
  protected:
   scoped_refptr<BluetoothAdapter> adapter_;
 

services/device/serial/bluetooth_serial_port_impl.cc

@@ -14,26 +14,29 @@ namespace device {
 
 // static
 void BluetoothSerialPortImpl::Create(
-    std::unique_ptr<BluetoothDevice> device,
+    scoped_refptr<BluetoothAdapter> adapter,
+    const std::string& address,
     mojo::PendingReceiver<mojom::SerialPort> receiver,
     mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher) {
   DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBluetoothSerialPortProfileInSerialApi));
 
   // This BluetoothSerialPortImpl is owned by |receiver| and |watcher|.
-  new BluetoothSerialPortImpl(std::move(device), std::move(receiver),
+  new BluetoothSerialPortImpl(std::move(adapter), address, std::move(receiver),
                               std::move(watcher));
 }
 
 BluetoothSerialPortImpl::BluetoothSerialPortImpl(
-    std::unique_ptr<BluetoothDevice> device,
+    scoped_refptr<BluetoothAdapter> adapter,
+    const std::string& address,
     mojo::PendingReceiver<mojom::SerialPort> receiver,
     mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher)
     : receiver_(this, std::move(receiver)),
       watcher_(std::move(watcher)),
       in_stream_watcher_(FROM_HERE, mojo::SimpleWatcher::ArmingPolicy::MANUAL),
       out_stream_watcher_(FROM_HERE, mojo::SimpleWatcher::ArmingPolicy::MANUAL),
-      bluetooth_device_(std::move(device)) {
+      bluetooth_adapter_(std::move(adapter)),
+      address_(address) {
   receiver_.set_disconnect_handler(
       base::BindOnce([](BluetoothSerialPortImpl* self) { delete self; },
                      base::Unretained(this)));
@@ -55,12 +58,16 @@ void BluetoothSerialPortImpl::Open(
     OpenCallback callback) {
   if (client)
     client_.Bind(std::move(client));
-
-  BluetoothDevice::UUIDSet device_uuids = bluetooth_device_->GetUUIDs();
+  BluetoothDevice* device = bluetooth_adapter_->GetDevice(address_);
+  if (!device) {
+    std::move(callback).Run(false);
+    return;
+  }
+  BluetoothDevice::UUIDSet device_uuids = device->GetUUIDs();
   if (base::Contains(device_uuids, GetSerialPortProfileUUID())) {
     auto copyable_callback =
         base::AdaptCallbackForRepeating(std::move(callback));
-    bluetooth_device_->ConnectToService(
+    device->ConnectToService(
         GetSerialPortProfileUUID(),
         base::BindOnce(&BluetoothSerialPortImpl::OnSocketConnected,
                        weak_ptr_factory_.GetWeakPtr(), copyable_callback),

services/device/serial/bluetooth_serial_port_impl.h

@@ -7,6 +7,7 @@
 
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "device/bluetooth/bluetooth_adapter.h"
 #include "device/bluetooth/bluetooth_device.h"
 #include "device/bluetooth/bluetooth_socket.h"
 #include "mojo/public/cpp/system/data_pipe.h"
@@ -22,15 +23,17 @@ namespace device {
 class BluetoothSerialPortImpl : public mojom::SerialPort {
  public:
   // Creates of instance of BluetoothSerialPortImpl using a Bluetooth
-  // device and a receiver/watcher to create a pipe. The receiver and
-  // watcher will own this object.
+  // adapter, a Bluetooth device address and a receiver/watcher to
+  // create a pipe. The receiver and watcher will own this object.
   static void Create(
-      std::unique_ptr<BluetoothDevice> device,
+      scoped_refptr<BluetoothAdapter> adapter,
+      const std::string& address,
       mojo::PendingReceiver<mojom::SerialPort> receiver,
       mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher);
 
   BluetoothSerialPortImpl(
-      std::unique_ptr<BluetoothDevice> device,
+      scoped_refptr<BluetoothAdapter> adapter,
+      const std::string& address,
       mojo::PendingReceiver<mojom::SerialPort> receiver,
       mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher);
   BluetoothSerialPortImpl(const BluetoothSerialPortImpl&) = delete;
@@ -91,8 +94,9 @@ class BluetoothSerialPortImpl : public mojom::SerialPort {
   FlushCallback write_flush_callback_;
   DrainCallback drain_callback_;
 
-  scoped_refptr<device::BluetoothSocket> bluetooth_socket_;
-  std::unique_ptr<BluetoothDevice> bluetooth_device_;
+  scoped_refptr<BluetoothSocket> bluetooth_socket_;
+  const scoped_refptr<BluetoothAdapter> bluetooth_adapter_;
+  const std::string address_;
 
   bool read_pending_ = false;
   bool write_pending_ = false;

services/device/serial/bluetooth_serial_port_impl_unittest.cc

@@ -33,6 +33,7 @@ namespace {
 using ::base::test::RunOnceCallback;
 using ::testing::_;
 using ::testing::Invoke;
+using ::testing::Return;
 using ::testing::WithArgs;
 
 constexpr char kBuffer[] = "test";
@@ -62,15 +63,17 @@ class BluetoothSerialPortImplTest : public testing::Test {
     scoped_refptr<MockBluetoothAdapter> adapter =
         base::MakeRefCounted<MockBluetoothAdapter>();
     device::BluetoothAdapterFactory::SetAdapterForTesting(adapter);
-    auto mock_device = std::make_unique<MockBluetoothDevice>(
+    mock_device_ = std::make_unique<MockBluetoothDevice>(
         adapter.get(), 0, "Test Device", kDeviceAddress, false, false);
-    mock_device->AddUUID(GetSerialPortProfileUUID());
+    mock_device_->AddUUID(GetSerialPortProfileUUID());
 
-    EXPECT_CALL(*mock_device,
+    EXPECT_CALL(*adapter, GetDevice(kDeviceAddress))
+        .WillOnce(Return(mock_device_.get()));
+    EXPECT_CALL(*mock_device_,
                 ConnectToService(GetSerialPortProfileUUID(), _, _))
         .WillOnce(RunOnceCallback<1>(mock_socket_));
 
-    BluetoothSerialPortImpl::Create(std::move(mock_device),
+    BluetoothSerialPortImpl::Create(std::move(adapter), kDeviceAddress,
                                     port->BindNewPipeAndPassReceiver(),
                                     std::move(watcher_remote));
   }
@@ -86,15 +89,17 @@ class BluetoothSerialPortImplTest : public testing::Test {
     scoped_refptr<MockBluetoothAdapter> adapter =
         base::MakeRefCounted<MockBluetoothAdapter>();
     device::BluetoothAdapterFactory::SetAdapterForTesting(adapter);
-    auto mock_device = std::make_unique<MockBluetoothDevice>(
+    mock_device_ = std::make_unique<MockBluetoothDevice>(
         adapter.get(), 0, "Test Device", kDeviceAddress, false, false);
-    mock_device->AddUUID(GetSerialPortProfileUUID());
+    mock_device_->AddUUID(GetSerialPortProfileUUID());
 
-    EXPECT_CALL(*mock_device,
+    EXPECT_CALL(*adapter, GetDevice(kDeviceAddress))
+        .WillOnce(Return(mock_device_.get()));
+    EXPECT_CALL(*mock_device_,
                 ConnectToService(GetSerialPortProfileUUID(), _, _))
         .WillOnce(RunOnceCallback<2>("Error"));
 
-    BluetoothSerialPortImpl::Create(std::move(mock_device),
+    BluetoothSerialPortImpl::Create(std::move(adapter), kDeviceAddress,
                                     port->BindNewPipeAndPassReceiver(),
                                     std::move(watcher_remote));
   }
@@ -116,6 +121,7 @@ class BluetoothSerialPortImplTest : public testing::Test {
  private:
   scoped_refptr<MockBluetoothSocket> mock_socket_ =
       base::MakeRefCounted<MockBluetoothSocket>();
+  std::unique_ptr<MockBluetoothDevice> mock_device_;
 
   base::test::SingleThreadTaskEnvironment task_environment_;
 };

services/device/serial/serial_port_manager_impl.cc

@@ -14,6 +14,7 @@
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "services/device/public/cpp/serial/serial_switches.h"
 #include "services/device/serial/bluetooth_serial_device_enumerator.h"
+#include "services/device/serial/bluetooth_serial_port_impl.h"
 #include "services/device/serial/serial_device_enumerator.h"
 #include "services/device/serial/serial_port_impl.h"
 
@@ -90,6 +91,17 @@ void SerialPortManagerImpl::GetPort(
         FROM_HERE,
         base::BindOnce(&SerialPortImpl::Create, *path, std::move(receiver),
                        std::move(watcher), ui_task_runner_));
+    return;
+  }
+
+  DCHECK(bluetooth_enumerator_);
+  base::Optional<std::string> address =
+      bluetooth_enumerator_->GetAddressFromToken(token);
+  if (address) {
+    ui_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&BluetoothSerialPortImpl::Create,
+                                  bluetooth_enumerator_->GetAdapter(), *address,
+                                  std::move(receiver), std::move(watcher)));
   }
 }
 

services/device/serial/serial_port_manager_impl_unittest.cc

@@ -14,16 +14,19 @@
 #include "base/macros.h"
 #include "base/task/post_task.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/gmock_callback_support.h"
 #include "base/threading/thread.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/public/cpp/bluetooth_uuid.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
 #include "device/bluetooth/test/mock_bluetooth_device.h"
+#include "device/bluetooth/test/mock_bluetooth_socket.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "services/device/device_service_test_base.h"
+#include "services/device/public/cpp/bluetooth/bluetooth_utils.h"
 #include "services/device/public/cpp/serial/serial_switches.h"
 #include "services/device/public/mojom/serial.mojom.h"
 #include "services/device/serial/bluetooth_serial_device_enumerator.h"
@@ -31,8 +34,10 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using testing::_;
-using testing::Invoke;
+using ::base::test::RunOnceCallback;
+using ::testing::_;
+using ::testing::Invoke;
+using ::testing::Return;
 
 namespace device {
 
@@ -63,6 +68,25 @@ class MockSerialPortManagerClient : public mojom::SerialPortManagerClient {
   mojo::Receiver<mojom::SerialPortManagerClient> receiver_{this};
 };
 
+class FakeSerialPortClient : public mojom::SerialPortClient {
+ public:
+  FakeSerialPortClient() = default;
+  FakeSerialPortClient(FakeSerialPortClient&) = delete;
+  FakeSerialPortClient& operator=(FakeSerialPortClient&) = delete;
+  ~FakeSerialPortClient() override = default;
+
+  void Bind(mojo::PendingReceiver<device::mojom::SerialPortClient> receiver) {
+    receiver_.Bind(std::move(receiver));
+  }
+
+  // mojom::SerialPortClient
+  void OnReadError(mojom::SerialReceiveError error) override {}
+  void OnSendError(mojom::SerialSendError error) override {}
+
+ private:
+  mojo::Receiver<mojom::SerialPortClient> receiver_{this};
+};
+
 }  // namespace
 
 class SerialPortManagerImplTest : public DeviceServiceTestBase {
@@ -94,10 +118,38 @@ class SerialPortManagerImplTest : public DeviceServiceTestBase {
 
     auto mock_device = std::make_unique<MockBluetoothDevice>(
         adapter_.get(), 0, "Test Device", kDeviceAddress, false, false);
-    static const BluetoothUUID kSerialPortProfileUUID("1101");
-    mock_device->AddUUID(kSerialPortProfileUUID);
+    mock_device->AddUUID(GetSerialPortProfileUUID());
+    adapter_->AddMockDevice(std::move(mock_device));
+
+    auto bluetooth_enumerator =
+        std::make_unique<BluetoothSerialDeviceEnumerator>();
+    bluetooth_enumerator_ = bluetooth_enumerator.get();
+
+    manager_->SetBluetoothSerialEnumeratorForTesting(
+        std::move(bluetooth_enumerator));
+  }
+
+  void SetupBluetoothEnumeratorWithExpectations() {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kEnableBluetoothSerialPortProfileInSerialApi);
+
+    ON_CALL(*adapter_, GetDevices())
+        .WillByDefault(
+            Invoke(adapter_.get(), &MockBluetoothAdapter::GetConstMockDevices));
+    device::BluetoothAdapterFactory::SetAdapterForTesting(adapter_);
+
+    auto mock_device = std::make_unique<MockBluetoothDevice>(
+        adapter_.get(), 0, "Test Device", kDeviceAddress, false, false);
+    mock_device->AddUUID(GetSerialPortProfileUUID());
+    MockBluetoothDevice* mock_device_ptr = mock_device.get();
     adapter_->AddMockDevice(std::move(mock_device));
 
+    EXPECT_CALL(*adapter_, GetDevice(kDeviceAddress))
+        .WillOnce(Return(mock_device_ptr));
+    EXPECT_CALL(*mock_device_ptr,
+                ConnectToService(GetSerialPortProfileUUID(), _, _))
+        .WillOnce(RunOnceCallback<1>(mock_socket_));
+
     auto bluetooth_enumerator =
         std::make_unique<BluetoothSerialDeviceEnumerator>();
     bluetooth_enumerator_ = bluetooth_enumerator.get();
@@ -111,6 +163,8 @@ class SerialPortManagerImplTest : public DeviceServiceTestBase {
   BluetoothSerialDeviceEnumerator* bluetooth_enumerator_;
   scoped_refptr<MockBluetoothAdapter> adapter_ =
       base::MakeRefCounted<MockBluetoothAdapter>();
+  scoped_refptr<MockBluetoothSocket> mock_socket_ =
+      base::MakeRefCounted<MockBluetoothSocket>();
 
   void Bind(mojo::PendingReceiver<mojom::SerialPortManager> receiver) {
     manager_->Bind(std::move(receiver));
@@ -154,11 +208,9 @@ TEST_F(SerialPortManagerImplTest, GetDevices) {
   SetupBluetoothEnumerator();
   mojo::Remote<mojom::SerialPortManager> port_manager;
   Bind(port_manager.BindNewPipeAndPassReceiver());
-  const std::string address_identifier =
-      std::string(kDeviceAddress) + "-Identifier";
   const std::set<base::FilePath> expected_paths = {
       kFakeDevicePath1, kFakeDevicePath2,
-      base::FilePath::FromUTF8Unsafe(address_identifier)};
+      base::FilePath::FromUTF8Unsafe(kDeviceAddress)};
 
   base::RunLoop loop;
   port_manager->GetDevices(base::BindLambdaForTesting(
@@ -247,6 +299,58 @@ TEST_F(SerialPortManagerImplTest, GetPort) {
   loop.Run();
 }
 
+TEST_F(SerialPortManagerImplTest, GetBluetoothDevicePort) {
+  SetupBluetoothEnumeratorWithExpectations();
+  mojo::Remote<mojom::SerialPortManager> port_manager;
+  Bind(port_manager.BindNewPipeAndPassReceiver());
+  mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher_remote;
+  mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher =
+      mojo::MakeSelfOwnedReceiver(
+          std::make_unique<mojom::SerialPortConnectionWatcher>(),
+          watcher_remote.InitWithNewPipeAndPassReceiver());
+  auto options = mojom::SerialConnectionOptions::New();
+  mojo::PendingRemote<mojom::SerialPortClient> client;
+  FakeSerialPortClient serial_client;
+  serial_client.Bind(client.InitWithNewPipeAndPassReceiver());
+  mojo::Remote<mojom::SerialPort> serial_port;
+
+  // Since we only want to use devices enumerated by the Bluetooth
+  // enumerator, we can remove the devices that are not.
+  enumerator_->RemoveDevicePath(kFakeDevicePath1);
+  enumerator_->RemoveDevicePath(kFakeDevicePath2);
+
+  const std::set<base::FilePath> expected_paths = {
+      base::FilePath::FromUTF8Unsafe(kDeviceAddress)};
+
+  base::RunLoop loop;
+  port_manager->GetDevices(base::BindLambdaForTesting(
+      [&](std::vector<mojom::SerialPortInfoPtr> results) {
+        EXPECT_EQ(expected_paths.size(), results.size());
+        std::set<base::FilePath> actual_paths;
+        for (size_t i = 0; i < results.size(); ++i)
+          actual_paths.insert(results[i]->path);
+        EXPECT_EQ(expected_paths, actual_paths);
+
+        port_manager->GetPort(results[0]->token,
+                              /*use_alternate_path=*/false,
+                              serial_port.BindNewPipeAndPassReceiver(),
+                              /*watcher=*/std::move(watcher_remote));
+        serial_port->Open(std::move(options), std::move(client),
+                          base::BindLambdaForTesting([&loop](bool success) {
+                            EXPECT_TRUE(success);
+                            loop.Quit();
+                          }));
+      }));
+
+  loop.Run();
+
+  base::RunLoop disconnect_loop;
+  watcher->set_connection_error_handler(disconnect_loop.QuitClosure());
+
+  serial_port.reset();
+  disconnect_loop.Run();
+}
+
 TEST_F(SerialPortManagerImplTest, BluetoothPortRemovedAndAdded) {
   SetupBluetoothEnumerator();
   mojo::Remote<mojom::SerialPortManager> port_manager;
@@ -255,16 +359,13 @@ TEST_F(SerialPortManagerImplTest, BluetoothPortRemovedAndAdded) {
   MockSerialPortManagerClient client;
   port_manager->SetClient(client.BindNewPipeAndPassRemote());
 
-  const std::string address_identifier =
-      std::string(kDeviceAddress) + "-Identifier";
   base::UnguessableToken port1_token;
   {
     base::RunLoop run_loop;
     port_manager->GetDevices(base::BindLambdaForTesting(
         [&](std::vector<mojom::SerialPortInfoPtr> results) {
           for (const auto& port : results) {
-            if (port->path ==
-                base::FilePath::FromUTF8Unsafe(address_identifier)) {
+            if (port->path == base::FilePath::FromUTF8Unsafe(kDeviceAddress)) {
               port1_token = port->token;
               break;
             }
@@ -282,8 +383,7 @@ TEST_F(SerialPortManagerImplTest, BluetoothPortRemovedAndAdded) {
     EXPECT_CALL(client, OnPortRemoved(_))
         .WillOnce(Invoke([&](mojom::SerialPortInfoPtr port) {
           EXPECT_EQ(port1_token, port->token);
-          EXPECT_EQ(port->path,
-                    base::FilePath::FromUTF8Unsafe(address_identifier));
+          EXPECT_EQ(port->path, base::FilePath::FromUTF8Unsafe(kDeviceAddress));
           EXPECT_EQ(mojom::DeviceType::SPP_DEVICE, port->type);
           run_loop.Quit();
         }));
@@ -303,8 +403,7 @@ TEST_F(SerialPortManagerImplTest, BluetoothPortRemovedAndAdded) {
     EXPECT_CALL(client, OnPortAdded(_))
         .WillOnce(Invoke([&](mojom::SerialPortInfoPtr port) {
           EXPECT_NE(port1_token, port->token);
-          EXPECT_EQ(port->path,
-                    base::FilePath::FromUTF8Unsafe(address_identifier));
+          EXPECT_EQ(port->path, base::FilePath::FromUTF8Unsafe(kDeviceAddress));
           EXPECT_EQ(mojom::DeviceType::SPP_DEVICE, port->type);
           run_loop.Quit();
         }));