[CrOS PhoneHub] Refactor SecureChannelImpl initialization

Now that BleScanner (and its dependency BleSynchronizer) will be shared
with Nearby, these classes cannot be initialized within
BleConnectionManagerImpl, since this class only supports connections via
BLE GATT connections. This CL moves their initialization to the top-
level SecureChannelImpl class.

Bug: 1106937
Change-Id: Ic4581ebacdc346441109420bb3778bfd300e7ab2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2402166
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@{#805744}

chromeos/services/secure_channel/ble_connection_manager_impl.cc

@@ -16,7 +16,6 @@
 #include "chromeos/services/secure_channel/ble_initiator_failure_type.h"
 #include "chromeos/services/secure_channel/ble_listener_failure_type.h"
 #include "chromeos/services/secure_channel/ble_scanner_impl.h"
-#include "chromeos/services/secure_channel/ble_synchronizer.h"
 #include "chromeos/services/secure_channel/ble_weave_client_connection.h"
 #include "chromeos/services/secure_channel/latency_metrics_logger.h"
 #include "chromeos/services/secure_channel/public/mojom/secure_channel.mojom.h"
@@ -59,15 +58,19 @@ BleConnectionManagerImpl::Factory*
 std::unique_ptr<BleConnectionManager> BleConnectionManagerImpl::Factory::Create(
     scoped_refptr<device::BluetoothAdapter> bluetooth_adapter,
     BleServiceDataHelper* ble_service_data_helper,
+    BleSynchronizerBase* ble_synchronizer,
+    BleScanner* ble_scanner,
     TimerFactory* timer_factory,
     base::Clock* clock) {
   if (test_factory_) {
     return test_factory_->CreateInstance(
-        bluetooth_adapter, ble_service_data_helper, timer_factory, clock);
+        bluetooth_adapter, ble_service_data_helper, ble_synchronizer,
+        ble_scanner, timer_factory, clock);
   }
 
   return base::WrapUnique(new BleConnectionManagerImpl(
-      bluetooth_adapter, ble_service_data_helper, timer_factory, clock));
+      bluetooth_adapter, ble_service_data_helper, ble_synchronizer, ble_scanner,
+      timer_factory, clock));
 }
 
 // static
@@ -204,20 +207,18 @@ void BleConnectionManagerImpl::ConnectionAttemptTimestamps::
 BleConnectionManagerImpl::BleConnectionManagerImpl(
     scoped_refptr<device::BluetoothAdapter> bluetooth_adapter,
     BleServiceDataHelper* ble_service_data_helper,
+    BleSynchronizerBase* ble_synchronizer,
+    BleScanner* ble_scanner,
     TimerFactory* timer_factory,
     base::Clock* clock)
     : bluetooth_adapter_(bluetooth_adapter),
-      ble_service_data_helper_(ble_service_data_helper),
       clock_(clock),
-      ble_synchronizer_(BleSynchronizer::Factory::Create(bluetooth_adapter)),
+      ble_scanner_(ble_scanner),
       ble_advertiser_(
           BleAdvertiserImpl::Factory::Create(this /* delegate */,
-                                             ble_service_data_helper_,
-                                             ble_synchronizer_.get(),
+                                             ble_service_data_helper,
+                                             ble_synchronizer,
                                              timer_factory)),
-      ble_scanner_(BleScannerImpl::Factory::Create(ble_service_data_helper_,
-                                                   ble_synchronizer_.get(),
-                                                   bluetooth_adapter)),
       secure_channel_disconnector_(
           SecureChannelDisconnectorImpl::Factory::Create()) {
   ble_scanner_->AddObserver(this);

chromeos/services/secure_channel/ble_connection_manager_impl.h

@@ -49,6 +49,8 @@ class BleConnectionManagerImpl : public BleConnectionManager,
     static std::unique_ptr<BleConnectionManager> Create(
         scoped_refptr<device::BluetoothAdapter> bluetooth_adapter,
         BleServiceDataHelper* ble_service_data_helper,
+        BleSynchronizerBase* ble_synchronizer,
+        BleScanner* ble_scanner,
         TimerFactory* timer_factory,
         base::Clock* clock = base::DefaultClock::GetInstance());
     static void SetFactoryForTesting(Factory* test_factory);
@@ -58,6 +60,8 @@ class BleConnectionManagerImpl : public BleConnectionManager,
     virtual std::unique_ptr<BleConnectionManager> CreateInstance(
         scoped_refptr<device::BluetoothAdapter> bluetooth_adapter,
         BleServiceDataHelper* ble_service_data_helper,
+        BleSynchronizerBase* ble_synchronizer,
+        BleScanner* ble_scanner,
         TimerFactory* timer_factory,
         base::Clock* clock = base::DefaultClock::GetInstance()) = 0;
 
@@ -103,6 +107,8 @@ class BleConnectionManagerImpl : public BleConnectionManager,
   BleConnectionManagerImpl(
       scoped_refptr<device::BluetoothAdapter> bluetooth_adapter,
       BleServiceDataHelper* ble_service_data_helper,
+      BleSynchronizerBase* ble_synchronizer,
+      BleScanner* ble_scanner,
       TimerFactory* timer_factory,
       base::Clock* clock);
 
@@ -197,12 +203,10 @@ class BleConnectionManagerImpl : public BleConnectionManager,
       const std::string& remote_device_id);
 
   scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
