Sync: Pass GetPreferredDataTypes as a callback into StartupController

This makes a few things a bit nicer: Now StartupController doesn't have
to know about the set of registered data types anymore, which are really
not its business. As a consequence, it's not necessary to call Reset()
before using the StartupController.
This also removes one of two uses of SyncPrefs from StartupController;
I hope to remove the remaining one soon.

Bug: 854978
Change-Id: I75c8971342554eb190287724eff2ff3238ffbff6
Reviewed-on: https://chromium-review.googlesource.com/1110220
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569337}

components/browser_sync/profile_sync_service.cc

@@ -232,6 +232,8 @@ void ProfileSyncService::Initialize() {
 
   startup_controller_ = std::make_unique<syncer::StartupController>(
       &sync_prefs_,
+      base::BindRepeating(&ProfileSyncService::GetPreferredDataTypes,
+                          base::Unretained(this)),
       base::BindRepeating(&ProfileSyncService::CanSyncStart,
                           base::Unretained(this)),
       base::BindRepeating(&ProfileSyncService::StartUpSlowEngineComponents,
@@ -338,7 +340,6 @@ void ProfileSyncService::Initialize() {
   memory_pressure_listener_ = std::make_unique<base::MemoryPressureListener>(
       base::BindRepeating(&ProfileSyncService::OnMemoryPressure,
                           sync_enabled_weak_factory_.GetWeakPtr()));
-  startup_controller_->Reset(GetRegisteredDataTypes());
 
   // Auto-start means the first time the profile starts up, sync should start up
   // immediately.
@@ -693,7 +694,7 @@ void ProfileSyncService::ShutdownImpl(syncer::ShutdownReason reason) {
 
   sync_enabled_weak_factory_.InvalidateWeakPtrs();
 
-  startup_controller_->Reset(GetRegisteredDataTypes());
+  startup_controller_->Reset();
 
   // If the sync DB is getting destroyed, the local DeviceInfo is no longer
   // valid and should be cleared from the cache.

components/sync/driver/startup_controller.cc

@@ -57,12 +57,15 @@ enum DeferredInitTrigger {
 
 }  // namespace
 
-StartupController::StartupController(const SyncPrefs* sync_prefs,
-                                     base::RepeatingCallback<bool()> can_start,
-                                     base::RepeatingClosure start_engine)
+StartupController::StartupController(
+    const SyncPrefs* sync_prefs,
+    base::RepeatingCallback<ModelTypeSet()> get_preferred_data_types,
+    base::RepeatingCallback<bool()> can_start,
+    base::RepeatingClosure start_engine)
     : sync_prefs_(sync_prefs),
-      can_start_(std::move(can_start)),
-      start_engine_(std::move(start_engine)),
+      get_preferred_data_types_callback_(std::move(get_preferred_data_types)),
+      can_start_callback_(std::move(can_start)),
+      start_engine_callback_(std::move(start_engine)),
       bypass_setup_complete_(false),
       received_start_request_(false),
       setup_in_progress_(false),
@@ -70,14 +73,13 @@ StartupController::StartupController(const SyncPrefs* sync_prefs,
 
 StartupController::~StartupController() {}
 
-void StartupController::Reset(const ModelTypeSet& registered_types) {
+void StartupController::Reset() {
   received_start_request_ = false;
   bypass_setup_complete_ = false;
   start_up_time_ = base::Time();
   start_engine_time_ = base::Time();
   // Don't let previous timers affect us post-reset.
   weak_factory_.InvalidateWeakPtrs();
-  registered_types_ = registered_types;
 }
 
 void StartupController::SetSetupInProgress(bool setup_in_progress) {
@@ -94,7 +96,7 @@ void StartupController::StartUp(StartUpDeferredOption deferred_option) {
   }
 
   if (deferred_option == STARTUP_DEFERRED && IsDeferredStartupEnabled() &&
-      sync_prefs_->GetPreferredDataTypes(registered_types_).Has(SESSIONS)) {
+      get_preferred_data_types_callback_.Run().Has(SESSIONS)) {
     if (first_start) {
       base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
           FROM_HERE,
@@ -107,12 +109,12 @@ void StartupController::StartUp(StartUpDeferredOption deferred_option) {
 
   if (start_engine_time_.is_null()) {
     start_engine_time_ = base::Time::Now();
-    start_engine_.Run();
+    start_engine_callback_.Run();
   }
 }
 
 void StartupController::TryStart() {
-  if (!can_start_.Run()) {
+  if (!can_start_callback_.Run()) {
     return;
   }
 

components/sync/driver/startup_controller.h

@@ -29,9 +29,11 @@ class StartupController {
     STARTED
   };
 
-  StartupController(const SyncPrefs* sync_prefs,
-                    base::RepeatingCallback<bool()> can_start,
-                    base::RepeatingClosure start_engine);
+  StartupController(
+      const SyncPrefs* sync_prefs,
+      base::RepeatingCallback<ModelTypeSet()> get_preferred_data_types,
+      base::RepeatingCallback<bool()> can_start,
+      base::RepeatingClosure start_engine);
   ~StartupController();
 
   // Starts up sync if it is requested by the user and preconditions are met.
@@ -54,7 +56,7 @@ class StartupController {
   // touch values that are explicitly set and reset by higher layers to
   // tell this class whether a setup UI dialog is being shown to the user.
   // See setup_in_progress_.
-  void Reset(const ModelTypeSet& registered_types);
+  void Reset();
 
   // Sets the setup in progress flag and tries to start sync if it's true.
   void SetSetupInProgress(bool setup_in_progress);
@@ -75,16 +77,16 @@ class StartupController {
 
   const SyncPrefs* sync_prefs_;
 
+  const base::RepeatingCallback<ModelTypeSet()>
+      get_preferred_data_types_callback_;
+
   // A function that can be invoked repeatedly to determine whether sync can be
   // started. |start_engine_| should not be invoked unless this returns true.
-  const base::RepeatingCallback<bool()> can_start_;
+  const base::RepeatingCallback<bool()> can_start_callback_;
 
   // The callback we invoke when it's time to call expensive
   // startup routines for the sync engine.
-  const base::RepeatingClosure start_engine_;
-
-  // Used to compute preferred_types from SyncPrefs as-needed.
-  ModelTypeSet registered_types_;
+  const base::RepeatingClosure start_engine_callback_;
 
   // If true, will bypass the FirstSetupComplete check when triggering sync
   // startup. Set in TryStartImmediately.

components/sync/driver/startup_controller_unittest.cc

@@ -18,7 +18,8 @@ namespace syncer {
 
 class StartupControllerTest : public testing::Test {
  public:
-  StartupControllerTest() : can_start_(false), started_(false) {}
+  StartupControllerTest()
+      : preferred_types_(UserTypes()), can_start_(false), started_(false) {}
 
   void SetUp() override {
     base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
@@ -28,10 +29,17 @@ class StartupControllerTest : public testing::Test {
     sync_prefs_ = std::make_unique<SyncPrefs>(&pref_service_);
     controller_ = std::make_unique<StartupController>(
         sync_prefs_.get(),
-        base::Bind(&StartupControllerTest::CanStart, base::Unretained(this)),
-        base::Bind(&StartupControllerTest::FakeStartBackend,
-                   base::Unretained(this)));
-    controller_->Reset(UserTypes());
+        base::BindRepeating(&StartupControllerTest::GetPreferredDataTypes,
+                            base::Unretained(this)),
+        base::BindRepeating(&StartupControllerTest::CanStart,
+                            base::Unretained(this)),
+        base::BindRepeating(&StartupControllerTest::FakeStartBackend,
+                            base::Unretained(this)));
+    controller_->Reset();
+  }
+
+  void SetPreferredDataTypes(const ModelTypeSet& types) {
+    preferred_types_ = types;
   }
 
   void SetCanStart(bool can_start) { can_start_ = can_start; }
@@ -58,9 +66,11 @@ class StartupControllerTest : public testing::Test {
   SyncPrefs* sync_prefs() { return sync_prefs_.get(); }
 
  private:
+  ModelTypeSet GetPreferredDataTypes() { return preferred_types_; }
   bool CanStart() { return can_start_; }
   void FakeStartBackend() { started_ = true; }
 
+  ModelTypeSet preferred_types_;
   bool can_start_;
   bool started_;
   base::MessageLoop message_loop_;
@@ -133,9 +143,7 @@ TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
   types.Remove(PROXY_TABS);
   types.Remove(TYPED_URLS);
   types.Remove(SUPERVISED_USER_SETTINGS);
-  sync_prefs()->SetKeepEverythingSynced(false);
-  sync_prefs()->SetPreferredDataTypes(UserTypes(), types);
-  controller()->Reset(UserTypes());
+  SetPreferredDataTypes(types);
 
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
@@ -187,7 +195,7 @@ TEST_F(StartupControllerTest, ResetDuringSetup) {
   controller()->SetSetupInProgress(true);
 
   // This could happen if the UI triggers a stop-syncing permanently call.
-  controller()->Reset(UserTypes());
+  controller()->Reset();
 
   // From the UI's point of view, setup is still in progress.
   EXPECT_TRUE(controller()->IsSetupInProgress());