[chromecast][Bluetooth] Fix connect/services logic

This allows WebBT to work if we were connected before or after the WebBT
application starts.

Remove BluetoothAdapterCast::OnServicesUpdated because Connect now
implies services are discovered. Dynamic service updating is also
deprecated:
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?rcl=21e8d8c8dc11cd4b8f77c47b7f37098ebde13d3a&l=142

BUG=internal b/79542125
TEST=device_unittests. Manual

Change-Id: I99ed5dc61942cb310ed76581648fdab6c31316a1
Reviewed-on: https://chromium-review.googlesource.com/1058605
Commit-Queue: Bailey Forrest <bcf@chromium.org>
Reviewed-by: Stephen Lanham <slan@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558898}

chromecast/device/bluetooth/le/le_scan_result.cc

@@ -58,9 +58,9 @@ base::Optional<LeScanResult::UuidList> GetUuidsAsUuid(
       return base::nullopt;
     }
 
-    ret.emplace_back();
     for (size_t i = 0; i < field.size();
          i += sizeof(bluetooth_v2_shlib::Uuid)) {
+      ret.emplace_back();
       // GAP UUIDs are little endian and bluetooth_v2_shlib::Uuid is big endian.
       std::reverse_copy(field.begin() + i,
                         field.begin() + i + sizeof(bluetooth_v2_shlib::Uuid),

chromecast/device/bluetooth/le/le_scan_result_test.cc

@@ -240,18 +240,26 @@ TEST(LeScanResultTest, CompleteListOf128BitServiceUuid) {
   static const bluetooth_v2_shlib::Uuid kUuid2 = {
       {0xa8, 0x22, 0xc8, 0x85, 0xaf, 0x02, 0xc7, 0x80, 0x9d, 0x4d, 0xbd, 0x9a,
        0x1f, 0xa0, 0x6d, 0x93}};
+  static const bluetooth_v2_shlib::Uuid kUuid3 = {
+      {0xaa, 0x22, 0xc8, 0x85, 0xaf, 0x02, 0xc7, 0x80, 0x9d, 0x4d, 0xbd, 0x9a,
+       0x1f, 0xa0, 0x6d, 0x93}};
 
   LeScanResult scan_result;
   scan_result.type_to_data[LeScanResult::kGapComplete128BitServiceUuids]
       .emplace_back(kUuid1.rbegin(), kUuid1.rend());
   scan_result.type_to_data[LeScanResult::kGapComplete128BitServiceUuids]
       .emplace_back(kUuid2.rbegin(), kUuid2.rend());
+  auto& end =
+      scan_result.type_to_data[LeScanResult::kGapComplete128BitServiceUuids]
+          .back();
+  end.insert(end.end(), kUuid3.rbegin(), kUuid3.rend());
 
   auto uuids = scan_result.CompleteListOf128BitServiceUuids();
   ASSERT_TRUE(uuids);
-  ASSERT_EQ(2ul, uuids->size());
+  ASSERT_EQ(3ul, uuids->size());
   EXPECT_EQ(kUuid1, (*uuids)[0]);
   EXPECT_EQ(kUuid2, (*uuids)[1]);
+  EXPECT_EQ(kUuid3, (*uuids)[2]);
 }
 
 TEST(LeScanResultTest, AllServiceData) {

chromecast/device/bluetooth/le/remote_device_impl.cc

@@ -288,6 +288,10 @@ void RemoteDeviceImpl::SetConnected(bool connected) {
       LOG(ERROR) << "Couldn't discover services, disconnecting";
       Disconnect({});
     }
+  } else {
+    uuid_to_service_.clear();
+    handle_to_characteristic_.clear();
+    handle_to_descriptor_.clear();
   }
 }
 

device/BUILD.gn

@@ -246,6 +246,7 @@ test("device_unittests") {
       deps += [
         "//chromecast/device/bluetooth:util",
         "//chromecast/device/bluetooth/le",
+        "//chromecast/device/bluetooth/le:test_support",
         "//chromecast/device/bluetooth/shlib:mock_shlib",
       ]
     } else {

device/bluetooth/DEPS

 include_rules = [
   "+chromecast/device/bluetooth",
+  "+chromecast/public/bluetooth",
   "+chromeos/dbus",
   "+crypto",
   "+dbus",

device/bluetooth/cast/bluetooth_adapter_cast.cc

@@ -300,24 +300,6 @@ void BluetoothAdapterCast::OnMtuChanged(
            << " mtu: " << mtu;
 }
 
-void BluetoothAdapterCast::OnServicesUpdated(
-    scoped_refptr<chromecast::bluetooth::RemoteDevice> device,
-    std::vector<scoped_refptr<chromecast::bluetooth::RemoteService>> services) {
-  DVLOG(1) << __func__;
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  std::string address = GetCanonicalBluetoothAddress(device->addr());
-  BluetoothDeviceCast* cast_device = GetCastDevice(address);
-  if (!cast_device)
-    return;
-
-  if (!cast_device->UpdateServices(services))
-    LOG(WARNING) << "The services were not updated. Alerting anyway.";
-
-  cast_device->SetGattServicesDiscoveryComplete(true);
-  NotifyGattServicesDiscovered(cast_device);
-}
-
 void BluetoothAdapterCast::OnCharacteristicNotification(
     scoped_refptr<chromecast::bluetooth::RemoteDevice> device,
     scoped_refptr<chromecast::bluetooth::RemoteCharacteristic> characteristic,
@@ -347,17 +329,19 @@ void BluetoothAdapterCast::OnNewScanResult(
   // to send an async request to |gatt_client_manager_| for a handle to the
   // device.
   if (devices_.find(address) == devices_.end()) {
+    bool first_time_seen =
+        pending_scan_results_.find(address) == pending_scan_results_.end();
+    // These results will be used to construct the BluetoothDeviceCast.
+    pending_scan_results_[address].push_back(result);
+
     // Only send a request if this is the first time we've seen this |address|
     // in a scan. This may happen if we pick up additional GAP advertisements
     // while the first request is in-flight.
-    if (pending_scan_results_.find(address) == pending_scan_results_.end()) {
+    if (first_time_seen) {
       gatt_client_manager_->GetDevice(
           result.addr, base::BindOnce(&BluetoothAdapterCast::OnGetDevice,
                                       weak_factory_.GetWeakPtr()));
     }
-
-    // These results will be used to construct the BluetoothDeviceCast.
-    pending_scan_results_[address].push_back(result);
     return;
   }
 

device/bluetooth/cast/bluetooth_adapter_cast.h

@@ -137,10 +137,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterCast
       bool connected) override;
   void OnMtuChanged(scoped_refptr<chromecast::bluetooth::RemoteDevice> device,
                     int mtu) override;
-  void OnServicesUpdated(
-      scoped_refptr<chromecast::bluetooth::RemoteDevice> device,
-      std::vector<scoped_refptr<chromecast::bluetooth::RemoteService>> services)
-      override;
   void OnCharacteristicNotification(
       scoped_refptr<chromecast::bluetooth::RemoteDevice> device,
       scoped_refptr<chromecast::bluetooth::RemoteCharacteristic> characteristic,

device/bluetooth/cast/bluetooth_device_cast.cc

@@ -14,6 +14,7 @@
 #include "chromecast/device/bluetooth/bluetooth_util.h"
 #include "chromecast/device/bluetooth/le/remote_characteristic.h"
 #include "chromecast/device/bluetooth/le/remote_service.h"
+#include "device/bluetooth/bluetooth_adapter.h"
 #include "device/bluetooth/cast/bluetooth_remote_gatt_characteristic_cast.h"
 #include "device/bluetooth/cast/bluetooth_remote_gatt_service_cast.h"
 #include "device/bluetooth/cast/bluetooth_utils.h"
@@ -60,7 +61,12 @@ BluetoothDeviceCast::BluetoothDeviceCast(
       connected_(device->IsConnected()),
       remote_device_(std::move(device)),
       address_(GetCanonicalBluetoothAddress(remote_device_->addr())),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  if (connected_) {
+    remote_device_->GetServices(base::BindOnce(
+        &BluetoothDeviceCast::OnGetServices, weak_factory_.GetWeakPtr()));
+  }
+}
 
 BluetoothDeviceCast::~BluetoothDeviceCast() {}
 
@@ -286,6 +292,8 @@ bool BluetoothDeviceCast::SetConnected(bool connected) {
   // fired.
   if (!was_connected && connected) {
     DidConnectGatt();
+    remote_device_->GetServices(base::BindOnce(
+        &BluetoothDeviceCast::OnGetServices, weak_factory_.GetWeakPtr()));
   } else if (was_connected && !connected) {
     DidDisconnectGatt();
   }
@@ -294,43 +302,23 @@ bool BluetoothDeviceCast::SetConnected(bool connected) {
   return was_connected != connected;
 }
 
-bool BluetoothDeviceCast::UpdateServices(
+void BluetoothDeviceCast::OnGetServices(
     std::vector<scoped_refptr<chromecast::bluetooth::RemoteService>> services) {
   DVLOG(2) << __func__;
-  bool changed = false;
-
-  // Create a look-up for the updated list of services.
-  std::unordered_set<std::string> new_service_uuids;
-  for (const auto& service : services)
-    new_service_uuids.insert(GetCanonicalBluetoothUuid(service->uuid()));
-
-  // Remove any services in |gatt_services_| that are not present in |services|.
-  for (auto it = gatt_services_.cbegin(); it != gatt_services_.cend();) {
-    if (new_service_uuids.find(it->first) == new_service_uuids.end()) {
-      gatt_services_.erase(it++);
-      changed = true;
-    } else {
-      ++it;
-    }
-  }
+  gatt_services_.clear();
 
   // Add new services.
   for (auto& service : services) {
     auto key = GetCanonicalBluetoothUuid(service->uuid());
-
-    if (gatt_services_.find(key) != gatt_services_.end())
-      continue;
-
     auto cast_service = std::make_unique<BluetoothRemoteGattServiceCast>(
         this, std::move(service));
     DCHECK_EQ(key, cast_service->GetIdentifier());
     gatt_services_[key] = std::move(cast_service);
-    changed = true;
   }
 
-  if (changed)
-    device_uuids_.ReplaceServiceUUIDs(gatt_services_);
-  return changed;
+  device_uuids_.ReplaceServiceUUIDs(gatt_services_);
+  SetGattServicesDiscoveryComplete(true);
+  adapter_->NotifyGattServicesDiscovered(this);
 }
 
 bool BluetoothDeviceCast::UpdateCharacteristicValue(
@@ -375,18 +363,14 @@ void BluetoothDeviceCast::DisconnectGatt() {
 void BluetoothDeviceCast::OnConnect(bool success) {
   DVLOG(2) << __func__ << " success:" << success;
   pending_connect_ = false;
-  if (success)
-    SetConnected(true);
-  else
+  if (!success)
     DidFailToConnectGatt(ERROR_FAILED);
 }
 
 void BluetoothDeviceCast::OnDisconnect(bool success) {
   DVLOG(2) << __func__ << " success:" << success;
   pending_disconnect_ = false;
-  if (success)
-    SetConnected(false);
-  else
+  if (!success)
     LOG(ERROR) << "Request to DisconnectGatt() failed!";
 }
 

device/bluetooth/cast/bluetooth_device_cast.h

@@ -95,13 +95,6 @@ class BluetoothDeviceCast : public BluetoothDevice {
   // connection state changed as a result.
   bool SetConnected(bool connected);
 
-  // Called by BluetoothAdapterCast when the GATT services for this device are
-  // updated. Updates the services in this devices to reflect |services|.
-  // Returns true if a service was added or removed.
-  bool UpdateServices(
-      std::vector<scoped_refptr<chromecast::bluetooth::RemoteService>>
-          services);
-
   // Called by BluetoothAdapterCast when the value of a characteristic in one of
   // this device's services has changed, resulting in a notification to the
   // device. Locate the characteristc and update the underluing value. If the
@@ -132,6 +125,11 @@ class BluetoothDeviceCast : public BluetoothDevice {
   // Called back from disconnect requests.
   void OnDisconnect(bool success);
 
+  // Called in response to GetServices
+  void OnGetServices(
+      std::vector<scoped_refptr<chromecast::bluetooth::RemoteService>>
+          services);
+
   bool connected_;
   bool pending_connect_ = false;
   bool pending_disconnect_ = false;

device/bluetooth/test/bluetooth_test_cast.cc

@@ -7,52 +7,46 @@
 #include "base/bind_helpers.h"
 #include "base/task_scheduler/post_task.h"
 #include "chromecast/device/bluetooth/bluetooth_util.h"
-#include "chromecast/device/bluetooth/le/gatt_client_manager_impl.h"
+#include "chromecast/device/bluetooth/le/mock_gatt_client_manager.h"
 #include "chromecast/device/bluetooth/le/remote_device.h"
 #include "device/bluetooth/cast/bluetooth_adapter_cast.h"
-#include "device/bluetooth/test/bluetooth_test_cast.h"
+
+using ::testing::Return;
 
 namespace device {
 
-class BluetoothTestCast::FakeLeScanManager
-    : public chromecast::bluetooth::LeScanManager {
+class BluetoothTestCast::GattClientManager
+    : public chromecast::bluetooth::MockGattClientManager {
  public:
-  FakeLeScanManager() = default;
-  ~FakeLeScanManager() override = default;
-
-  void AddObserver(Observer* o) override {
-    DCHECK(!observer_);
-    observer_ = o;
-  }
-  void RemoveObserver(Observer* o) override {
-    DCHECK_EQ(observer_, o);
-    observer_ = nullptr;
-  }
-  void SetScanEnable(bool enable, SetScanEnableCallback cb) override {
-    std::move(cb).Run(true);
-  }
-  void GetScanResults(GetScanResultsCallback cb,
-                      base::Optional<chromecast::bluetooth::ScanFilter>
-                          scan_filter = base::nullopt) override {}
-  void ClearScanResults() override {}
-
-  Observer* observer() {
-    DCHECK(observer_);
-    return observer_;
+  GattClientManager() = default;
+  ~GattClientManager() override = default;
+
+  // chromecast::bluetooth::GattClientManager implementation:
+  void GetDevice(
+      const chromecast::bluetooth_v2_shlib::Addr& addr,
+      base::OnceCallback<void(
+          scoped_refptr<chromecast::bluetooth::RemoteDevice>)> cb) override {
+    auto it = addr_to_remote_device_.find(addr);
+    if (it != addr_to_remote_device_.end()) {
+      std::move(cb).Run(it->second);
+      return;
+    }
+
+    auto device =
+        base::MakeRefCounted<chromecast::bluetooth::MockRemoteDevice>(addr);
+    addr_to_remote_device_.emplace(addr, device);
+    std::move(cb).Run(device);
   }
 
  private:
-  Observer* observer_ = nullptr;
+  std::map<chromecast::bluetooth_v2_shlib::Addr,
+           scoped_refptr<chromecast::bluetooth::MockRemoteDevice>>
+      addr_to_remote_device_;
 };
 
 BluetoothTestCast::BluetoothTestCast()
-    : io_task_runner_(base::CreateSingleThreadTaskRunnerWithTraits(
-          {base::MayBlock(), base::TaskPriority::BACKGROUND})),
-      gatt_client_manager_(
-          new chromecast::bluetooth::GattClientManagerImpl(&mock_gatt_client_)),
-      fake_le_scan_manager_(new FakeLeScanManager()) {
-  gatt_client_manager_->Initialize(io_task_runner_);
-  mock_gatt_client_.SetDelegate(gatt_client_manager_.get());
+    : gatt_client_manager_(std::make_unique<GattClientManager>()) {
+  ON_CALL(le_scan_manager_, SetScanEnable).WillByDefault(Return(true));
 }
 
 BluetoothTestCast::~BluetoothTestCast() {
@@ -62,12 +56,11 @@ BluetoothTestCast::~BluetoothTestCast() {
 
   // Tear down adapter, which relies on members in the subclass.
   adapter_ = nullptr;
-  gatt_client_manager_->Finalize();
 }
 
 void BluetoothTestCast::InitWithFakeAdapter() {
-  adapter_ = new BluetoothAdapterCast(gatt_client_manager_.get(),
-                                      fake_le_scan_manager_.get());
+  adapter_ =
+      new BluetoothAdapterCast(gatt_client_manager_.get(), &le_scan_manager_);
   adapter_->SetPowered(true, base::DoNothing(), base::DoNothing());
 }
 
@@ -124,7 +117,7 @@ void BluetoothTestCast::UpdateAdapter(
     const std::vector<std::string>& service_uuids,
     const std::map<std::string, std::vector<uint8_t>>& service_data,
     const std::map<uint16_t, std::vector<uint8_t>>& manufacturer_data) {
-  // Create a scan result with the desired address and name.
+  // Create a scan result with the desired values.
   chromecast::bluetooth::LeScanResult result;
   ASSERT_TRUE(chromecast::bluetooth::util::ParseAddr(address, &result.addr));
   if (name) {
@@ -132,6 +125,18 @@ void BluetoothTestCast::UpdateAdapter(
         .push_back(std::vector<uint8_t>(name->begin(), name->end()));
   }
 
+  // Add service_uuids.
+  std::vector<uint8_t> data;
+  for (const auto& uuid_str : service_uuids) {
+    chromecast::bluetooth_v2_shlib::Uuid uuid;
+    ASSERT_TRUE(chromecast::bluetooth::util::ParseUuid(uuid_str, &uuid));
+    data.insert(data.end(), uuid.rbegin(), uuid.rend());
+  }
+  result
+      .type_to_data
+          [chromecast::bluetooth::LeScanResult::kGapComplete128BitServiceUuids]
+      .push_back(std::move(data));
+
   // Add service data.
   for (const auto& it : service_data) {
     chromecast::bluetooth_v2_shlib::Uuid uuid;
@@ -154,17 +159,7 @@ void BluetoothTestCast::UpdateAdapter(
   }
 
   // Update the adapter with the ScanResult.
-  fake_le_scan_manager_->observer()->OnNewScanResult(result);
-  scoped_task_environment_.RunUntilIdle();
-
-  // Add the services.
-  std::vector<chromecast::bluetooth_v2_shlib::Gatt::Service> services;
-  for (const auto& uuid : service_uuids) {
-    chromecast::bluetooth_v2_shlib::Gatt::Service service;
-    ASSERT_TRUE(chromecast::bluetooth::util::ParseUuid(uuid, &service.uuid));
-    services.push_back(service);
-  }
-  mock_gatt_client_.delegate()->OnServicesAdded(result.addr, services);
+  le_scan_manager_.observer_->OnNewScanResult(result);
   scoped_task_environment_.RunUntilIdle();
 }
 

device/bluetooth/test/bluetooth_test_cast.h

@@ -6,23 +6,20 @@
 #define DEVICE_BLUETOOTH_TEST_BLUETOOTH_TEST_CAST_H_
 
 #include <cstdint>
+#include <map>
+#include <string>
 #include <vector>
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/optional.h"
-#include "chromecast/device/bluetooth/shlib/mock_gatt_client.h"
+#include "chromecast/device/bluetooth/le/mock_le_scan_manager.h"
+#include "chromecast/public/bluetooth/bluetooth_types.h"
 #include "device/bluetooth/bluetooth_device.h"
 #include "device/bluetooth/bluetooth_local_gatt_characteristic.h"
 #include "device/bluetooth/bluetooth_local_gatt_descriptor.h"
 #include "device/bluetooth/test/bluetooth_test.h"
 
-namespace chromecast {
-namespace bluetooth {
-class GattClientManagerImpl;
-}
-}  // namespace chromecast
-
 namespace device {
 
 // Cast implementation of BluetoothTestBase.
@@ -37,8 +34,7 @@ class BluetoothTestCast : public BluetoothTestBase {
   BluetoothDevice* SimulateLowEnergyDevice(int device_ordinal) override;
 
  private:
-  class FakeGattClientManager;
-  class FakeLeScanManager;
+  class GattClientManager;
 
   void UpdateAdapter(
       const std::string& address,
@@ -47,11 +43,8 @@ class BluetoothTestCast : public BluetoothTestBase {
       const std::map<std::string, std::vector<uint8_t>>& service_data,
       const std::map<uint16_t, std::vector<uint8_t>>& manufacturer_data);
 
-  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
-  chromecast::bluetooth_v2_shlib::MockGattClient mock_gatt_client_;
-  std::unique_ptr<chromecast::bluetooth::GattClientManagerImpl>
-      gatt_client_manager_;
-  std::unique_ptr<FakeLeScanManager> fake_le_scan_manager_;
+  const std::unique_ptr<GattClientManager> gatt_client_manager_;
+  ::chromecast::bluetooth::MockLeScanManager le_scan_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(BluetoothTestCast);
 };