[MultideviceStubs] Refactor stubbed device sync and use in Linux CrOS

Refactors the stub device sync to fix a conflict with the fake device
sync (used in testing). Renames SetForTesting method to reflect it being
used in non-testing code. Enables the use of stub device sync when in
Linux CrOS, i.e. while running on emulator.

Fixed: 1085459
Change-Id: Ib075405e39d323a9e04954f0dfb7a6259136da87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2406855
Commit-Queue: Daniel Classon <dclasson@google.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809565}

chrome/browser/chromeos/device_sync/device_sync_client_factory.cc

@@ -13,9 +13,11 @@
 #include "chrome/browser/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
+#include "chromeos/components/multidevice/stub_multidevice_util.h"
 #include "chromeos/services/device_sync/device_sync_impl.h"
 #include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "chromeos/services/device_sync/public/cpp/device_sync_client_impl.h"
+#include "chromeos/services/device_sync/stub_device_sync.h"
 #include "chromeos/services/multidevice_setup/public/cpp/prefs.h"
 #include "components/gcm_driver/gcm_profile_service.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -85,6 +87,14 @@ DeviceSyncClientFactory::DeviceSyncClientFactory()
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(IdentityManagerFactory::GetInstance());
   DependsOn(gcm::GCMProfileServiceFactory::GetInstance());
+
+  // If ShouldUseMultideviceStubs() is true, set a stub factory to facilitate
+  // fake devices for testing in the Linux Chrome OS build. Note that this is
+  // not done when a custom factory has already been set.
+  if (multidevice::ShouldUseMultideviceStubs() &&
+      !DeviceSyncImpl::Factory::IsCustomFactorySet()) {
+    SetStubDeviceSyncFactory();
+  }
 }
 
 DeviceSyncClientFactory::~DeviceSyncClientFactory() {}

chrome/test/base/in_process_browser_test.cc

@@ -301,7 +301,7 @@ void InProcessBrowserTest::SetUp() {
       chrome_browser_net::NetErrorTabHelper::TESTING_FORCE_DISABLED);
 
 #if defined(OS_CHROMEOS)
