Integrate SEARCH_ENGINES sync into pseudo-USS

This is part of a patch series where we leverage the recently introduced
layer that allows integrating legacy SyncableService implementations
within the newest architecture, USS.

SEARCH_ENGINES is the first datatype where the legacy controller
(SearchEngineDataTypeController) implements logic to make sure no
interactions with the SyncableService can occur before
TemplateURLService is fully loaded.

Implementing similar logic in pseudo-USS is nontrivial because:
1. ModelTypeController doesn't support it because it is designed for
   full-blown USS, where such logic is usually implemented in the bridge
   itself.

2. SyncableService doesn't support it, because in the legacy
   architecture the controllers took care (AsyncDirectoryTypeController).

For pseudo-USS, this being the first datatype, the least intrusive
solution is proposed, which involves a dedicated
ModelTypeControllerDelegate. We may change this in the future if more
datatypes have similar needs (e.g. ArcPackageSyncDataTypeController).

Bug: 870624
Change-Id: I28bb028de99d98608bdf07a06db1883221b8161e
Reviewed-on: https://chromium-review.googlesource.com/c/1261435
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596756}

chrome/browser/sync/chrome_sync_client.cc

@@ -63,6 +63,7 @@
 #include "components/password_manager/core/browser/password_model_worker.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/search_engines/search_engine_data_type_controller.h"
+#include "components/search_engines/search_engine_model_type_controller.h"
 #include "components/spellcheck/spellcheck_buildflags.h"
 #include "components/sync/base/pref_names.h"
 #include "components/sync/base/report_unrecoverable_error.h"
