arc: bluetooth: Separate mojo callbacks for connection state changed

For the device connected state change, use a separate callback for
Android, and leave the existing callback for reflecting the state of
BluetoothGattConnection object we hold. Also move the code for this
from DeviceChanged() to DeviceConnectedStateChanged() which will be
much clearer than before.

This change is needed for the gatt connection management and
acl_stated_changed() callback (ag/10312919) on the Android side.

BUG=b:148355935
BUG=b:149658771
TEST=unit_tests --gtest_filter=ArcBluetoothBridgeTest.*

Change-Id: I8cdd734c88402f61547d8dc1d0b3178d3f8c7f0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2053748
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Commit-Queue: Jie Jiang <jiejiang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762706}

chrome/browser/chromeos/arc/bluetooth/arc_bluetooth_bridge.cc

@@ -510,6 +510,11 @@ void ArcBluetoothBridge::AdapterPoweredChanged(BluetoothAdapter* adapter,
 void ArcBluetoothBridge::DeviceAdded(BluetoothAdapter* adapter,
                                      BluetoothDevice* device) {
   DeviceChanged(adapter, device);
+
+  // We need to trigger this manually if the device is connected when it is
+  // added. This may happen for a incoming connection from an unknown device.
+  if (device->IsConnected())
+    DeviceConnectedStateChanged(adapter, device, /*is_now_connected=*/true);
 }
 
 void ArcBluetoothBridge::DeviceChanged(BluetoothAdapter* adapter,
@@ -534,27 +539,6 @@ void ArcBluetoothBridge::DeviceChanged(BluetoothAdapter* adapter,
           GetDeviceProperties(mojom::BluetoothPropertyType::ALL, device));
     }
   }
-
-  if (!(device->GetType() & device::BLUETOOTH_TRANSPORT_LE))
-    return;
-
-  auto it = gatt_connections_.find(addr);
-  bool was_connected =
-      (it != gatt_connections_.end() &&
-       it->second.state == GattConnection::ConnectionState::CONNECTED);
-  bool is_connected = device->IsConnected();
-  if (was_connected == is_connected)
-    return;
-
-  if (!is_connected) {
-    OnGattDisconnected(mojom::BluetoothAddress::From(addr));
-    return;
-  }
-
-  // Only process connection from remote device. Connection to remote device is
-  // processed in the callback of ConnectLEDevice().
-  if (it == gatt_connections_.end())
-    OnGattConnected(mojom::BluetoothAddress::From(addr), nullptr);
 }
 
 void ArcBluetoothBridge::DeviceAddressChanged(BluetoothAdapter* adapter,
@@ -649,6 +633,31 @@ void ArcBluetoothBridge::DeviceAdvertisementReceived(
                                             GetAdvertisingData(device));
 }
 
+void ArcBluetoothBridge::DeviceConnectedStateChanged(BluetoothAdapter* adapter,
+                                                     BluetoothDevice* device,
+                                                     bool is_now_connected) {
+  if (!arc_bridge_service_->bluetooth()->IsConnected())
+    return;
+
+  const std::string addr = device->GetAddress();
+
+  // If this event is about 1) an device supports LE becomes disconnected and 2)
+  // we are holding the connection object for this device, we need to remove
+  // this object and notify Android.
+  bool support_le = device->GetType() & device::BLUETOOTH_TRANSPORT_LE;
+  auto it = gatt_connections_.find(addr);
+  if (support_le && it != gatt_connections_.end() && !is_now_connected)
+    OnGattDisconnected(mojom::BluetoothAddress::From(addr));
+
+  auto* bluetooth_instance = ARC_GET_INSTANCE_FOR_METHOD(
+      arc_bridge_service_->bluetooth(), OnConnectionStateChanged);
+  if (!bluetooth_instance)
+    return;
+
+  bluetooth_instance->OnConnectionStateChanged(
+      mojom::BluetoothAddress::From(addr), device->GetType(), is_now_connected);
+}
+
 void ArcBluetoothBridge::DeviceRemoved(BluetoothAdapter* adapter,
                                        BluetoothDevice* device) {
   if (!arc_bridge_service_->bluetooth()->IsConnected())

chrome/browser/chromeos/arc/bluetooth/arc_bluetooth_bridge.h

@@ -104,6 +104,10 @@ class ArcBluetoothBridge
                                    int16_t rssi,
                                    const std::vector<uint8_t>& eir) override;
 
