Sync: Consistently handle kSavingBrowserHistoryDisabled in controllers

The pref/policy kSavingBrowserHistoryDisabled should cause all
history-related sync data types to get disabled. The relevant data
types are: Sessions, TypedUrls, HistoryDeleteDirectives, and ProxyTabs.

This CL is mostly a cleanup/simplification, but there are behavior
changes in some (edge) cases: The policy being changed will now
consistently apply at runtime, whereas before, in some cases it only
applied after a browser restart.

Before this CL, the policy implementations for those 4 data types were
inconsistent: Sessions and TypedUrls watched for changes to the pref,
and turned themselves off - but Sessions with kMustStopAndKeepData
and TypedUrls with kMustStopAndClearData. The other two controllers
didn't watch the pref at all.
Additionally, SyncApiComponentFactoryImpl didn't even instantiate the
controllers if the pref was already set during startup - which means
that history sync could never get re-enabled during runtime.

This CL makes the behavior consistent across all 4 data types: Watch
the pref, and use kMustStopAndClearData. The logic in
SyncApiComponentFactoryImpl is not needed anymore and is removed.

Bug: 1303833
Change-Id: I65fe584745d15db12702afc46fba8f08b6de7c23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3620600
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001368}

components/browser_sync/sync_api_component_factory_impl.cc

@@ -251,46 +251,40 @@ SyncApiComponentFactoryImpl::CreateCommonDataTypeControllers(
     }
   }
 
-  // These features are enabled only if history is not disabled.
-  if (!sync_client_->GetPrefService()->GetBoolean(
-          prefs::kSavingBrowserHistoryDisabled)) {
-    // TypedUrl sync is enabled by default.  Register unless explicitly
-    // disabled.
-    if (!disabled_types.Has(syncer::TYPED_URLS)) {
-      // TypedURLModelTypeController uses a proxy delegate internally, as
-      // provided by HistoryService.
-      controllers.push_back(
-          std::make_unique<history::TypedURLModelTypeController>(
-              sync_client_->GetHistoryService(),
-              sync_client_->GetPrefService()));
-    }
+  // TypedUrl sync is enabled by default.  Register unless explicitly disabled.
+  if (!disabled_types.Has(syncer::TYPED_URLS)) {
+    // TypedURLModelTypeController uses a proxy delegate internally, as
+    // provided by HistoryService.
+    controllers.push_back(
+        std::make_unique<history::TypedURLModelTypeController>(
+            sync_service, sync_client_->GetHistoryService(),
+            sync_client_->GetPrefService()));
+  }
 
-    // Delete directive sync is enabled by default.
-    if (!disabled_types.Has(syncer::HISTORY_DELETE_DIRECTIVES)) {
-      controllers.push_back(
-          std::make_unique<history::HistoryDeleteDirectivesModelTypeController>(
-              dump_stack, sync_service,
-              sync_client_->GetModelTypeStoreService(),
-              sync_client_->GetHistoryService()));
-    }
+  // Delete directive sync is enabled by default.
+  if (!disabled_types.Has(syncer::HISTORY_DELETE_DIRECTIVES)) {
+    controllers.push_back(
+        std::make_unique<history::HistoryDeleteDirectivesModelTypeController>(
+            dump_stack, sync_service, sync_client_->GetModelTypeStoreService(),
+            sync_client_->GetHistoryService(), sync_client_->GetPrefService()));
+  }
 
-    // Session sync is enabled by default.  This is disabled if history is
-    // disabled because the tab sync data is added to the web history on the
-    // server.
-    if (!disabled_types.Has(syncer::PROXY_TABS)) {
-      controllers.push_back(
-          std::make_unique<sync_sessions::ProxyTabsDataTypeController>(
-              base::BindRepeating(
-                  &sync_sessions::SessionSyncService::ProxyTabsStateChanged,
-                  base::Unretained(sync_client_->GetSessionSyncService()))));
-      controllers.push_back(
-          std::make_unique<sync_sessions::SessionModelTypeController>(
-              sync_service, sync_client_->GetPrefService(),
-              std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
-                  sync_client_->GetSessionSyncService()
-                      ->GetControllerDelegate()
-                      .get())));
-    }
+  if (!disabled_types.Has(syncer::PROXY_TABS)) {
+    controllers.push_back(
+        std::make_unique<sync_sessions::ProxyTabsDataTypeController>(
+            sync_service, sync_client_->GetPrefService(),
+            base::BindRepeating(
+                &sync_sessions::SessionSyncService::ProxyTabsStateChanged,
+                base::Unretained(sync_client_->GetSessionSyncService()))));
+  }
+  if (!disabled_types.Has(syncer::SESSIONS)) {
+    controllers.push_back(
+        std::make_unique<sync_sessions::SessionModelTypeController>(
+            sync_service, sync_client_->GetPrefService(),
+            std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
+                sync_client_->GetSessionSyncService()
+                    ->GetControllerDelegate()
+                    .get())));
   }
 
   // Password sync is enabled by default.  Register unless explicitly