@@ -373,9 +374,15 @@ ChromeSyncClient::CreateDataTypeControllers(
   // Search Engine sync is enabled by default.  Register unless explicitly
   // disabled.
   if (!disabled_types.Has(syncer::SEARCH_ENGINES)) {
-    controllers.push_back(std::make_unique<SearchEngineDataTypeController>(
-        error_callback, this,
-        TemplateURLServiceFactory::GetForProfile(profile_)));
+    if (base::FeatureList::IsEnabled(switches::kSyncPseudoUSSSearchEngines)) {
+      controllers.push_back(std::make_unique<SearchEngineModelTypeController>(
+          error_callback, GetModelTypeStoreService()->GetStoreFactory(),
+          TemplateURLServiceFactory::GetForProfile(profile_)));
+    } else {
+      controllers.push_back(std::make_unique<SearchEngineDataTypeController>(
+          error_callback, this,
+          TemplateURLServiceFactory::GetForProfile(profile_)));
+    }
   }
 #endif  // !defined(OS_ANDROID)
 

chrome/browser/sync/test/integration/single_client_search_engines_sync_test.cc

@@ -3,25 +3,34 @@
 // found in the LICENSE file.
 
 #include "base/macros.h"
+#include "chrome/browser/sync/test/integration/feature_toggler.h"
 #include "chrome/browser/sync/test/integration/search_engines_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/search_engines/template_url_service.h"
+#include "components/sync/driver/sync_driver_switches.h"
 
-class SingleClientSearchEnginesSyncTest : public SyncTest {
+class SingleClientSearchEnginesSyncTest : public FeatureToggler,
+                                          public SyncTest {
  public:
-  SingleClientSearchEnginesSyncTest() : SyncTest(SINGLE_CLIENT) {}
+  SingleClientSearchEnginesSyncTest()
+      : FeatureToggler(switches::kSyncPseudoUSSSearchEngines),
+        SyncTest(SINGLE_CLIENT) {}
   ~SingleClientSearchEnginesSyncTest() override {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SingleClientSearchEnginesSyncTest);
 };
 
-IN_PROC_BROWSER_TEST_F(SingleClientSearchEnginesSyncTest, Sanity) {
+IN_PROC_BROWSER_TEST_P(SingleClientSearchEnginesSyncTest, Sanity) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::ServiceMatchesVerifier(0));
   search_engines_helper::AddSearchEngine(0, 0);
   ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
   ASSERT_TRUE(search_engines_helper::ServiceMatchesVerifier(0));
 }
+
+INSTANTIATE_TEST_CASE_P(USS,
+                        SingleClientSearchEnginesSyncTest,
+                        ::testing::Values(false, true));

chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc

@@ -5,6 +5,7 @@
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/sync/test/integration/feature_toggler.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/search_engines_helper.h"
 #include "chrome/browser/sync/test/integration/sync_datatype_helper.h"
@@ -12,19 +13,22 @@
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_service.h"
+#include "components/sync/driver/sync_driver_switches.h"
 
 using base::ASCIIToUTF16;
 
-class TwoClientSearchEnginesSyncTest : public SyncTest {
+class TwoClientSearchEnginesSyncTest : public FeatureToggler, public SyncTest {
  public:
-  TwoClientSearchEnginesSyncTest() : SyncTest(TWO_CLIENT) {}
+  TwoClientSearchEnginesSyncTest()
+      : FeatureToggler(switches::kSyncPseudoUSSSearchEngines),
+        SyncTest(TWO_CLIENT) {}
   ~TwoClientSearchEnginesSyncTest() override {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(TwoClientSearchEnginesSyncTest);
 };
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, E2E_ENABLED(Add)) {
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest, E2E_ENABLED(Add)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
 
@@ -38,7 +42,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, E2E_ENABLED(Add)) {
   ASSERT_TRUE(search_engines_helper::HasSearchEngine(1, search_engine_seed));
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, E2E_ENABLED(Delete)) {
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest, E2E_ENABLED(Delete)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
 
@@ -57,7 +61,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, E2E_ENABLED(Delete)) {
   ASSERT_FALSE(search_engines_helper::HasSearchEngine(1, search_engine_seed));
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest,
                        E2E_ENABLED(AddMultiple)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
@@ -69,7 +73,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
   ASSERT_TRUE(SearchEnginesMatchChecker().Wait());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, Duplicates) {
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest, Duplicates) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
 
@@ -86,7 +90,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, Duplicates) {
   ASSERT_TRUE(SearchEnginesMatchChecker().Wait());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest,
                        E2E_ENABLED(UpdateKeyword)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
@@ -103,7 +107,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
   ASSERT_TRUE(SearchEnginesMatchChecker().Wait());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, E2E_ENABLED(UpdateUrl)) {
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest, E2E_ENABLED(UpdateUrl)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
 
@@ -119,7 +123,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, E2E_ENABLED(UpdateUrl)) {
   ASSERT_TRUE(SearchEnginesMatchChecker().Wait());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest,
                        E2E_ENABLED(UpdateName)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
@@ -135,7 +139,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
   ASSERT_TRUE(SearchEnginesMatchChecker().Wait());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, ConflictKeyword) {
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest, ConflictKeyword) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   DisableVerifier();
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
@@ -155,7 +159,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, ConflictKeyword) {
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, MergeMultiple) {
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest, MergeMultiple) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   DisableVerifier();
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
@@ -180,7 +184,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, MergeMultiple) {
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, DisableSync) {
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest, DisableSync) {
   ASSERT_TRUE(SetupSync());
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
 
@@ -195,7 +199,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest, DisableSync) {
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest,
                        E2E_ENABLED(SyncDefault)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
@@ -212,7 +216,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
 
 // Ensure that we can change the search engine and immediately delete it
 // without putting the clients out of sync.
-IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientSearchEnginesSyncTest,
                        E2E_ENABLED(DeleteSyncedDefault)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(search_engines_helper::AllServicesMatch());
@@ -229,3 +233,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSearchEnginesSyncTest,
   search_engines_helper::DeleteSearchEngineBySeed(0, 0);
   ASSERT_TRUE(SearchEnginesMatchChecker().Wait());
 }
+
+INSTANTIATE_TEST_CASE_P(USS,
+                        TwoClientSearchEnginesSyncTest,
+                        ::testing::Values(false, true));

components/search_engines/BUILD.gn

