Sync StartupController: Get rid of TryStartImmediately

All it did was setting some sticky state and then calling TryStart(). So
let's at least make that explicit by having clients call a state setter
plus TryStart().

This also lets us rename the "setup_in_progress" param for TryStart()
to "force_immediate" which makes a lot more sense.

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

components/browser_sync/profile_sync_service.cc

@@ -345,10 +345,9 @@ void ProfileSyncService::Initialize() {
   // immediately.
   if (start_behavior_ == AUTO_START && IsSyncRequested() &&
       !IsFirstSetupComplete()) {
-    startup_controller_->TryStartImmediately();
-  } else {
-    startup_controller_->TryStart(/*setup_in_progress=*/false);
+    startup_controller_->SetBypassSetupCompleteAndDeferredStartup();
   }
+  startup_controller_->TryStart(/*force_immediate=*/false);
 }
 
 void ProfileSyncService::StartSyncingWithServer() {
@@ -1242,7 +1241,7 @@ ProfileSyncService::GetSetupInProgressHandle() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (++outstanding_setup_in_progress_handles_ == 1) {
-    startup_controller_->TryStart(/*setup_in_progress=*/true);
+    startup_controller_->TryStart(/*force_immediate=*/true);
 
     NotifyObservers();
   }
@@ -1967,7 +1966,8 @@ void ProfileSyncService::RequestStart() {
     sync_prefs_.SetSyncRequested(true);
     NotifyObservers();
   }
-  startup_controller_->TryStartImmediately();
+  startup_controller_->SetBypassSetupCompleteAndDeferredStartup();
+  startup_controller_->TryStart(IsSetupInProgress());
 }
 
 void ProfileSyncService::ReconfigureDatatypeManager() {

components/sync/driver/startup_controller.cc

@@ -67,13 +67,13 @@ StartupController::StartupController(
       can_start_callback_(std::move(can_start)),
       start_engine_callback_(std::move(start_engine)),
       bypass_setup_complete_(false),
-      received_start_request_(false),
+      bypass_deferred_startup_(false),
       weak_factory_(this) {}
 
 StartupController::~StartupController() {}
 
 void StartupController::Reset() {
-  received_start_request_ = false;
+  bypass_deferred_startup_ = false;
   bypass_setup_complete_ = false;
   start_up_time_ = base::Time();
   start_engine_time_ = base::Time();
@@ -105,7 +105,7 @@ void StartupController::StartUp(StartUpDeferredOption deferred_option) {
   }
 }
 
-void StartupController::TryStart(bool setup_in_progress) {
+void StartupController::TryStart(bool force_immediate) {
   if (!can_start_callback_.Run()) {
     return;
   }
@@ -117,17 +117,16 @@ void StartupController::TryStart(bool setup_in_progress) {
   //   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 (setup_in_progress) {
+  if (force_immediate) {
     StartUp(STARTUP_IMMEDIATE);
   } else if (sync_prefs_->IsFirstSetupComplete() || bypass_setup_complete_) {
-    StartUp(received_start_request_ ? STARTUP_IMMEDIATE : STARTUP_DEFERRED);
+    StartUp(bypass_deferred_startup_ ? STARTUP_IMMEDIATE : STARTUP_DEFERRED);
   }
 }
 
-void StartupController::TryStartImmediately() {
-  received_start_request_ = true;
+void StartupController::SetBypassSetupCompleteAndDeferredStartup() {
+  bypass_deferred_startup_ = true;
   bypass_setup_complete_ = true;
-  TryStart(/*setup_in_progress=*/false);
 }
 
 void StartupController::RecordTimeDeferred() {
@@ -148,8 +147,8 @@ void StartupController::OnFallbackStartupTimerExpired() {
   RecordTimeDeferred();
   UMA_HISTOGRAM_ENUMERATION("Sync.Startup.DeferredInitTrigger",
                             TRIGGER_FALLBACK_TIMER, MAX_TRIGGER_VALUE);
-  received_start_request_ = true;
-  TryStart(/*setup_in_progress=*/false);
+  bypass_deferred_startup_ = true;
+  TryStart(/*force_immediate=*/false);
 }
 
 StartupController::State StartupController::GetState() const {
@@ -183,8 +182,8 @@ void StartupController::OnDataTypeRequestsSyncStartup(ModelType type) {
     UMA_HISTOGRAM_ENUMERATION("Sync.Startup.DeferredInitTrigger",
                               TRIGGER_DATA_TYPE_REQUEST, MAX_TRIGGER_VALUE);
   }
-  received_start_request_ = true;
-  TryStart(/*setup_in_progress=*/false);
+  bypass_deferred_startup_ = true;
+  TryStart(/*force_immediate=*/false);
 }
 
 }  // namespace syncer

components/sync/driver/startup_controller.h

@@ -37,15 +37,17 @@ class StartupController {
   ~StartupController();
 
   // Starts up sync if it is requested by the user and preconditions are met.
-  // If |setup_in_progress| is true, this will start sync immediately, bypassing
-  // deferred startup and the "first setup complete" check.
-  // TODO(crbug.com/854978): Rename "setup_in_progress" to "force_immediate" or
-  // something like that after TryStartImmediately is gone.
-  void TryStart(bool setup_in_progress);
-
-  // Same as TryStart() above, but bypasses deferred startup and the first setup
-  // complete check.
-  void TryStartImmediately();
+  // 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!).
+  void TryStart(bool force_immediate);
+
+  // Tells the controller to bypass deferred startup and the "first setup
+  // complete" check. After this, any TryStart() calls (until a Reset()) will
+  // cause immediate startup.
+  // TODO(crbug.com/854978): See if we can get rid of this and just call
+  // TryStart(true) instead.
+  void SetBypassSetupCompleteAndDeferredStartup();
 
   // Called when a datatype (SyncableService) has a need for sync to start
   // ASAP, presumably because a local change event has occurred but we're
@@ -85,13 +87,13 @@ class StartupController {
   const base::RepeatingClosure start_engine_callback_;
 
   // If true, will bypass the FirstSetupComplete check when triggering sync
-  // startup. Set in TryStartImmediately.
+  // startup. Set in SetBypassSetupCompleteAndDeferredStartup.
   bool bypass_setup_complete_;
 
-  // True if we should start sync ASAP because either a data type has
-  // requested it, or TryStartImmediately was called, or our deferred startup
-  // timer has expired.
-  bool received_start_request_;
+  // True if we should start sync ASAP because either a data type has requested
+  // it, or SetBypassSetupCompleteAndDeferredStartup was called, or our deferred
+  // startup timer has expired.
+  bool bypass_deferred_startup_;
 
   // The time that StartUp() is called. This is used to calculate time spent
   // in the deferred state; that is, after StartUp and before invoking the

components/sync/driver/startup_controller_unittest.cc

@@ -81,11 +81,11 @@ class StartupControllerTest : public testing::Test {
 
 // Test that sync doesn't start if setup is not in progress or complete.
 TEST_F(StartupControllerTest, NoSetupComplete) {
-  controller()->TryStart(/*setup_in_progress=*/false);
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectNotStarted();
 
   SetCanStart(true);
-  controller()->TryStart(/*setup_in_progress=*/false);
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectNotStarted();
 }
 
@@ -93,7 +93,7 @@ TEST_F(StartupControllerTest, NoSetupComplete) {
 TEST_F(StartupControllerTest, DefersAfterFirstSetupComplete) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart(/*setup_in_progress=*/false);
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectStartDeferred();
 }
 
@@ -110,7 +110,7 @@ TEST_F(StartupControllerTest, NoDeferralDataTypeTrigger) {
 TEST_F(StartupControllerTest, DataTypeTriggerInterruptsDeferral) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart(/*setup_in_progress=*/false);
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectStartDeferred();
 
   controller()->OnDataTypeRequestsSyncStartup(SESSIONS);
@@ -128,7 +128,7 @@ TEST_F(StartupControllerTest, DataTypeTriggerInterruptsDeferral) {
 TEST_F(StartupControllerTest, FallbackTimer) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart(/*setup_in_progress=*/false);
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectStartDeferred();
 
   base::RunLoop().RunUntilIdle();
@@ -147,14 +147,14 @@ TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
 
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart(/*setup_in_progress=*/false);
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectStarted();
 }
 
 // Sanity check that the fallback timer doesn't fire before startup
 // conditions are met.
 TEST_F(StartupControllerTest, FallbackTimerWaits) {
-  controller()->TryStart(/*setup_in_progress=*/false);
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectNotStarted();
   base::RunLoop().RunUntilIdle();
   ExpectNotStarted();
@@ -164,7 +164,7 @@ TEST_F(StartupControllerTest, FallbackTimerWaits) {
 TEST_F(StartupControllerTest, NoDeferralSetupInProgressTrigger) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart(/*setup_in_progress=*/true);
+  controller()->TryStart(/*force_immediate=*/true);
   ExpectStarted();
 }
 
@@ -173,17 +173,20 @@ TEST_F(StartupControllerTest, NoDeferralSetupInProgressTrigger) {
 TEST_F(StartupControllerTest, SetupInProgressTriggerInterruptsDeferral) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart(/*setup_in_progress=*/false);
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectStartDeferred();
 
-  controller()->TryStart(/*setup_in_progress=*/true);
+  controller()->TryStart(/*force_immediate=*/true);
   ExpectStarted();
 }
 
-// Test that immediate startup can be forced.
-TEST_F(StartupControllerTest, ForceImmediateStartup) {
+// Test that deferred startup and the FirstSetupComplete check can be bypassed.
+TEST_F(StartupControllerTest, BypassDeferredStartupAndSetupCompleteCheck) {
   SetCanStart(true);
-  controller()->TryStartImmediately();
+  controller()->SetBypassSetupCompleteAndDeferredStartup();
+  ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
+  ExpectNotStarted();
+  controller()->TryStart(/*force_immediate=*/false);
   ExpectStarted();
 }