-  chromeos::device_sync::DeviceSyncImpl::Factory::SetFactoryForTesting(
+  chromeos::device_sync::DeviceSyncImpl::Factory::SetCustomFactory(
       GetFakeDeviceSyncImplFactory());
 
   // On Chrome OS, access to files via file: scheme is restricted. Enable
@@ -353,7 +353,7 @@ void InProcessBrowserTest::TearDown() {
 #endif
 
 #if defined(OS_CHROMEOS)
-  chromeos::device_sync::DeviceSyncImpl::Factory::SetFactoryForTesting(nullptr);
+  chromeos::device_sync::DeviceSyncImpl::Factory::SetCustomFactory(nullptr);
 #endif
 }
 

chromeos/services/device_sync/device_sync_impl.cc

@@ -240,7 +240,7 @@ void RecordForceSyncNowResult(ForceCryptAuthOperationResult result) {
 }  // namespace
 
 // static
-DeviceSyncImpl::Factory* DeviceSyncImpl::Factory::test_factory_instance_ =
+DeviceSyncImpl::Factory* DeviceSyncImpl::Factory::custom_factory_instance_ =
     nullptr;
 
 // static
@@ -252,8 +252,8 @@ std::unique_ptr<DeviceSyncBase> DeviceSyncImpl::Factory::Create(
     ClientAppMetadataProvider* client_app_metadata_provider,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     std::unique_ptr<base::OneShotTimer> timer) {
-  if (test_factory_instance_) {
-    return test_factory_instance_->CreateInstance(
+  if (custom_factory_instance_) {
+    return custom_factory_instance_->CreateInstance(
         identity_manager, gcm_driver, profile_prefs, gcm_device_info_provider,
         client_app_metadata_provider, std::move(url_loader_factory),
         std::move(timer));
@@ -266,8 +266,13 @@ std::unique_ptr<DeviceSyncBase> DeviceSyncImpl::Factory::Create(
 }
 
 // static
-void DeviceSyncImpl::Factory::SetFactoryForTesting(Factory* test_factory) {
-  test_factory_instance_ = test_factory;
+void DeviceSyncImpl::Factory::SetCustomFactory(Factory* custom_factory) {
+  custom_factory_instance_ = custom_factory;
+}
+
+// static
+bool DeviceSyncImpl::Factory::IsCustomFactorySet() {
+  return custom_factory_instance_ != nullptr;
 }
 
 DeviceSyncImpl::Factory::~Factory() = default;

chromeos/services/device_sync/device_sync_impl.h

@@ -84,7 +84,8 @@ class DeviceSyncImpl : public DeviceSyncBase,
         ClientAppMetadataProvider* client_app_metadata_provider,
         scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
         std::unique_ptr<base::OneShotTimer> timer);
-    static void SetFactoryForTesting(Factory* test_factory);
+    static void SetCustomFactory(Factory* custom_factory);
+    static bool IsCustomFactorySet();
 
    protected:
     virtual ~Factory();
@@ -98,7 +99,7 @@ class DeviceSyncImpl : public DeviceSyncBase,
         std::unique_ptr<base::OneShotTimer> timer) = 0;
 
    private:
-    static Factory* test_factory_instance_;
+    static Factory* custom_factory_instance_;
   };
 
   ~DeviceSyncImpl() override;

chromeos/services/device_sync/device_sync_service_unittest.cc

@@ -855,7 +855,7 @@ class DeviceSyncServiceTest
     fake_device_sync_impl_factory_ =
         std::make_unique<FakeDeviceSyncImplFactory>(std::move(mock_timer),
                                                     simple_test_clock_.get());
-    DeviceSyncImpl::Factory::SetFactoryForTesting(
+    DeviceSyncImpl::Factory::SetCustomFactory(
         fake_device_sync_impl_factory_.get());
 
     fake_gcm_device_info_provider_ =
@@ -870,7 +870,7 @@ class DeviceSyncServiceTest
     CryptAuthEnrollmentManagerImpl::Factory::SetFactoryForTesting(nullptr);
     RemoteDeviceProviderImpl::Factory::SetFactoryForTesting(nullptr);
     SoftwareFeatureManagerImpl::Factory::SetFactoryForTesting(nullptr);
-    DeviceSyncImpl::Factory::SetFactoryForTesting(nullptr);
+    DeviceSyncImpl::Factory::SetCustomFactory(nullptr);
 
     NetworkHandler::Shutdown();
     DBusThreadManager::Shutdown();

chromeos/services/device_sync/public/cpp/device_sync_client_impl_unittest.cc

@@ -166,7 +166,7 @@ class DeviceSyncClientImplTest : public testing::Test {
     fake_device_sync_impl_factory_ =
         std::make_unique<FakeDeviceSyncImplFactory>(
             std::move(fake_device_sync));
-    DeviceSyncImpl::Factory::SetFactoryForTesting(
+    DeviceSyncImpl::Factory::SetCustomFactory(
         fake_device_sync_impl_factory_.get());
 
     auto shared_url_loader_factory =
@@ -303,7 +303,7 @@ class DeviceSyncClientImplTest : public testing::Test {
   }
 
   void TearDown() override {
-    DeviceSyncImpl::Factory::SetFactoryForTesting(nullptr);
+    DeviceSyncImpl::Factory::SetCustomFactory(nullptr);
     client_->RemoveObserver(test_observer_.get());
   }
 

chromeos/services/device_sync/stub_device_sync.cc

@@ -5,11 +5,16 @@
 #include "chromeos/services/device_sync/stub_device_sync.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/memory/ptr_util.h"
+#include "base/no_destructor.h"
 #include "base/optional.h"
 #include "base/time/time.h"
+#include "chromeos/components/multidevice/remote_device.h"
 #include "chromeos/components/multidevice/stub_multidevice_util.h"
+#include "chromeos/services/device_sync/device_sync_base.h"
+#include "chromeos/services/device_sync/device_sync_impl.h"
 #include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
 
 namespace chromeos {
@@ -51,124 +56,163 @@ void SetDeviceSoftwareFeatureState(
     device.software_features[feature] = new_state;
 }
 
-}  // namespace
-
-StubDeviceSync::StubDeviceSync()
-    : DeviceSyncBase(),
-      synced_devices_{multidevice::CreateStubHostPhone(),
-                      multidevice::CreateStubClientComputer()},
-      local_device_metadata_(multidevice::CreateStubClientComputer()) {}
-
-StubDeviceSync::~StubDeviceSync() = default;
-
-multidevice::RemoteDevice* StubDeviceSync::GetRemoteDevice(
-    const base::Optional<std::string>& device_public_key,
-    const base::Optional<std::string>& device_instance_id) {
-  auto it = std::find_if(synced_devices_.begin(), synced_devices_.end(),
-                         [&](const auto& device) {
-                           if (device_public_key != base::nullopt)
-                             return device.public_key == device_public_key;
-                           if (device_instance_id != base::nullopt)
-                             return device.instance_id == device_instance_id;
-                           return false;
-                         });
-  if (it == synced_devices_.end()) {
-    return nullptr;
+// Stub Device Sync implementation for Linux CrOS build. Creates two
+// fake devices, a fake phone and a fake computer.
+class StubDeviceSync : public DeviceSyncBase {
+ public:
+  StubDeviceSync()
+      : DeviceSyncBase(),
+        synced_devices_{multidevice::CreateStubHostPhone(),
+                        multidevice::CreateStubClientComputer()},
+        local_device_metadata_(multidevice::CreateStubClientComputer()) {}
+
+  ~StubDeviceSync() override = default;
+
+ protected:
+  // mojom::DeviceSync:
+  void ForceEnrollmentNow(ForceEnrollmentNowCallback callback) override {
+    std::move(callback).Run(/*success=*/true);
   }
-  return &(*it);
-}
 
-void StubDeviceSync::ForceEnrollmentNow(ForceEnrollmentNowCallback callback) {
-  std::move(callback).Run(/*success=*/true);
-}
+  void ForceSyncNow(ForceSyncNowCallback callback) override {
+    std::move(callback).Run(/*success=*/true);
+  }
 
-void StubDeviceSync::ForceSyncNow(ForceSyncNowCallback callback) {
-  std::move(callback).Run(/*success=*/true);
-}
+  void GetLocalDeviceMetadata(
+      GetLocalDeviceMetadataCallback callback) override {
+    std::move(callback).Run(local_device_metadata_);
+  }
 
-void StubDeviceSync::GetLocalDeviceMetadata(
-    GetLocalDeviceMetadataCallback callback) {
-  std::move(callback).Run(local_device_metadata_);
-}
+  void GetSyncedDevices(GetSyncedDevicesCallback callback) override {
+    std::move(callback).Run(synced_devices_);
+  }
 
-void StubDeviceSync::GetSyncedDevices(GetSyncedDevicesCallback callback) {
-  std::move(callback).Run(synced_devices_);
-}
+  void SetSoftwareFeatureState(
+      const std::string& device_public_key,
+      multidevice::SoftwareFeature software_feature,
+      bool enabled,
+      bool is_exclusive,
+      SetSoftwareFeatureStateCallback callback) override {
+    multidevice::RemoteDevice* device = GetRemoteDevice(
+        device_public_key, /*device_instance_id=*/base::nullopt);
+    if (!device) {
+      std::move(callback).Run(
+          /*result=*/mojom::NetworkRequestResult::kBadRequest);
+      return;
+    }
+
+    // Once software feature set for the appropriate device, return success.
+    SetDeviceSoftwareFeatureState(*device, software_feature, enabled);
+    NotifyOnNewDevicesSynced();
+    std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess);
+  }
 
-void StubDeviceSync::SetSoftwareFeatureState(
-    const std::string& device_public_key,
-    multidevice::SoftwareFeature software_feature,
-    bool enabled,
-    bool is_exclusive,
-    SetSoftwareFeatureStateCallback callback) {
-  multidevice::RemoteDevice* device =
-      GetRemoteDevice(device_public_key, /*device_instance_id=*/base::nullopt);
-  if (device == nullptr) {
-    std::move(callback).Run(
-        /*result=*/mojom::NetworkRequestResult::kBadRequest);
-    return;
+  void SetFeatureStatus(const std::string& device_instance_id,
+                        multidevice::SoftwareFeature feature,
+                        FeatureStatusChange status_change,
+                        SetFeatureStatusCallback callback) override {
+    multidevice::RemoteDevice* device = GetRemoteDevice(
+        /*device_public_key=*/base::nullopt, device_instance_id);
+    if (!device) {
+      std::move(callback).Run(
+          /*result=*/mojom::NetworkRequestResult::kBadRequest);
+      return;
+    }
+
+    SetDeviceSoftwareFeatureState(
+        *device, feature, status_change != FeatureStatusChange::kDisable);
+    NotifyOnNewDevicesSynced();
+    std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess);
   }
 
-  // Once software feature set for the appropriate device, return success.
-  SetDeviceSoftwareFeatureState(*device, software_feature, enabled);
-  NotifyOnNewDevicesSynced();
-  std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess);
-}
+  void FindEligibleDevices(multidevice::SoftwareFeature software_feature,
+                           FindEligibleDevicesCallback callback) override {
+    multidevice::RemoteDeviceList eligible_devices;
+    multidevice::RemoteDeviceList ineligible_devices;
 
-void StubDeviceSync::SetFeatureStatus(const std::string& device_instance_id,
-                                      multidevice::SoftwareFeature feature,
-                                      FeatureStatusChange status_change,
-                                      SetFeatureStatusCallback callback) {
-  multidevice::RemoteDevice* device =
-      GetRemoteDevice(/*device_public_key=*/base::nullopt, device_instance_id);
-  if (device == nullptr) {
     std::move(callback).Run(
-        /*result=*/mojom::NetworkRequestResult::kBadRequest);
-    return;
+        /*result=*/mojom::NetworkRequestResult::kSuccess,
+        /*response=*/mojom::FindEligibleDevicesResponse::New(
+            eligible_devices, ineligible_devices));
   }
 
-  SetDeviceSoftwareFeatureState(*device, feature,
-                                status_change != FeatureStatusChange::kDisable);
-  NotifyOnNewDevicesSynced();
-  std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess);
-}
+  void NotifyDevices(const std::vector<std::string>& device_instance_ids,
+                     cryptauthv2::TargetService target_service,
+                     multidevice::SoftwareFeature feature,
+                     NotifyDevicesCallback callback) override {
+    std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess);
+  }
 
-void StubDeviceSync::FindEligibleDevices(
-    multidevice::SoftwareFeature software_feature,
-    FindEligibleDevicesCallback callback) {
-  multidevice::RemoteDeviceList eligible_devices;
-  multidevice::RemoteDeviceList ineligible_devices;
+  void GetDebugInfo(GetDebugInfoCallback callback) override {
+    // Arbitrary values.
+    std::move(callback).Run(mojom::DebugInfo::New(
+        /*last_enrollment_time=*/base::Time::Now(),
+        /*time_to_next_enrollment_attempt=*/
+        base::TimeDelta::FromMilliseconds(10),
+        /*is_recovering_from_enrollment_failure=*/false,
+        /*is_enrollment_in_progress=*/false,
+        /*last_sync_time=*/base::Time::Now(),
+        /*time_to_next_sync_attempt=*/base::TimeDelta::FromMilliseconds(10),
+        /*is_recovering_from_sync_failure=*/false,
+        /*is_sync_in_progress=*/false));
+  }
 
-  std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess,
-                          /*response=*/mojom::FindEligibleDevicesResponse::New(
-                              eligible_devices, ineligible_devices));
-}
+  void GetDevicesActivityStatus(
+      GetDevicesActivityStatusCallback callback) override {
+    std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess,
+                            /*response=*/base::nullopt);
+  }
 
-void StubDeviceSync::NotifyDevices(
-    const std::vector<std::string>& device_instance_ids,
-    cryptauthv2::TargetService target_service,
-    multidevice::SoftwareFeature feature,
-    NotifyDevicesCallback callback) {
-  std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess);
-}
+ private:
+  // Returns the synced device that has a matching |device_public_key| or a
+  // matching |device_instance_id|, otherwise returns nullptr.
+  multidevice::RemoteDevice* GetRemoteDevice(
+      const base::Optional<std::string>& device_public_key,
+      const base::Optional<std::string>& device_instance_id) {
+    auto it = std::find_if(synced_devices_.begin(), synced_devices_.end(),
+                           [&](const auto& device) {
+                             if (device_public_key.has_value())
+                               return device.public_key == device_public_key;
+                             if (device_instance_id.has_value())
+                               return device.instance_id == device_instance_id;
+                             return false;
+                           });
+    if (it == synced_devices_.end()) {
+      return nullptr;
+    }
+    return &(*it);
+  }
 
-void StubDeviceSync::GetDebugInfo(GetDebugInfoCallback callback) {
-  // Arbitrary values.
-  std::move(callback).Run(mojom::DebugInfo::New(
-      /*last_enrollment_time=*/base::Time::Now(),
-      /*time_to_next_enrollment_attempt=*/base::TimeDelta::FromMilliseconds(10),
-      /*is_recovering_from_enrollment_failure=*/false,
-      /*is_enrollment_in_progress=*/false,
-      /*last_sync_time=*/base::Time::Now(),
-      /*time_to_next_sync_attempt=*/base::TimeDelta::FromMilliseconds(10),
-      /*is_recovering_from_sync_failure=*/false,
-      /*is_sync_in_progress=*/false));
-}
+  std::vector<multidevice::RemoteDevice> synced_devices_;
+  base::Optional<multidevice::RemoteDevice> local_device_metadata_;
+};
+
+class StubDeviceSyncImplFactory
+    : public chromeos::device_sync::DeviceSyncImpl::Factory {
+ public:
+  StubDeviceSyncImplFactory() = default;
+  ~StubDeviceSyncImplFactory() override = default;
+
+  // chromeos::device_sync::DeviceSyncImpl::Factory:
+  std::unique_ptr<chromeos::device_sync::DeviceSyncBase> CreateInstance(
+      signin::IdentityManager* identity_manager,
+      gcm::GCMDriver* gcm_driver,
+      PrefService* profile_prefs,
+      const chromeos::device_sync::GcmDeviceInfoProvider*
+          gcm_device_info_provider,
+      chromeos::device_sync::ClientAppMetadataProvider*
+          client_app_metadata_provider,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+      std::unique_ptr<base::OneShotTimer> timer) override {
+    return std::make_unique<StubDeviceSync>();
+  }
+};
+
+}  // namespace
 