@@ -19,6 +19,8 @@ static_library("search_engines") {
     "keyword_web_data_service.h",
     "search_engine_data_type_controller.cc",
     "search_engine_data_type_controller.h",
+    "search_engine_model_type_controller.cc",
+    "search_engine_model_type_controller.h",
     "search_engine_type.h",
     "search_engines_pref_names.cc",
     "search_engines_pref_names.h",

components/search_engines/search_engine_model_type_controller.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/search_engines/search_engine_model_type_controller.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "components/search_engines/template_url_service.h"
+#include "components/sync/base/model_type.h"
+#include "components/sync/model_impl/client_tag_based_model_type_processor.h"
+#include "components/sync/model_impl/forwarding_model_type_controller_delegate.h"
+#include "components/sync/model_impl/syncable_service_based_bridge.h"
+
+namespace browser_sync {
+
+namespace {
+
+// Similar to ForwardingModelTypeControllerDelegate, but allows owning a bridge
+// and deferring OnSyncStarting() until TemplateURLService is loaded.
+class ControllerDelegate
+    : public syncer::ForwardingModelTypeControllerDelegate {
+ public:
+  // |bridge| and |template_url_service| must not be null.
+  ControllerDelegate(std::unique_ptr<syncer::ModelTypeSyncBridge> bridge,
+                     TemplateURLService* template_url_service)
+      : ForwardingModelTypeControllerDelegate(
+            bridge->change_processor()->GetControllerDelegate().get()),
+        bridge_(std::move(bridge)),
+        template_url_service_(template_url_service) {}
+
+  ~ControllerDelegate() override = default;
+
+  void OnSyncStarting(const syncer::DataTypeActivationRequest& request,
+                      StartCallback callback) override {
+    DCHECK(!template_url_subscription_);
+
+    // We force a load here to allow remote updates to be processed, without
+    // waiting for TemplateURLService's lazy load.
+    template_url_service_->Load();
+
+    // If the service is loaded, continue normally, which means requesting the
+    // processor to start.
+    if (template_url_service_->loaded()) {
+      ForwardingModelTypeControllerDelegate::OnSyncStarting(
+          request, std::move(callback));
+      return;
+    }
+
+    // Otherwise, wait until it becomes ready. Using base::Unretained() should
+    // be safe here because the subscription itself will be destroyed together
+    // with this object.
+    template_url_subscription_ =
+        template_url_service_->RegisterOnLoadedCallback(
+            base::AdaptCallbackForRepeating(base::BindOnce(
+                &ControllerDelegate::OnTemplateURLServiceLoaded,
+                base::Unretained(this), request, std::move(callback))));
+  }
+
+ private:
+  void OnTemplateURLServiceLoaded(
+      const syncer::DataTypeActivationRequest& request,
+      StartCallback callback) {
+    template_url_subscription_.reset();
+    DCHECK(template_url_service_->loaded());
+    // Now that we're loaded, continue normally, which means requesting the
+    // processor to start.
+    ForwardingModelTypeControllerDelegate::OnSyncStarting(request,
+                                                          std::move(callback));
+  }
+
+  const std::unique_ptr<syncer::ModelTypeSyncBridge> bridge_;
+  TemplateURLService* const template_url_service_;
+  std::unique_ptr<TemplateURLService::Subscription> template_url_subscription_;
+
+  DISALLOW_COPY_AND_ASSIGN(ControllerDelegate);
+};
+
+// Helper function to construct the various objects needed to run this datatype.
+std::unique_ptr<ControllerDelegate> BuildDelegate(
+    const base::RepeatingClosure& dump_stack,
+    syncer::OnceModelTypeStoreFactory store_factory,
+    TemplateURLService* template_url_service) {
+  DCHECK(template_url_service);
+
+  auto processor = std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
+      syncer::SEARCH_ENGINES, dump_stack);
+
+  auto bridge = std::make_unique<syncer::SyncableServiceBasedBridge>(
+      syncer::SEARCH_ENGINES, std::move(store_factory), std::move(processor),
+      template_url_service);
+
+  return std::make_unique<ControllerDelegate>(std::move(bridge),
+                                              template_url_service);
+}
+
+}  // namespace
+
+SearchEngineModelTypeController::SearchEngineModelTypeController(
+    const base::RepeatingClosure& dump_stack,
+    syncer::OnceModelTypeStoreFactory store_factory,
+    TemplateURLService* template_url_service)
+    : ModelTypeController(syncer::SEARCH_ENGINES,
+                          BuildDelegate(dump_stack,
+                                        std::move(store_factory),
+                                        template_url_service)) {}
+
+SearchEngineModelTypeController::~SearchEngineModelTypeController() = default;
+
+}  // namespace browser_sync