+  void DeviceConnectedStateChanged(device::BluetoothAdapter* adapter,
+                                   device::BluetoothDevice* device,
+                                   bool is_now_connected) override;
+
   void DeviceRemoved(device::BluetoothAdapter* adapter,
                      device::BluetoothDevice* device) override;
 

chrome/browser/chromeos/arc/bluetooth/arc_bluetooth_bridge_unittest.cc

@@ -82,6 +82,17 @@ class ArcBluetoothBridgeTest : public testing::Test {
         rssi);
   }
 
+  void ChangeTestDeviceConnected(bool connected) {
+    bluez::BluezDBusManager* dbus_manager = bluez::BluezDBusManager::Get();
+    auto* fake_bluetooth_device_client =
+        static_cast<bluez::FakeBluetoothDeviceClient*>(
+            dbus_manager->GetBluetoothDeviceClient());
+    bluez::FakeBluetoothDeviceClient::Properties* properties =
+        fake_bluetooth_device_client->GetProperties(
+            dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+    properties->connected.ReplaceValue(connected);
+  }
+
   void OnAdapterInitialized(scoped_refptr<device::BluetoothAdapter> adapter) {
     adapter_ = adapter;
     get_adapter_run_loop_.Quit();
@@ -108,7 +119,7 @@ class ArcBluetoothBridgeTest : public testing::Test {
         nullptr, arc_bridge_service_.get());
     fake_bluetooth_instance_ = std::make_unique<FakeBluetoothInstance>();
     arc_bridge_service_->bluetooth()->SetInstance(
-        fake_bluetooth_instance_.get(), 16);
+        fake_bluetooth_instance_.get(), 17);
     base::SysInfo::SetChromeOSVersionInfoForTest(
         "CHROMEOS_ARC_ANDROID_SDK_VERSION=28", base::Time::Now());
     WaitForInstanceReady(arc_bridge_service_->bluetooth());
@@ -322,6 +333,98 @@ TEST_F(ArcBluetoothBridgeTest, LEDeviceFoundForN) {
             fake_bluetooth_instance_->le_device_found_data().back()->rssi());
 }
 