components/history/core/browser/BUILD.gn

@@ -52,6 +52,8 @@ static_library("browser") {
     "sync/delete_directive_handler.h",
     "sync/history_delete_directives_model_type_controller.cc",
     "sync/history_delete_directives_model_type_controller.h",
+    "sync/history_model_type_controller_helper.cc",
+    "sync/history_model_type_controller_helper.h",
     "sync/typed_url_model_type_controller.cc",
     "sync/typed_url_model_type_controller.h",
     "sync/typed_url_sync_bridge.cc",

components/history/core/browser/sync/history_delete_directives_model_type_controller.cc

@@ -6,7 +6,6 @@
 
 #include <utility>
 
-#include "base/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/sync/driver/sync_service.h"
@@ -32,23 +31,26 @@ HistoryDeleteDirectivesModelTypeController::
         const base::RepeatingClosure& dump_stack,
         syncer::SyncService* sync_service,
         syncer::ModelTypeStoreService* model_type_store_service,
-        HistoryService* history_service)
+        HistoryService* history_service,
+        PrefService* pref_service)
     : SyncableServiceBasedModelTypeController(
           syncer::HISTORY_DELETE_DIRECTIVES,
           model_type_store_service->GetStoreFactory(),
           GetSyncableServiceFromHistoryService(history_service),
           dump_stack),
+      helper_(syncer::HISTORY_DELETE_DIRECTIVES, sync_service, pref_service),
       sync_service_(sync_service) {}
 
 HistoryDeleteDirectivesModelTypeController::
