Sync StartupController: Get rid of SetBypassSetupCompleteAndDeferredStartup

This method set some state on the StartupController which was equivalent
to calling TryStart(/*force_immediate=*/true), only that it was also
sticky, i.e. it'd also affect later TryStart(false) calls. This was
confusing and hard to reason about, and not actually required for
anything, so this CL gets rid of it.

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

components/browser_sync/profile_sync_service.cc

@@ -343,11 +343,9 @@ void ProfileSyncService::Initialize() {
 
   // Auto-start means the first time the profile starts up, sync should start up
   // immediately.
-  if (start_behavior_ == AUTO_START && IsSyncRequested() &&
-      !IsFirstSetupComplete()) {
-    startup_controller_->SetBypassSetupCompleteAndDeferredStartup();
-  }
-  startup_controller_->TryStart(/*force_immediate=*/false);
+  bool force_immediate = (start_behavior_ == AUTO_START && IsSyncRequested() &&
+                          !IsFirstSetupComplete());
+  startup_controller_->TryStart(force_immediate);
 }
 
 void ProfileSyncService::StartSyncingWithServer() {
@@ -1966,8 +1964,7 @@ void ProfileSyncService::RequestStart() {
     sync_prefs_.SetSyncRequested(true);
     NotifyObservers();
   }
-  startup_controller_->SetBypassSetupCompleteAndDeferredStartup();
-  startup_controller_->TryStart(IsSetupInProgress());
+  startup_controller_->TryStart(/*force_immediate=*/true);
 }
 
 void ProfileSyncService::ReconfigureDatatypeManager() {

components/sync/driver/startup_controller.cc

@@ -66,7 +66,6 @@ StartupController::StartupController(
       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),
       bypass_deferred_startup_(false),
       weak_factory_(this) {}
 
@@ -74,7 +73,6 @@ StartupController::~StartupController() {}
 
 void StartupController::Reset() {
   bypass_deferred_startup_ = false;
-  bypass_setup_complete_ = false;
   start_up_time_ = base::Time();
   start_engine_time_ = base::Time();
   // Don't let previous timers affect us post-reset.
@@ -119,16 +117,11 @@ void StartupController::TryStart(bool force_immediate) {
   // in progress, unless told to otherwise.
   if (force_immediate) {
     StartUp(STARTUP_IMMEDIATE);
-  } else if (sync_prefs_->IsFirstSetupComplete() || bypass_setup_complete_) {
+  } else if (sync_prefs_->IsFirstSetupComplete()) {
     StartUp(bypass_deferred_startup_ ? STARTUP_IMMEDIATE : STARTUP_DEFERRED);
   }
 }
 
-void StartupController::SetBypassSetupCompleteAndDeferredStartup() {
-  bypass_deferred_startup_ = true;
-  bypass_setup_complete_ = true;
-}
-
 void StartupController::RecordTimeDeferred() {
   DCHECK(!start_up_time_.is_null());
   base::TimeDelta time_deferred = base::Time::Now() - start_up_time_;
@@ -147,6 +140,8 @@ void StartupController::OnFallbackStartupTimerExpired() {
   RecordTimeDeferred();
   UMA_HISTOGRAM_ENUMERATION("Sync.Startup.DeferredInitTrigger",
                             TRIGGER_FALLBACK_TIMER, MAX_TRIGGER_VALUE);
+  // Once the deferred init timer has expired, don't defer startup again (until
+  // Reset() or browser restart), even if this startup attempt doesn't succeed.
   bypass_deferred_startup_ = true;
   TryStart(/*force_immediate=*/false);
 }

components/sync/driver/startup_controller.h

@@ -42,13 +42,6 @@ class StartupController {
   // |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
   // still in deferred start mode, meaning the SyncableService hasn't been
@@ -86,13 +79,8 @@ class StartupController {
   // startup routines for the sync engine.
   const base::RepeatingClosure start_engine_callback_;
 
-  // If true, will bypass the FirstSetupComplete check when triggering sync
-  // startup. Set in SetBypassSetupCompleteAndDeferredStartup.
-  bool bypass_setup_complete_;
-
   // 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.
+  // it or our deferred startup timer has expired.
   bool bypass_deferred_startup_;
 
   // The time that StartUp() is called. This is used to calculate time spent

components/sync/driver/startup_controller_unittest.cc

@@ -183,10 +183,9 @@ TEST_F(StartupControllerTest, SetupInProgressTriggerInterruptsDeferral) {
 // Test that deferred startup and the FirstSetupComplete check can be bypassed.
 TEST_F(StartupControllerTest, BypassDeferredStartupAndSetupCompleteCheck) {
   SetCanStart(true);
-  controller()->SetBypassSetupCompleteAndDeferredStartup();
   ASSERT_FALSE(sync_prefs()->IsFirstSetupComplete());
   ExpectNotStarted();
-  controller()->TryStart(/*force_immediate=*/false);
+  controller()->TryStart(/*force_immediate=*/true);
   ExpectStarted();
 }