[CrOS PhoneHub] Change BleScanner::Delegate to Observer

PhoneHub will be utilizing BleScanner to understand when a device is
in proximity before attempting a connection via Nearby Connections.
Thus, the BleScanner class must support multiple observers (one for the
existing use case used by Instant Tethering and Smart Lock), and one for
the Phone Hub case).

BleScanner previously had a Delegate pattern which allowed only one
listener for when BLE scans were picked up, so this CL converts this
code to an Observer pattern which supports the new use case.

For now, BleConnectionManagerImpl is still the only observer; a future
CL will add an additional one.

Bug: 1106937
Change-Id: I0dd3cea3d0f050d8caeb82784ed6193821ff657d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2393258
Auto-Submit: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805010}

chromeos/services/secure_channel/ble_connection_manager_impl.cc

@@ -215,14 +215,17 @@ BleConnectionManagerImpl::BleConnectionManagerImpl(
                                              ble_service_data_helper_,
                                              ble_synchronizer_.get(),
                                              timer_factory)),
-      ble_scanner_(BleScannerImpl::Factory::Create(this /* delegate */,
-                                                   ble_service_data_helper_,
+      ble_scanner_(BleScannerImpl::Factory::Create(ble_service_data_helper_,
                                                    ble_synchronizer_.get(),
                                                    bluetooth_adapter)),
       secure_channel_disconnector_(
-          SecureChannelDisconnectorImpl::Factory::Create()) {}
+          SecureChannelDisconnectorImpl::Factory::Create()) {
+  ble_scanner_->AddObserver(this);
+}
 
-BleConnectionManagerImpl::~BleConnectionManagerImpl() = default;
+BleConnectionManagerImpl::~BleConnectionManagerImpl() {
+  ble_scanner_->RemoveObserver(this);
+}
 
 void BleConnectionManagerImpl::PerformAttemptBleInitiatorConnection(
     const DeviceIdPair& device_id_pair,

chromeos/services/secure_channel/ble_connection_manager_impl.h

@@ -41,7 +41,7 @@ class TimerFactory;
 // this process is complete, an AuthenticatedChannel is returned to the client.
 class BleConnectionManagerImpl : public BleConnectionManager,
                                  public BleAdvertiser::Delegate,
-                                 public BleScanner::Delegate,
+                                 public BleScanner::Observer,
                                  public SecureChannel::Observer {
  public:
   class Factory {
@@ -131,7 +131,7 @@ class BleConnectionManagerImpl : public BleConnectionManager,
   void OnFailureToGenerateAdvertisement(
       const DeviceIdPair& device_id_pair) override;
 
-  // BleScanner::Delegate:
+  // BleScanner::Observer:
   void OnReceivedAdvertisement(multidevice::RemoteDeviceRef remote_device,
                                device::BluetoothDevice* bluetooth_device,
                                ConnectionRole connection_role) override;

chromeos/services/secure_channel/ble_connection_manager_impl_unittest.cc

@@ -149,7 +149,6 @@ class FakeBleScannerFactory : public BleScannerImpl::Factory {
  private:
   // BleScannerImpl::Factory:
   std::unique_ptr<BleScanner> CreateInstance(
-      BleScanner::Delegate* delegate,
       BleServiceDataHelper* service_data_helper,
       BleSynchronizerBase* ble_synchronizer_base,
       scoped_refptr<device::BluetoothAdapter> adapter) override {
@@ -159,7 +158,7 @@ class FakeBleScannerFactory : public BleScannerImpl::Factory {
     EXPECT_EQ(expected_mock_adapter_, adapter);
     EXPECT_FALSE(instance_);
 
-    auto instance = std::make_unique<FakeBleScanner>(delegate);
+    auto instance = std::make_unique<FakeBleScanner>();
     instance_ = instance.get();
     return instance;
   }

chromeos/services/secure_channel/ble_scanner.cc

@@ -13,7 +13,7 @@ namespace chromeos {
 
 namespace secure_channel {
 
-BleScanner::BleScanner(Delegate* delegate) : delegate_(delegate) {}
+BleScanner::BleScanner() = default;
 
 BleScanner::~BleScanner() = default;
 
@@ -43,6 +43,14 @@ bool BleScanner::HasScanFilter(const ScanFilter& scan_filter) {
   return base::Contains(scan_filters_, scan_filter);
 }
 
+void BleScanner::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void BleScanner::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
 DeviceIdPairSet BleScanner::GetAllDeviceIdPairs() {
   DeviceIdPairSet set;
   for (const auto& scan_filter : scan_filters_)
@@ -54,8 +62,10 @@ void BleScanner::NotifyReceivedAdvertisementFromDevice(
     const multidevice::RemoteDeviceRef& remote_device,
     device::BluetoothDevice* bluetooth_device,
     ConnectionRole connection_role) {
-  delegate_->OnReceivedAdvertisement(remote_device, bluetooth_device,
+  for (auto& observer : observer_list_) {
+    observer.OnReceivedAdvertisement(remote_device, bluetooth_device,
                                      connection_role);
+  }
 }
 
 std::ostream& operator<<(std::ostream& stream,

chromeos/services/secure_channel/ble_scanner.h

@@ -10,6 +10,8 @@
 
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
 #include "chromeos/components/multidevice/remote_device_ref.h"
 #include "chromeos/services/secure_channel/connection_role.h"
 #include "chromeos/services/secure_channel/device_id_pair.h"
@@ -26,9 +28,9 @@ namespace secure_channel {
 // received from a remote device.
 class BleScanner {
  public:
-  class Delegate {
+  class Observer : public base::CheckedObserver {
    public:
-    virtual ~Delegate() = default;
+    ~Observer() override = default;
     virtual void OnReceivedAdvertisement(
         multidevice::RemoteDeviceRef remote_device,
         device::BluetoothDevice* bluetooth_device,
@@ -51,8 +53,11 @@ class BleScanner {
 
   bool HasScanFilter(const ScanFilter& scan_filter);
 
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
  protected:
-  explicit BleScanner(Delegate* delegate);
+  BleScanner();
 
   virtual void HandleScanFilterChange() = 0;
 
@@ -66,7 +71,7 @@ class BleScanner {
       ConnectionRole connection_role);
 
  private:
-  Delegate* delegate_;
+  base::ObserverList<Observer> observer_list_;
 
   base::flat_set<ScanFilter> scan_filters_;
 

chromeos/services/secure_channel/ble_scanner_impl.cc

@@ -35,17 +35,16 @@ BleScannerImpl::Factory* BleScannerImpl::Factory::test_factory_ = nullptr;
 
 // static
 std::unique_ptr<BleScanner> BleScannerImpl::Factory::Create(
-    Delegate* delegate,
     BleServiceDataHelper* service_data_helper,
     BleSynchronizerBase* ble_synchronizer,
     scoped_refptr<device::BluetoothAdapter> adapter) {
   if (test_factory_) {
-    return test_factory_->CreateInstance(delegate, service_data_helper,
-                                         ble_synchronizer, adapter);
+    return test_factory_->CreateInstance(service_data_helper, ble_synchronizer,
+                                         adapter);
   }
 
-  return base::WrapUnique(new BleScannerImpl(delegate, service_data_helper,
-                                             ble_synchronizer, adapter));
+  return base::WrapUnique(
+      new BleScannerImpl(service_data_helper, ble_synchronizer, adapter));
 }
 
 // static
@@ -64,12 +63,10 @@ BleScannerImpl::ServiceDataProvider::ExtractProximityAuthServiceData(
       device::BluetoothUUID(kAdvertisingServiceUuid));
 }
 
-BleScannerImpl::BleScannerImpl(Delegate* delegate,
-                               BleServiceDataHelper* service_data_helper,
+BleScannerImpl::BleScannerImpl(BleServiceDataHelper* service_data_helper,
                                BleSynchronizerBase* ble_synchronizer,
                                scoped_refptr<device::BluetoothAdapter> adapter)
-    : BleScanner(delegate),
-      service_data_helper_(service_data_helper),
+    : service_data_helper_(service_data_helper),
       ble_synchronizer_(ble_synchronizer),
       adapter_(adapter),
       service_data_provider_(std::make_unique<ServiceDataProvider>()) {

chromeos/services/secure_channel/ble_scanner_impl.h

@@ -34,7 +34,6 @@ class BleScannerImpl : public BleScanner,
   class Factory {
    public:
     static std::unique_ptr<BleScanner> Create(
-        Delegate* delegate,
         BleServiceDataHelper* service_data_helper,
         BleSynchronizerBase* ble_synchronizer,
         scoped_refptr<device::BluetoothAdapter> adapter);
@@ -43,7 +42,6 @@ class BleScannerImpl : public BleScanner,
    protected:
     virtual ~Factory();
     virtual std::unique_ptr<BleScanner> CreateInstance(
-        Delegate* delegate,
         BleServiceDataHelper* service_data_helper,
         BleSynchronizerBase* ble_synchronizer,
         scoped_refptr<device::BluetoothAdapter> adapter) = 0;
@@ -67,8 +65,7 @@ class BleScannerImpl : public BleScanner,
         device::BluetoothDevice* bluetooth_device);
   };
 
-  BleScannerImpl(Delegate* delegate,
-                 BleServiceDataHelper* service_data_helper,
+  BleScannerImpl(BleServiceDataHelper* service_data_helper,
                  BleSynchronizerBase* ble_synchronizer,
                  scoped_refptr<device::BluetoothAdapter> adapter);
 

chromeos/services/secure_channel/ble_scanner_impl_unittest.cc

@@ -82,7 +82,7 @@ class SecureChannelBleScannerImplTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
-    fake_delegate_ = std::make_unique<FakeBleScannerDelegate>();
+    fake_delegate_ = std::make_unique<FakeBleScannerObserver>();
     fake_ble_service_data_helper_ =
         std::make_unique<FakeBleServiceDataHelper>();
     fake_ble_synchronizer_ = std::make_unique<FakeBleSynchronizer>();
@@ -91,8 +91,9 @@ class SecureChannelBleScannerImplTest : public testing::Test {
         base::MakeRefCounted<testing::NiceMock<device::MockBluetoothAdapter>>();
 
     ble_scanner_ = BleScannerImpl::Factory::Create(
-        fake_delegate_.get(), fake_ble_service_data_helper_.get(),
-        fake_ble_synchronizer_.get(), mock_adapter_);
+        fake_ble_service_data_helper_.get(), fake_ble_synchronizer_.get(),
+        mock_adapter_);
+    ble_scanner_->AddObserver(fake_delegate_.get());
 
     auto fake_service_data_provider =
         std::make_unique<FakeServiceDataProvider>();
@@ -122,6 +123,10 @@ class SecureChannelBleScannerImplTest : public testing::Test {
             }));
   }
 
+  void TearDown() override {
+    ble_scanner_->RemoveObserver(fake_delegate_.get());
+  }
+
   void AddScanFilter(const BleScanner::ScanFilter& scan_filter) {
     EXPECT_FALSE(ble_scanner_->HasScanFilter(scan_filter));
     ble_scanner_->AddScanFilter(scan_filter);
@@ -149,7 +154,7 @@ class SecureChannelBleScannerImplTest : public testing::Test {
 
   void ProcessScanResultAndVerifyNoDeviceIdentified(
       const std::string& service_data) {
-    const FakeBleScannerDelegate::ScannedResultList& results =
+    const FakeBleScannerObserver::ScannedResultList& results =
         fake_delegate_->handled_scan_results();
 
     size_t num_results_before_call = results.size();
@@ -161,7 +166,7 @@ class SecureChannelBleScannerImplTest : public testing::Test {
       const std::string& service_data,
       multidevice::RemoteDeviceRef expected_remote_device,
       bool is_background_advertisement) {
-    const FakeBleScannerDelegate::ScannedResultList& results =
+    const FakeBleScannerObserver::ScannedResultList& results =
         fake_delegate_->handled_scan_results();
 
     fake_ble_service_data_helper_->SetIdentifiedDevice(
@@ -242,7 +247,7 @@ class SecureChannelBleScannerImplTest : public testing::Test {
 
   const multidevice::RemoteDeviceRefList test_devices_;
 
-  std::unique_ptr<FakeBleScannerDelegate> fake_delegate_;
+  std::unique_ptr<FakeBleScannerObserver> fake_delegate_;
   std::unique_ptr<FakeBleServiceDataHelper> fake_ble_service_data_helper_;
   std::unique_ptr<FakeBleSynchronizer> fake_ble_synchronizer_;
   scoped_refptr<testing::NiceMock<device::MockBluetoothAdapter>> mock_adapter_;

chromeos/services/secure_channel/fake_ble_scanner.cc

@@ -8,7 +8,7 @@ namespace chromeos {
 
 namespace secure_channel {
 
-FakeBleScanner::FakeBleScanner(Delegate* delegate) : BleScanner(delegate) {}
+FakeBleScanner::FakeBleScanner() = default;
 
 FakeBleScanner::~FakeBleScanner() = default;
 
@@ -27,11 +27,11 @@ void FakeBleScanner::HandleScanFilterChange() {
   ++num_scan_filter_changes_handled_;
 }
 
-FakeBleScannerDelegate::FakeBleScannerDelegate() = default;
+FakeBleScannerObserver::FakeBleScannerObserver() = default;
 
-FakeBleScannerDelegate::~FakeBleScannerDelegate() = default;
+FakeBleScannerObserver::~FakeBleScannerObserver() = default;
 
-void FakeBleScannerDelegate::OnReceivedAdvertisement(
+void FakeBleScannerObserver::OnReceivedAdvertisement(
     multidevice::RemoteDeviceRef remote_device,
     device::BluetoothDevice* bluetooth_device,
     ConnectionRole connection_role) {

chromeos/services/secure_channel/fake_ble_scanner.h

@@ -19,7 +19,7 @@ namespace secure_channel {
 // Test BleScanner implementation.
 class FakeBleScanner : public BleScanner {
  public:
-  explicit FakeBleScanner(Delegate* delegate);
+  FakeBleScanner();
   ~FakeBleScanner() override;
 
   size_t num_scan_filter_changes_handled() const {
@@ -41,11 +41,11 @@ class FakeBleScanner : public BleScanner {
   DISALLOW_COPY_AND_ASSIGN(FakeBleScanner);
 };
 
-// Test BleScanner::Delegate implementation.
-class FakeBleScannerDelegate : public BleScanner::Delegate {
+// Test BleScanner::Observer implementation.
+class FakeBleScannerObserver : public BleScanner::Observer {
  public:
-  FakeBleScannerDelegate();
-  ~FakeBleScannerDelegate() override;
+  FakeBleScannerObserver();
+  ~FakeBleScannerObserver() override;
 
   using ScannedResultList = std::vector<std::tuple<multidevice::RemoteDeviceRef,
                                                    device::BluetoothDevice*,
@@ -62,7 +62,7 @@ class FakeBleScannerDelegate : public BleScanner::Delegate {
 
   ScannedResultList handled_scan_results_;
 
-  DISALLOW_COPY_AND_ASSIGN(FakeBleScannerDelegate);
+  DISALLOW_COPY_AND_ASSIGN(FakeBleScannerObserver);
 };
 
 }  // namespace secure_channel