-    ~HistoryDeleteDirectivesModelTypeController() {}
+    ~HistoryDeleteDirectivesModelTypeController() = default;
 
 syncer::DataTypeController::PreconditionState
 HistoryDeleteDirectivesModelTypeController::GetPreconditionState() const {
   DCHECK(CalledOnValidThread());
-  return sync_service_->GetUserSettings()->IsEncryptEverythingEnabled()
-             ? PreconditionState::kMustStopAndClearData
-             : PreconditionState::kPreconditionsMet;
+  if (sync_service_->GetUserSettings()->IsEncryptEverythingEnabled()) {
+    return PreconditionState::kMustStopAndClearData;
+  }
+  return helper_.GetPreconditionState();
 }
 
 void HistoryDeleteDirectivesModelTypeController::LoadModels(

components/history/core/browser/sync/history_delete_directives_model_type_controller.h

@@ -5,11 +5,13 @@
 #ifndef COMPONENTS_HISTORY_CORE_BROWSER_SYNC_HISTORY_DELETE_DIRECTIVES_MODEL_TYPE_CONTROLLER_H_
 #define COMPONENTS_HISTORY_CORE_BROWSER_SYNC_HISTORY_DELETE_DIRECTIVES_MODEL_TYPE_CONTROLLER_H_
 
-#include "base/callback_forward.h"
 #include "base/memory/raw_ptr.h"
+#include "components/history/core/browser/sync/history_model_type_controller_helper.h"
 #include "components/sync/driver/sync_service_observer.h"
 #include "components/sync/driver/syncable_service_based_model_type_controller.h"
 
+class PrefService;
+
 namespace syncer {
 class ModelTypeStoreService;
 class SyncService;
@@ -25,13 +27,14 @@ class HistoryDeleteDirectivesModelTypeController
     : public syncer::SyncableServiceBasedModelTypeController,
       public syncer::SyncServiceObserver {
  public:
-  // `sync_service` and `history_service` must not be null and must outlive this
-  // object.
+  // `sync_service`, `history_service`, and `pref_service` must not be null and
+  // must outlive this object.
   HistoryDeleteDirectivesModelTypeController(
       const base::RepeatingClosure& dump_stack,
       syncer::SyncService* sync_service,
       syncer::ModelTypeStoreService* model_type_store_service,
-      HistoryService* history_service);
+      HistoryService* history_service,
+      PrefService* pref_service);
 
   HistoryDeleteDirectivesModelTypeController(
       const HistoryDeleteDirectivesModelTypeController&) = delete;
@@ -51,6 +54,8 @@ class HistoryDeleteDirectivesModelTypeController
   void OnStateChanged(syncer::SyncService* sync) override;
 
  private:
+  history::HistoryModelTypeControllerHelper helper_;
+
   const raw_ptr<syncer::SyncService> sync_service_;
 };
 

components/history/core/browser/sync/history_model_type_controller_helper.cc

@@ -0,0 +1,44 @@
+// Copyright 2022 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/history/core/browser/sync/history_model_type_controller_helper.h"
+
+#include "base/bind.h"
+#include "components/history/core/common/pref_names.h"
+#include "components/prefs/pref_service.h"
+#include "components/sync/driver/sync_service.h"
+
+namespace history {
+
+HistoryModelTypeControllerHelper::HistoryModelTypeControllerHelper(
+    syncer::ModelType model_type,
+    syncer::SyncService* sync_service,
+    PrefService* pref_service)
+    : model_type_(model_type),
+      sync_service_(sync_service),
+      pref_service_(pref_service) {
+  pref_registrar_.Init(pref_service_);
+  // base::Unretained() is safe because `pref_registar_` is owned by `this`.
+  pref_registrar_.Add(
+      prefs::kSavingBrowserHistoryDisabled,
+      base::BindRepeating(&HistoryModelTypeControllerHelper::
+                              OnSavingBrowserHistoryDisabledChanged,
+                          base::Unretained(this)));
+}
+
+HistoryModelTypeControllerHelper::~HistoryModelTypeControllerHelper() = default;
+
+syncer::DataTypeController::PreconditionState
+HistoryModelTypeControllerHelper::GetPreconditionState() const {
+  return pref_service_->GetBoolean(prefs::kSavingBrowserHistoryDisabled)
+             ? syncer::DataTypeController::PreconditionState::
+                   kMustStopAndClearData
+             : syncer::DataTypeController::PreconditionState::kPreconditionsMet;
+}
+
+void HistoryModelTypeControllerHelper::OnSavingBrowserHistoryDisabledChanged() {
+  sync_service_->DataTypePreconditionChanged(model_type_);
+}
+
+}  // namespace history

components/history/core/browser/sync/history_model_type_controller_helper.h

@@ -0,0 +1,53 @@
+// Copyright 2022 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_HISTORY_CORE_BROWSER_SYNC_HISTORY_MODEL_TYPE_CONTROLLER_HELPER_H_
+#define COMPONENTS_HISTORY_CORE_BROWSER_SYNC_HISTORY_MODEL_TYPE_CONTROLLER_HELPER_H_
+
+#include "base/memory/raw_ptr.h"
+#include "components/prefs/pref_change_registrar.h"
+#include "components/sync/base/model_type.h"
+#include "components/sync/driver/data_type_controller.h"
+
+class PrefService;
+
+namespace syncer {
+class SyncService;
+}  // namespace syncer
+
+namespace history {
+
+// Helper class for implementing history-related ModelTypeControllers. It
+// implements the pref/policy kSavingBrowserHistoryDisabled: It calls
+// SyncService::DataTypePreconditionChanged() when the pref changes.
+// DataTypeControllers using this helper must call its GetPreconditionState().
+class HistoryModelTypeControllerHelper {
+ public:
+  HistoryModelTypeControllerHelper(syncer::ModelType model_type,
+                                   syncer::SyncService* sync_service,
+                                   PrefService* pref_service);
+
+  HistoryModelTypeControllerHelper(const HistoryModelTypeControllerHelper&) =
+      delete;
+  HistoryModelTypeControllerHelper& operator=(
+      const HistoryModelTypeControllerHelper&) = delete;
+
+  ~HistoryModelTypeControllerHelper();
+
+  // Must be called from DataTypeController::GetPreconditionState().
+  syncer::DataTypeController::PreconditionState GetPreconditionState() const;
+
+ private:
+  void OnSavingBrowserHistoryDisabledChanged();
+
+  const syncer::ModelType model_type_;
+  const raw_ptr<syncer::SyncService> sync_service_;
+  const raw_ptr<PrefService> pref_service_;
+
+  PrefChangeRegistrar pref_registrar_;
+};
+
+}  // namespace history
+
+#endif  // COMPONENTS_HISTORY_CORE_BROWSER_SYNC_HISTORY_MODEL_TYPE_CONTROLLER_HELPER_H_

components/history/core/browser/sync/typed_url_model_type_controller.cc

@@ -8,12 +8,6 @@
 
 #include "base/bind.h"
 #include "components/history/core/browser/history_service.h"
-#include "components/history/core/common/pref_names.h"
-#include "components/prefs/pref_service.h"
-#include "components/sync/driver/model_type_controller.h"
-#include "components/sync/driver/sync_client.h"
-
-using syncer::SyncClient;
 
 namespace history {
 
@@ -30,42 +24,18 @@ GetDelegateFromHistoryService(HistoryService* history_service) {
 }  // namespace
 
 TypedURLModelTypeController::TypedURLModelTypeController(
+    syncer::SyncService* sync_service,
     HistoryService* history_service,
     PrefService* pref_service)
     : ModelTypeController(syncer::TYPED_URLS,
                           GetDelegateFromHistoryService(history_service)),
-      history_service_(history_service),
-      pref_service_(pref_service) {
-  pref_registrar_.Init(pref_service_);
-  pref_registrar_.Add(
-      prefs::kSavingBrowserHistoryDisabled,
-      base::BindRepeating(
-          &TypedURLModelTypeController::OnSavingBrowserHistoryDisabledChanged,
-          base::AsWeakPtr(this)));
-}
+      helper_(syncer::TYPED_URLS, sync_service, pref_service) {}
 
-TypedURLModelTypeController::~TypedURLModelTypeController() {}
+TypedURLModelTypeController::~TypedURLModelTypeController() = default;
 
 syncer::DataTypeController::PreconditionState
 TypedURLModelTypeController::GetPreconditionState() const {
-  return (history_service_ &&
-          !pref_service_->GetBoolean(prefs::kSavingBrowserHistoryDisabled))
-             ? PreconditionState::kPreconditionsMet
-             : PreconditionState::kMustStopAndClearData;
-}
-
-void TypedURLModelTypeController::OnSavingBrowserHistoryDisabledChanged() {
-  if (pref_service_->GetBoolean(prefs::kSavingBrowserHistoryDisabled)) {
-    // We've turned off history persistence, so if we are running,
-    // generate an unrecoverable error. This can be fixed by restarting
-    // Chrome (on restart, typed urls will not be a registered type).
-    // TODO(crbug.com/990802): Adopt DataTypePreconditionChanged().
-    if (state() != NOT_RUNNING && state() != STOPPING) {
-      ReportModelError(
-          syncer::SyncError::DATATYPE_POLICY_ERROR,
-          {FROM_HERE, "History saving is now disabled by policy."});
-    }
-  }
+  return helper_.GetPreconditionState();
 }
 
 }  // namespace history

components/history/core/browser/sync/typed_url_model_type_controller.h

@@ -5,19 +5,23 @@
 #ifndef COMPONENTS_HISTORY_CORE_BROWSER_SYNC_TYPED_URL_MODEL_TYPE_CONTROLLER_H_
 #define COMPONENTS_HISTORY_CORE_BROWSER_SYNC_TYPED_URL_MODEL_TYPE_CONTROLLER_H_
 
-#include "base/memory/raw_ptr.h"
-#include "components/prefs/pref_change_registrar.h"
+#include "components/history/core/browser/sync/history_model_type_controller_helper.h"
 #include "components/sync/driver/model_type_controller.h"
 
 class PrefService;
 
+namespace syncer {
+class SyncService;
+}  // namespace syncer
+
 namespace history {
 
 class HistoryService;
 
 class TypedURLModelTypeController : public syncer::ModelTypeController {
  public:
-  TypedURLModelTypeController(HistoryService* history_service,
+  TypedURLModelTypeController(syncer::SyncService* sync_service,
+                              HistoryService* history_service,
                               PrefService* pref_service);
 
   TypedURLModelTypeController(const TypedURLModelTypeController&) = delete;
@@ -30,12 +34,7 @@ class TypedURLModelTypeController : public syncer::ModelTypeController {
   PreconditionState GetPreconditionState() const override;
 
  private:
-  void OnSavingBrowserHistoryDisabledChanged();
-
-  const raw_ptr<HistoryService> history_service_;
-  const raw_ptr<PrefService> pref_service_;
-
-  PrefChangeRegistrar pref_registrar_;
+  HistoryModelTypeControllerHelper helper_;
 };
 
 }  // namespace history

components/sync_sessions/BUILD.gn

@@ -53,6 +53,7 @@ static_library("sync_sessions") {
     "//components/bookmarks/browser",
     "//components/favicon/core",
     "//components/favicon_base",
+    "//components/history/core/browser",
     "//components/history/core/common",
     "//components/keyed_service/core",
     "//components/prefs",

components/sync_sessions/DEPS

@@ -2,6 +2,7 @@ include_rules = [
   "+components/bookmarks/browser",
   "+components/favicon/core",
   "+components/favicon_base",
+  "+components/history/core/browser/sync",
   "+components/history/core/common",
   "+components/keyed_service/core",
   "+components/prefs",