components/search_engines/search_engine_model_type_controller.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_SEARCH_ENGINES_SEARCH_ENGINE_MODEL_TYPE_CONTROLLER_H_
+#define COMPONENTS_SEARCH_ENGINES_SEARCH_ENGINE_MODEL_TYPE_CONTROLLER_H_
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "components/sync/driver/model_type_controller.h"
+#include "components/sync/model/model_type_store.h"
+
+class TemplateURLService;
+
+namespace browser_sync {
+
+// Controller for the SEARCH_ENGINES sync data type. This class tells sync how
+// to load the model for this data type, and the superclasses manage controlling
+// the rest of the state of the datatype with regards to sync. This is analogous
+// to SearchEngineDataTypeController but allows exercising the more modern USS
+// architecture.
+class SearchEngineModelTypeController : public syncer::ModelTypeController {
+ public:
+  // |template_url_service| represents the syncable service itself for
+  // SEARCH_ENGINES. It must not be null and must outlive this object.
+  // |dump_stack| allows the internal implementation (the processor) to report
+  // error dumps. |store_factory| is used to instantiate a ModelTypeStore that
+  // is used to persist sync [meta]data.
+  SearchEngineModelTypeController(
+      const base::RepeatingClosure& dump_stack,
+      syncer::OnceModelTypeStoreFactory store_factory,
+      TemplateURLService* template_url_service);
+  ~SearchEngineModelTypeController() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SearchEngineModelTypeController);
+};
+
+}  // namespace browser_sync
+
+#endif  // COMPONENTS_SEARCH_ENGINES_SEARCH_ENGINE_MODEL_TYPE_CONTROLLER_H_

components/sync/driver/sync_driver_switches.cc

@@ -64,6 +64,8 @@ const base::Feature kSyncPseudoUSSPreferences{
     "SyncPseudoUSSPreferences", base::FEATURE_DISABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSPriorityPreferences{
     "SyncPseudoUSSPriorityPreferences", base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kSyncPseudoUSSSearchEngines{
+    "SyncPseudoUSSSearchEngines", base::FEATURE_DISABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSSupervisedUsers{
     "SyncPseudoUSSSupervisedUsers", base::FEATURE_DISABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSThemes{"SyncPseudoUSSThemes",

components/sync/driver/sync_driver_switches.h

@@ -31,6 +31,7 @@ extern const base::Feature kSyncPseudoUSSFavicons;
 extern const base::Feature kSyncPseudoUSSHistoryDeleteDirectives;
 extern const base::Feature kSyncPseudoUSSPreferences;
 extern const base::Feature kSyncPseudoUSSPriorityPreferences;
+extern const base::Feature kSyncPseudoUSSSearchEngines;
 extern const base::Feature kSyncPseudoUSSSupervisedUsers;
 extern const base::Feature kSyncPseudoUSSThemes;
 extern const base::Feature kSyncStandaloneTransport;

components/sync/driver/syncable_service_based_model_type_controller.cc

@@ -7,7 +7,6 @@
 #include <utility>
 
 #include "components/sync/model_impl/client_tag_based_model_type_processor.h"
-#include "components/sync/model_impl/forwarding_model_type_controller_delegate.h"
 #include "components/sync/model_impl/syncable_service_based_bridge.h"
 
 namespace syncer {