-void StubDeviceSync::GetDevicesActivityStatus(
-    GetDevicesActivityStatusCallback callback) {
-  std::move(callback).Run(/*result=*/mojom::NetworkRequestResult::kSuccess,
-                          /*response=*/base::nullopt);
+void SetStubDeviceSyncFactory() {
+  static base::NoDestructor<StubDeviceSyncImplFactory> factory;
+  DeviceSyncImpl::Factory::SetCustomFactory(factory.get());
 }
 
 }  // namespace device_sync

chromeos/services/device_sync/stub_device_sync.h

@@ -5,58 +5,13 @@
 #ifndef CHROMEOS_SERVICES_DEVICE_SYNC_STUB_DEVICE_SYNC_H_
 #define CHROMEOS_SERVICES_DEVICE_SYNC_STUB_DEVICE_SYNC_H_
 
-#include <vector>
-
-#include "chromeos/components/multidevice/remote_device.h"
-#include "chromeos/services/device_sync/device_sync_base.h"
-
 namespace chromeos {
 
 namespace device_sync {
 
-// Stub Device Sync implementation for Linux CrOS build. Creates two
-// fake devices, a fake phone and a fake computer.
-class StubDeviceSync : public DeviceSyncBase {
- public:
-  StubDeviceSync();
-  ~StubDeviceSync() override;
-
- protected:
-  // mojom::DeviceSync:
-  void ForceEnrollmentNow(ForceEnrollmentNowCallback callback) override;
-  void ForceSyncNow(ForceSyncNowCallback callback) override;
-  void GetLocalDeviceMetadata(GetLocalDeviceMetadataCallback callback) override;
-  void GetSyncedDevices(GetSyncedDevicesCallback callback) override;
-  void SetSoftwareFeatureState(
-      const std::string& device_public_key,
-      multidevice::SoftwareFeature software_feature,
-      bool enabled,
-      bool is_exclusive,
-      SetSoftwareFeatureStateCallback callback) override;
-  void SetFeatureStatus(const std::string& device_instance_id,
-                        multidevice::SoftwareFeature feature,
-                        FeatureStatusChange status_change,
-                        SetFeatureStatusCallback callback) override;
-  void FindEligibleDevices(multidevice::SoftwareFeature software_feature,
-                           FindEligibleDevicesCallback callback) override;
-  void NotifyDevices(const std::vector<std::string>& device_instance_ids,
-                     cryptauthv2::TargetService target_service,
-                     multidevice::SoftwareFeature feature,
-                     NotifyDevicesCallback callback) override;
-  void GetDebugInfo(GetDebugInfoCallback callback) override;
-  void GetDevicesActivityStatus(
-      GetDevicesActivityStatusCallback callback) override;
-
- private:
-  // Returns the synced device that has a matching |device_public_key| or a
-  // matching |device_instance_id|, otherwise returns nullptr.
-  multidevice::RemoteDevice* GetRemoteDevice(
-      const base::Optional<std::string>& device_public_key,
-      const base::Optional<std::string>& device_instance_id);
-
-  std::vector<multidevice::RemoteDevice> synced_devices_;
-  base::Optional<multidevice::RemoteDevice> local_device_metadata_;
-};
+// Creates a stub DeviceSync factory that initializes a stub DeviceSync, then
+// sets that factory as the DeviceSyncImpl custom factory.
+void SetStubDeviceSyncFactory();
 
 }  // namespace device_sync