+// If ARC starts the connection by LEConnect(), OnLEConnectionStateChange()
+// should be invoked when the connection is up/down. OnConnectionStateChanged()
+// should always be invoked when the physical link state changed.
+TEST_F(ArcBluetoothBridgeTest, DeviceConnectStateChangedAfterLEConnectReuqest) {
+  AddTestDevice();
+  arc_bluetooth_bridge_->ConnectLEDevice(mojom::BluetoothAddress::From(
+      std::string(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress)));
+
+  // OnConnectionStateChanged() should be invoked.
+  ASSERT_EQ(1u,
+            fake_bluetooth_instance_->connection_state_changed_data().size());
+  const auto& connected_data =
+      fake_bluetooth_instance_->connection_state_changed_data().back();
+  EXPECT_EQ(std::string(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress),
+            connected_data->addr()->To<std::string>());
+  EXPECT_EQ(device::BLUETOOTH_TRANSPORT_LE, connected_data->device_type());
+  EXPECT_EQ(true, connected_data->connected());
+
+  // OnLEConnectionStateChange() should be invoked.
+  ASSERT_EQ(1u,
+            fake_bluetooth_instance_->le_connection_state_change_data().size());
+  const auto& le_connected_data =
+      fake_bluetooth_instance_->le_connection_state_change_data().back();
+  EXPECT_EQ(std::string(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress),
+            le_connected_data->addr()->To<std::string>());
+  EXPECT_EQ(true, le_connected_data->connected());
+
+  // Device is disconnected.
+  ChangeTestDeviceConnected(false);
+
+  // OnConnectionStateChanged() should be invoked.
+  ASSERT_EQ(2u,
+            fake_bluetooth_instance_->connection_state_changed_data().size());
+  const auto& disconnected_data =
+      fake_bluetooth_instance_->connection_state_changed_data().back();
+  EXPECT_EQ(std::string(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress),
+            disconnected_data->addr()->To<std::string>());
+  EXPECT_EQ(device::BLUETOOTH_TRANSPORT_LE, disconnected_data->device_type());
+  EXPECT_EQ(false, disconnected_data->connected());
+
+  // OnLEConnectionStateChange() should be invoked.
+  ASSERT_EQ(2u,
+            fake_bluetooth_instance_->le_connection_state_change_data().size());
+  const auto& le_disconnected_data =
+      fake_bluetooth_instance_->le_connection_state_change_data().back();
+  EXPECT_EQ(std::string(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress),
+            le_disconnected_data->addr()->To<std::string>());
+  EXPECT_EQ(false, le_disconnected_data->connected());
+}
+
+// If the connection is up/down and is not requested by ARC (e.g., in the case
+// that another app in CrOS starts the connection, or it is an incoming
+// connection), OnLEConnectionStateChange() should not be invoked.
+// OnConnectionStateChanged() should always be invoked when the physical link
+// state changed.
+TEST_F(ArcBluetoothBridgeTest,
+       DeviceConnectStateChangedWithoutLEConnectRequest) {
+  AddTestDevice();
+  ChangeTestDeviceConnected(true);
+
+  // OnConnectionStateChanged() should be invoked.
+  ASSERT_EQ(1u,
+            fake_bluetooth_instance_->connection_state_changed_data().size());
+  const auto& connected_data =
+      fake_bluetooth_instance_->connection_state_changed_data().back();
+  EXPECT_EQ(std::string(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress),
+            connected_data->addr()->To<std::string>());
+  EXPECT_EQ(device::BLUETOOTH_TRANSPORT_LE, connected_data->device_type());
+  EXPECT_EQ(true, connected_data->connected());
+
+  // OnLEConnectionStateChange() should not be invoked.
+  ASSERT_EQ(0u,
+            fake_bluetooth_instance_->le_connection_state_change_data().size());
+
+  // Device is disconnected.
+  ChangeTestDeviceConnected(false);
+
+  // OnConnectionStateChanged() should be invoked.
+  ASSERT_EQ(2u,
+            fake_bluetooth_instance_->connection_state_changed_data().size());
+  const auto& disconnected_data =
+      fake_bluetooth_instance_->connection_state_changed_data().back();
+  EXPECT_EQ(std::string(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress),
+            disconnected_data->addr()->To<std::string>());
+  EXPECT_EQ(device::BLUETOOTH_TRANSPORT_LE, disconnected_data->device_type());
+  EXPECT_EQ(false, disconnected_data->connected());
+
+  // OnLEConnectionStateChange() should not be invoked.
+  ASSERT_EQ(0u,
+            fake_bluetooth_instance_->le_connection_state_change_data().size());
+}
+
 // Invoke GetGattDB and check correctness of the GattDB sent via arc bridge.
 TEST_F(ArcBluetoothBridgeTest, GetGattDB) {
   AddTestDevice();

components/arc/mojom/bluetooth.mojom

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Next MinVersion: 17
+// Next MinVersion: 18
 
 module arc.mojom;
 
@@ -443,7 +443,7 @@ interface BluetoothHost {
       => (BluetoothStatus status, RfcommConnectingSocketClient&? client);
 };
 
-// Next Method ID: 23
+// Next Method ID: 24
 // Deprecated Method ID: 2, 6, 11, 12
 interface BluetoothInstance {
   // DEPRECATED: Please use Init@18 instead.
@@ -462,6 +462,14 @@ interface BluetoothInstance {
                        BluetoothAddress remote_addr,
                        BluetoothBondState state);
 
+  // When the underlying BT link state changed, OnConnectionStateChanged() will
+  // always be invoked, while OnLEConnectionStateChange() will be invoked only
+  // when Chrome is holding a BluetoothGattConnection object for this address
+  // (i.e., this is a GATT connection which started by LEConnect() from ARC).
+  [MinVersion=17] OnConnectionStateChanged@23(BluetoothAddress remote_addr,
+                                              BluetoothDeviceType device_type,
+                                              bool connected);
+
   // Bluetooth Gatt Client callbacks
   // TODO(b/111367421): Remove this when no active device running Android N.
   [MinVersion=1] OnLEDeviceFoundForN@7(
@@ -471,6 +479,9 @@ interface BluetoothInstance {
   [MinVersion=13] OnLEDeviceFound@21(BluetoothAddress addr,
                                      int32 rssi,
                                      array<uint8> eir);
+  // Used to notfiy the state of BluetoothGattConnection held by Chrome, called
+  // when that object becomes connected / disconnected or the LE connect request
+  // from ARC failed.
   [MinVersion=1] OnLEConnectionStateChange@8(BluetoothAddress remote_addr,
                                              bool connected);
   [MinVersion=4] OnLEDeviceAddressChange@16(BluetoothAddress old_addr,

components/arc/test/fake_bluetooth_instance.cc

@@ -32,6 +32,25 @@ FakeBluetoothInstance::LEDeviceFoundData::LEDeviceFoundData(
 
 FakeBluetoothInstance::LEDeviceFoundData::~LEDeviceFoundData() {}
 
+FakeBluetoothInstance::ConnectionStateChangedData::ConnectionStateChangedData(
+    mojom::BluetoothAddressPtr addr,
+    device::BluetoothTransport device_type,
+    bool connected)
+    : addr_(std::move(addr)),
+      device_type_(device_type),
+      connected_(connected) {}
+
+FakeBluetoothInstance::ConnectionStateChangedData::
+    ~ConnectionStateChangedData() = default;
+
+FakeBluetoothInstance::LEConnectionStateChangeData::LEConnectionStateChangeData(
+    mojom::BluetoothAddressPtr addr,
+    bool connected)
+    : addr_(std::move(addr)), connected_(connected) {}
+
+FakeBluetoothInstance::LEConnectionStateChangeData::
+    ~LEConnectionStateChangeData() = default;
+
 void FakeBluetoothInstance::InitDeprecated(mojom::BluetoothHostPtr host_ptr) {
   Init(std::move(host_ptr), base::DoNothing());
 }
@@ -65,6 +84,15 @@ void FakeBluetoothInstance::OnBondStateChanged(
     mojom::BluetoothAddressPtr remote_addr,
     mojom::BluetoothBondState state) {}
 
+void FakeBluetoothInstance::OnConnectionStateChanged(
+    mojom::BluetoothAddressPtr remote_addr,
+    device::BluetoothTransport device_type,
+    bool connected) {
+  connection_state_changed_data_.push_back(
+      std::make_unique<ConnectionStateChangedData>(std::move(remote_addr),
+                                                   device_type, connected));
+}
+
 void FakeBluetoothInstance::OnLEDeviceFoundForN(
     mojom::BluetoothAddressPtr addr,
     int32_t rssi,
@@ -83,7 +111,11 @@ void FakeBluetoothInstance::OnLEDeviceFound(mojom::BluetoothAddressPtr addr,
 
 void FakeBluetoothInstance::OnLEConnectionStateChange(
     mojom::BluetoothAddressPtr remote_addr,
-    bool connected) {}
+    bool connected) {
+  le_connection_state_change_data_.push_back(
+      std::make_unique<LEConnectionStateChangeData>(std::move(remote_addr),
+                                                    connected));
+}
 
 void FakeBluetoothInstance::OnLEDeviceAddressChange(
     mojom::BluetoothAddressPtr old_addr,

components/arc/test/fake_bluetooth_instance.h

@@ -67,6 +67,41 @@ class FakeBluetoothInstance : public mojom::BluetoothInstance {
     DISALLOW_COPY_AND_ASSIGN(LEDeviceFoundData);
   };
 
+  class ConnectionStateChangedData {
+   public:
+    ConnectionStateChangedData(mojom::BluetoothAddressPtr addr,
+                               device::BluetoothTransport device_type,
+                               bool connected);
+    ~ConnectionStateChangedData();
+
+    const mojom::BluetoothAddressPtr& addr() const { return addr_; }
+    device::BluetoothTransport device_type() const { return device_type_; }
+    bool connected() const { return connected_; }
+
+   private:
+    mojom::BluetoothAddressPtr addr_;
+    device::BluetoothTransport device_type_;
+    bool connected_;
+
+    DISALLOW_COPY_AND_ASSIGN(ConnectionStateChangedData);
+  };
+
+  class LEConnectionStateChangeData {
+   public:
+    LEConnectionStateChangeData(mojom::BluetoothAddressPtr addr,
+                                bool connected);
+    ~LEConnectionStateChangeData();
+
+    const mojom::BluetoothAddressPtr& addr() const { return addr_; }
+    bool connected() const { return connected_; }
+
+   private:
+    mojom::BluetoothAddressPtr addr_;
+    bool connected_;
+
+    DISALLOW_COPY_AND_ASSIGN(LEConnectionStateChangeData);
+  };
+
   FakeBluetoothInstance();
   ~FakeBluetoothInstance() override;
 
@@ -85,6 +120,9 @@ class FakeBluetoothInstance : public mojom::BluetoothInstance {
   void OnBondStateChanged(mojom::BluetoothStatus status,
                           mojom::BluetoothAddressPtr remote_addr,
                           mojom::BluetoothBondState state) override;
+  void OnConnectionStateChanged(mojom::BluetoothAddressPtr remote_addr,
+                                device::BluetoothTransport device_type,
+                                bool connected) override;
   void OnLEDeviceFoundForN(
       mojom::BluetoothAddressPtr addr,
       int32_t rssi,
@@ -151,6 +189,16 @@ class FakeBluetoothInstance : public mojom::BluetoothInstance {
     return le_device_found_data_;
   }
 
+  const std::vector<std::unique_ptr<ConnectionStateChangedData>>&
+  connection_state_changed_data() const {
+    return connection_state_changed_data_;
+  }
+
+  const std::vector<std::unique_ptr<LEConnectionStateChangeData>>&
+  le_connection_state_change_data() {
+    return le_connection_state_change_data_;
+  }
+
   const std::vector<std::unique_ptr<GattDBResult>>& gatt_db_result() const {
     return gatt_db_result_;
   }
@@ -160,6 +208,10 @@ class FakeBluetoothInstance : public mojom::BluetoothInstance {
   std::vector<std::vector<mojom::BluetoothPropertyPtr>>
       device_properties_changed_data_;
   std::vector<std::unique_ptr<LEDeviceFoundData>> le_device_found_data_;
+  std::vector<std::unique_ptr<ConnectionStateChangedData>>
+      connection_state_changed_data_;
+  std::vector<std::unique_ptr<LEConnectionStateChangeData>>
+      le_connection_state_change_data_;
   std::vector<std::unique_ptr<GattDBResult>> gatt_db_result_;
 
   // Keeps the binding alive so that calls to this class can be correctly