-  BleServiceDataHelper* ble_service_data_helper_;
   base::Clock* clock_;
+  BleScanner* ble_scanner_;
 
-  std::unique_ptr<BleSynchronizerBase> ble_synchronizer_;
   std::unique_ptr<BleAdvertiser> ble_advertiser_;
-  std::unique_ptr<BleScanner> ble_scanner_;
   std::unique_ptr<SecureChannelDisconnector> secure_channel_disconnector_;
 
   using SecureChannelWithRole =

chromeos/services/secure_channel/secure_channel_impl.cc

@@ -13,7 +13,9 @@
 #include "chromeos/services/secure_channel/active_connection_manager_impl.h"
 #include "chromeos/services/secure_channel/authenticated_channel.h"
 #include "chromeos/services/secure_channel/ble_connection_manager_impl.h"
+#include "chromeos/services/secure_channel/ble_scanner_impl.h"
 #include "chromeos/services/secure_channel/ble_service_data_helper_impl.h"
+#include "chromeos/services/secure_channel/ble_synchronizer.h"
 #include "chromeos/services/secure_channel/client_connection_parameters_impl.h"
 #include "chromeos/services/secure_channel/device_id_pair.h"
 #include "chromeos/services/secure_channel/pending_connection_manager_impl.h"
@@ -68,9 +70,16 @@ SecureChannelImpl::SecureChannelImpl(
       remote_device_cache_(multidevice::RemoteDeviceCache::Factory::Create()),
       ble_service_data_helper_(BleServiceDataHelperImpl::Factory::Create(
           remote_device_cache_.get())),
+      ble_synchronizer_(BleSynchronizer::Factory::Create(bluetooth_adapter_)),
+      ble_scanner_(
+          BleScannerImpl::Factory::Create(ble_service_data_helper_.get(),
+                                          ble_synchronizer_.get(),
+                                          bluetooth_adapter_)),
       ble_connection_manager_(BleConnectionManagerImpl::Factory::Create(
           bluetooth_adapter_,
           ble_service_data_helper_.get(),
+          ble_synchronizer_.get(),
+          ble_scanner_.get(),
           timer_factory_.get())),
       pending_connection_manager_(PendingConnectionManagerImpl::Factory::Create(
           this /* delegate */,

chromeos/services/secure_channel/secure_channel_impl.h

@@ -29,7 +29,9 @@ namespace chromeos {
 namespace secure_channel {
 
 class BleConnectionManager;
+class BleScanner;
 class BleServiceDataHelper;
+class BleSynchronizerBase;
 class TimerFactory;
 
 // Concrete SecureChannelImpl implementation, which contains three pieces:
@@ -173,6 +175,8 @@ class SecureChannelImpl : public mojom::SecureChannel,
   std::unique_ptr<TimerFactory> timer_factory_;
   std::unique_ptr<multidevice::RemoteDeviceCache> remote_device_cache_;
   std::unique_ptr<BleServiceDataHelper> ble_service_data_helper_;
+  std::unique_ptr<BleSynchronizerBase> ble_synchronizer_;
+  std::unique_ptr<BleScanner> ble_scanner_;
   std::unique_ptr<BleConnectionManager> ble_connection_manager_;
   std::unique_ptr<PendingConnectionManager> pending_connection_manager_;
   std::unique_ptr<ActiveConnectionManager> active_connection_manager_;

chromeos/services/secure_channel/secure_channel_service_unittest.cc

@@ -16,12 +16,16 @@
 #include "chromeos/components/multidevice/remote_device_test_util.h"
 #include "chromeos/services/secure_channel/active_connection_manager_impl.h"
 #include "chromeos/services/secure_channel/ble_connection_manager_impl.h"
+#include "chromeos/services/secure_channel/ble_scanner_impl.h"
 #include "chromeos/services/secure_channel/ble_service_data_helper_impl.h"
+#include "chromeos/services/secure_channel/ble_synchronizer.h"
 #include "chromeos/services/secure_channel/client_connection_parameters_impl.h"
 #include "chromeos/services/secure_channel/fake_active_connection_manager.h"
 #include "chromeos/services/secure_channel/fake_authenticated_channel.h"
 #include "chromeos/services/secure_channel/fake_ble_connection_manager.h"
+#include "chromeos/services/secure_channel/fake_ble_scanner.h"
 #include "chromeos/services/secure_channel/fake_ble_service_data_helper.h"
+#include "chromeos/services/secure_channel/fake_ble_synchronizer.h"
 #include "chromeos/services/secure_channel/fake_client_connection_parameters.h"
 #include "chromeos/services/secure_channel/fake_connection_delegate.h"
 #include "chromeos/services/secure_channel/fake_pending_connection_manager.h"
@@ -123,16 +127,81 @@ class FakeBleServiceDataHelperFactory
   DISALLOW_COPY_AND_ASSIGN(FakeBleServiceDataHelperFactory);
 };
 
+class FakeBleSynchronizerFactory : public BleSynchronizer::Factory {
+ public:
+  FakeBleSynchronizerFactory() = default;
+  ~FakeBleSynchronizerFactory() override = default;
+
+  FakeBleSynchronizer* instance() { return instance_; }
+
+ private:
+  // BleSynchronizer::Factory:
+  std::unique_ptr<BleSynchronizerBase> CreateInstance(
+      scoped_refptr<device::BluetoothAdapter> bluetooth_adapter) override {
+    EXPECT_FALSE(instance_);
+
+    auto instance = std::make_unique<FakeBleSynchronizer>();
+    instance_ = instance.get();
+    return instance;
+  }
+
+  FakeBleSynchronizer* instance_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeBleSynchronizerFactory);
+};
+
+class FakeBleScannerFactory : public BleScannerImpl::Factory {
+ public:
+  FakeBleScannerFactory(
+      FakeBleServiceDataHelperFactory* fake_ble_service_data_helper_factory,
+      FakeBleSynchronizerFactory* fake_ble_synchronizer_factory)
+      : fake_ble_service_data_helper_factory_(
+            fake_ble_service_data_helper_factory),
+        fake_ble_synchronizer_factory_(fake_ble_synchronizer_factory) {}
+
+  ~FakeBleScannerFactory() override = default;
+
+  FakeBleScanner* instance() { return instance_; }
+
+ private:
+  // BleScannerImpl::Factory:
+  std::unique_ptr<BleScanner> CreateInstance(
+      BleServiceDataHelper* service_data_helper,
+      BleSynchronizerBase* ble_synchronizer_base,
+      scoped_refptr<device::BluetoothAdapter> adapter) override {
+    EXPECT_EQ(fake_ble_service_data_helper_factory_->instance(),
+              service_data_helper);
+    EXPECT_EQ(fake_ble_synchronizer_factory_->instance(),
+              ble_synchronizer_base);
+    EXPECT_FALSE(instance_);
+
+    auto instance = std::make_unique<FakeBleScanner>();
+    instance_ = instance.get();
+    return instance;
+  }
+
+  FakeBleScanner* instance_ = nullptr;
+
+  FakeBleServiceDataHelperFactory* fake_ble_service_data_helper_factory_;
+  FakeBleSynchronizerFactory* fake_ble_synchronizer_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeBleScannerFactory);
+};
+
 class FakeBleConnectionManagerFactory
     : public BleConnectionManagerImpl::Factory {
  public:
   FakeBleConnectionManagerFactory(
       device::BluetoothAdapter* expected_bluetooth_adapter,
       FakeBleServiceDataHelperFactory* fake_ble_service_data_helper_factory,
+      FakeBleSynchronizerFactory* fake_ble_synchronizer_factory,
+      FakeBleScannerFactory* fake_ble_scanner_factory,
       FakeTimerFactoryFactory* fake_timer_factory_factory)
       : expected_bluetooth_adapter_(expected_bluetooth_adapter),
         fake_ble_service_data_helper_factory_(
             fake_ble_service_data_helper_factory),
+        fake_ble_synchronizer_factory_(fake_ble_synchronizer_factory),
+        fake_ble_scanner_factory_(fake_ble_scanner_factory),
         fake_timer_factory_factory_(fake_timer_factory_factory) {}
 
   ~FakeBleConnectionManagerFactory() override = default;
@@ -144,12 +213,16 @@ class FakeBleConnectionManagerFactory
   std::unique_ptr<BleConnectionManager> CreateInstance(
       scoped_refptr<device::BluetoothAdapter> bluetooth_adapter,
       BleServiceDataHelper* ble_service_data_helper,
+      BleSynchronizerBase* ble_synchronizer,
+      BleScanner* ble_scanner,
       TimerFactory* timer_factory,
       base::Clock* clock) override {
     EXPECT_FALSE(instance_);
     EXPECT_EQ(expected_bluetooth_adapter_, bluetooth_adapter.get());
     EXPECT_EQ(fake_ble_service_data_helper_factory_->instance(),
               ble_service_data_helper);
+    EXPECT_EQ(fake_ble_synchronizer_factory_->instance(), ble_synchronizer);
+    EXPECT_EQ(fake_ble_scanner_factory_->instance(), ble_scanner);
     EXPECT_EQ(fake_timer_factory_factory_->instance(), timer_factory);
 
     auto instance = std::make_unique<FakeBleConnectionManager>();
@@ -159,6 +232,8 @@ class FakeBleConnectionManagerFactory
 
   device::BluetoothAdapter* expected_bluetooth_adapter_;
   FakeBleServiceDataHelperFactory* fake_ble_service_data_helper_factory_;
+  FakeBleSynchronizerFactory* fake_ble_synchronizer_factory_;
+  FakeBleScannerFactory* fake_ble_scanner_factory_;
   FakeTimerFactoryFactory* fake_timer_factory_factory_;
 
   FakeBleConnectionManager* instance_ = nullptr;
@@ -358,10 +433,22 @@ class SecureChannelServiceTest : public testing::Test {
     BleServiceDataHelperImpl::Factory::SetFactoryForTesting(
         fake_ble_service_data_helper_factory_.get());
 
+    fake_ble_synchronizer_factory_ =
+        std::make_unique<FakeBleSynchronizerFactory>();
+    BleSynchronizer::Factory::SetFactoryForTesting(
+        fake_ble_synchronizer_factory_.get());
+
+    fake_ble_scanner_factory_ = std::make_unique<FakeBleScannerFactory>(
+        fake_ble_service_data_helper_factory_.get(),
+        fake_ble_synchronizer_factory_.get());
+    BleScannerImpl::Factory::SetFactoryForTesting(
+        fake_ble_scanner_factory_.get());
+
     fake_ble_connection_manager_factory_ =
         std::make_unique<FakeBleConnectionManagerFactory>(
             mock_adapter_.get(), fake_ble_service_data_helper_factory_.get(),
-            fake_timer_factory_factory_.get());
+            fake_ble_synchronizer_factory_.get(),
+            fake_ble_scanner_factory_.get(), fake_timer_factory_factory_.get());
     BleConnectionManagerImpl::Factory::SetFactoryForTesting(
         fake_ble_connection_manager_factory_.get());
 
@@ -396,6 +483,8 @@ class SecureChannelServiceTest : public testing::Test {
     TimerFactoryImpl::Factory::SetFactoryForTesting(nullptr);
     multidevice::RemoteDeviceCache::Factory::SetFactoryForTesting(nullptr);
     BleServiceDataHelperImpl::Factory::SetFactoryForTesting(nullptr);
+    BleSynchronizer::Factory::SetFactoryForTesting(nullptr);
+    BleScannerImpl::Factory::SetFactoryForTesting(nullptr);
     BleConnectionManagerImpl::Factory::SetFactoryForTesting(nullptr);
     PendingConnectionManagerImpl::Factory::SetFactoryForTesting(nullptr);
     ActiveConnectionManagerImpl::Factory::SetFactoryForTesting(nullptr);
@@ -870,6 +959,8 @@ class SecureChannelServiceTest : public testing::Test {
       test_remote_device_cache_factory_;
   std::unique_ptr<FakeBleServiceDataHelperFactory>
       fake_ble_service_data_helper_factory_;
+  std::unique_ptr<FakeBleSynchronizerFactory> fake_ble_synchronizer_factory_;
+  std::unique_ptr<FakeBleScannerFactory> fake_ble_scanner_factory_;
   std::unique_ptr<FakeBleConnectionManagerFactory>
       fake_ble_connection_manager_factory_;
   std::unique_ptr<FakePendingConnectionManagerFactory>