Sync StartupController: Replace CanSyncStart by ShouldSyncStart

The new ShouldSyncStart includes the "FirstSetupComplete" check. With
this, StartupController doesn't have any knowledge of Sync setup state
anymore.
As a nice side effect, this allows us to remove SyncPrefs from
StartupController.

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

components/browser_sync/profile_sync_service.cc

@@ -231,10 +231,9 @@ void ProfileSyncService::Initialize() {
   sync_client_->Initialize();
 
   startup_controller_ = std::make_unique<syncer::StartupController>(
-      &sync_prefs_,
       base::BindRepeating(&ProfileSyncService::GetPreferredDataTypes,
                           base::Unretained(this)),
-      base::BindRepeating(&ProfileSyncService::CanSyncStart,
+      base::BindRepeating(&ProfileSyncService::ShouldSyncStart,
                           base::Unretained(this)),
       base::BindRepeating(&ProfileSyncService::StartUpSlowEngineComponents,
                           base::Unretained(this)));
@@ -451,6 +450,13 @@ void ProfileSyncService::CredentialsChanged() {
   NotifyObservers();
 }
 
+bool ProfileSyncService::ShouldSyncStart(bool bypass_first_setup_check) {
+  if (!CanSyncStart()) {
+    return false;
+  }
+  return bypass_first_setup_check || IsFirstSetupComplete();
+}
+
 void ProfileSyncService::ResetCryptoState() {
   crypto_ = std::make_unique<syncer::SyncServiceCrypto>(
       base::BindRepeating(&ProfileSyncService::NotifyObservers,

components/browser_sync/profile_sync_service.h

@@ -510,6 +510,9 @@ class ProfileSyncService : public syncer::SyncService,
   void AccountStateChanged();
   void CredentialsChanged();
 
+  // Callback for StartupController.
+  bool ShouldSyncStart(bool bypass_first_setup_check);
+
   // Destroys the |crypto_| object and creates a new one with fresh state.
   void ResetCryptoState();
 

components/sync/driver/startup_controller.cc

@@ -6,13 +6,13 @@
 
 #include <utility>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/sequenced_task_runner_handle.h"
-#include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_driver_switches.h"
 
 namespace syncer {
@@ -58,13 +58,11 @@ enum DeferredInitTrigger {
 }  // namespace
 
 StartupController::StartupController(
-    const SyncPrefs* sync_prefs,
     base::RepeatingCallback<ModelTypeSet()> get_preferred_data_types,
-    base::RepeatingCallback<bool()> can_start,
+    base::RepeatingCallback<bool(bool)> should_start,
     base::RepeatingClosure start_engine)
-    : sync_prefs_(sync_prefs),
-      get_preferred_data_types_callback_(std::move(get_preferred_data_types)),
-      can_start_callback_(std::move(can_start)),
+    : get_preferred_data_types_callback_(std::move(get_preferred_data_types)),
+      should_start_callback_(std::move(should_start)),
       start_engine_callback_(std::move(start_engine)),
       bypass_deferred_startup_(false),
       weak_factory_(this) {}
@@ -90,8 +88,8 @@ void StartupController::StartUp(StartUpDeferredOption deferred_option) {
     if (first_start) {
       base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
           FROM_HERE,
-          base::Bind(&StartupController::OnFallbackStartupTimerExpired,
-                     weak_factory_.GetWeakPtr()),
+          base::BindOnce(&StartupController::OnFallbackStartupTimerExpired,
+                         weak_factory_.GetWeakPtr()),
           GetDeferredInitDelay());
     }
     return;
@@ -104,7 +102,7 @@ void StartupController::StartUp(StartUpDeferredOption deferred_option) {
 }
 
 void StartupController::TryStart(bool force_immediate) {
-  if (!can_start_callback_.Run()) {
+  if (!should_start_callback_.Run(force_immediate)) {
     return;
   }
 
@@ -115,11 +113,8 @@ void StartupController::TryStart(bool force_immediate) {
   //   and encryption information to the UI.
   // Do not start up the sync engine if setup has not completed and isn't
   // in progress, unless told to otherwise.
-  if (force_immediate) {
-    StartUp(STARTUP_IMMEDIATE);
-  } else if (sync_prefs_->IsFirstSetupComplete()) {
-    StartUp(bypass_deferred_startup_ ? STARTUP_IMMEDIATE : STARTUP_DEFERRED);
-  }
+  StartUp((force_immediate || bypass_deferred_startup_) ? STARTUP_IMMEDIATE
+                                                        : STARTUP_DEFERRED);
 }
 
 void StartupController::RecordTimeDeferred() {

components/sync/driver/startup_controller.h

@@ -12,8 +12,6 @@
 
 namespace syncer {
 
-class SyncPrefs;
-
 // This class is used by ProfileSyncService to manage all logic and state
 // pertaining to initialization of the SyncEngine.
 class StartupController {
@@ -30,16 +28,15 @@ class StartupController {
   };
 
   StartupController(
-      const SyncPrefs* sync_prefs,
       base::RepeatingCallback<ModelTypeSet()> get_preferred_data_types,
-      base::RepeatingCallback<bool()> can_start,
+      base::RepeatingCallback<bool(bool force_immediate)> should_start,
       base::RepeatingClosure start_engine);
   ~StartupController();
 
   // Starts up sync if it is requested by the user and preconditions are met.
   // If |force_immediate| is true, this will start sync immediately, bypassing
   // deferred startup and the "first setup complete" check (but *not* the
-  // |can_start_callback_| check!).
+  // |should_start_callback_| check!).
   void TryStart(bool force_immediate);
 
   // Called when a datatype (SyncableService) has a need for sync to start
@@ -66,14 +63,12 @@ class StartupController {
   // Records time spent in deferred state with UMA histograms.
   void RecordTimeDeferred();
 
-  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_callback_;
+  // A function that can be invoked repeatedly to determine whether sync should
+  // be started. |start_engine_| should not be invoked unless this returns true.
+  const base::RepeatingCallback<bool(bool)> should_start_callback_;
 
   // The callback we invoke when it's time to call expensive
   // startup routines for the sync engine.

components/sync/driver/startup_controller_unittest.cc

@@ -6,12 +6,11 @@
 
 #include <memory>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_driver_switches.h"
-#include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
@@ -19,19 +18,16 @@ namespace syncer {
 class StartupControllerTest : public testing::Test {
  public:
   StartupControllerTest()
-      : preferred_types_(UserTypes()), can_start_(false), started_(false) {}
+      : preferred_types_(UserTypes()), should_start_(false), started_(false) {}
 
   void SetUp() override {
     base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
         switches::kSyncDeferredStartupTimeoutSeconds, "0");
 
-    SyncPrefs::RegisterProfilePrefs(pref_service_.registry());
-    sync_prefs_ = std::make_unique<SyncPrefs>(&pref_service_);
     controller_ = std::make_unique<StartupController>(
-        sync_prefs_.get(),
         base::BindRepeating(&StartupControllerTest::GetPreferredDataTypes,
                             base::Unretained(this)),
-        base::BindRepeating(&StartupControllerTest::CanStart,
+        base::BindRepeating(&StartupControllerTest::ShouldStart,
                             base::Unretained(this)),
         base::BindRepeating(&StartupControllerTest::FakeStartBackend,
                             base::Unretained(this)));
@@ -42,7 +38,7 @@ class StartupControllerTest : public testing::Test {
     preferred_types_ = types;
   }
 
-  void SetCanStart(bool can_start) { can_start_ = can_start; }
+  void SetShouldStart(bool should_start) { should_start_ = should_start; }
 
   void ExpectStarted() {
     EXPECT_TRUE(started());
@@ -63,44 +59,38 @@ class StartupControllerTest : public testing::Test {
   bool started() const { return started_; }
   void clear_started() { started_ = false; }
   StartupController* controller() { return controller_.get(); }
-  SyncPrefs* sync_prefs() { return sync_prefs_.get(); }
 
  private:
   ModelTypeSet GetPreferredDataTypes() { return preferred_types_; }
-  bool CanStart() { return can_start_; }
+  bool ShouldStart(bool force_immediate) { return should_start_; }
   void FakeStartBackend() { started_ = true; }
 
   ModelTypeSet preferred_types_;
-  bool can_start_;
+  bool should_start_;
   bool started_;
   base::MessageLoop message_loop_;
-  sync_preferences::TestingPrefServiceSyncable pref_service_;
-  std::unique_ptr<SyncPrefs> sync_prefs_;
   std::unique_ptr<StartupController> controller_;
 };
 
-// Test that sync doesn't start if setup is not in progress or complete.
-TEST_F(StartupControllerTest, NoSetupComplete) {
+// Test that sync doesn't start if the "should sync start" callback returns
+// false.
+TEST_F(StartupControllerTest, ShouldNotStart) {
   controller()->TryStart(/*force_immediate=*/false);
   ExpectNotStarted();
 
-  SetCanStart(true);
-  controller()->TryStart(/*force_immediate=*/false);
+  controller()->TryStart(/*force_immediate=*/true);
   ExpectNotStarted();
 }
 
-// Test that sync defers if first setup is complete.
-TEST_F(StartupControllerTest, DefersAfterFirstSetupComplete) {
-  sync_prefs()->SetFirstSetupComplete();
-  SetCanStart(true);
+TEST_F(StartupControllerTest, DefersByDefault) {
+  SetShouldStart(true);
   controller()->TryStart(/*force_immediate=*/false);
   ExpectStartDeferred();
 }
 
 // Test that a data type triggering startup starts sync immediately.
 TEST_F(StartupControllerTest, NoDeferralDataTypeTrigger) {
-  sync_prefs()->SetFirstSetupComplete();
-  SetCanStart(true);
+  SetShouldStart(true);
   controller()->OnDataTypeRequestsSyncStartup(SESSIONS);
   ExpectStarted();
 }
@@ -108,8 +98,7 @@ TEST_F(StartupControllerTest, NoDeferralDataTypeTrigger) {
 // Test that a data type trigger interrupts the deferral timer and starts
 // sync immediately.
 TEST_F(StartupControllerTest, DataTypeTriggerInterruptsDeferral) {
-  sync_prefs()->SetFirstSetupComplete();
-  SetCanStart(true);
+  SetShouldStart(true);
   controller()->TryStart(/*force_immediate=*/false);
   ExpectStartDeferred();
 
@@ -123,11 +112,10 @@ TEST_F(StartupControllerTest, DataTypeTriggerInterruptsDeferral) {
   EXPECT_FALSE(started());
 }
 
-// Test that the fallback timer starts sync in the event all
-// conditions are met and no data type requests sync.
+// Test that the fallback timer starts sync in the event all conditions are met
+// and no data type requests sync.
 TEST_F(StartupControllerTest, FallbackTimer) {
-  sync_prefs()->SetFirstSetupComplete();
-  SetCanStart(true);
+  SetShouldStart(true);
   controller()->TryStart(/*force_immediate=*/false);
   ExpectStartDeferred();
 
@@ -145,8 +133,7 @@ TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
   types.Remove(SUPERVISED_USER_SETTINGS);
   SetPreferredDataTypes(types);
 
-  sync_prefs()->SetFirstSetupComplete();
-  SetCanStart(true);
+  SetShouldStart(true);
   controller()->TryStart(/*force_immediate=*/false);
   ExpectStarted();
 }
@@ -160,19 +147,18 @@ TEST_F(StartupControllerTest, FallbackTimerWaits) {
   ExpectNotStarted();
 }
 
-// Test that sync starts immediately when setup in progress is true.
-TEST_F(StartupControllerTest, NoDeferralSetupInProgressTrigger) {
-  sync_prefs()->SetFirstSetupComplete();
-  SetCanStart(true);
+// Test that sync starts immediately when told to do so.
+TEST_F(StartupControllerTest, NoDeferralIfForceImmediate) {
+  SetShouldStart(true);
+  ExpectNotStarted();
   controller()->TryStart(/*force_immediate=*/true);
   ExpectStarted();
 }
 
-// Test that setup in progress being set to true interrupts the deferral timer
-// and starts sync immediately.
-TEST_F(StartupControllerTest, SetupInProgressTriggerInterruptsDeferral) {
-  sync_prefs()->SetFirstSetupComplete();
-  SetCanStart(true);
+// Test that setting |force_immediate| interrupts the deferral timer and starts
+// sync immediately.
+TEST_F(StartupControllerTest, ForceImmediateInterruptsDeferral) {
+  SetShouldStart(true);
   controller()->TryStart(/*force_immediate=*/false);
   ExpectStartDeferred();
 
@@ -180,13 +166,4 @@ TEST_F(StartupControllerTest, SetupInProgressTriggerInterruptsDeferral) {
   ExpectStarted();
 }
 
-// Test that deferred startup and the FirstSetupComplete check can be bypassed.
-TEST_F(StartupControllerTest, BypassDeferredStartupAndSetupCompleteCheck) {
-  SetCanStart(true);
-  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
-  ExpectNotStarted();
-  controller()->TryStart(/*force_immediate=*/true);
-  ExpectStarted();
-}
-
 }  // namespace syncer