Sync: Remove SetupInProgress state from StartupController

Before this CL, StartupController had an explicit setup_in_progress_
flag, which was set and cleared by ProfileSyncService. This was
unnecessary, since
a) ProfileSyncService itself knows that state anyway (hence no need to
   forward IsSetupInProgress queries to the StartupController), and
b) StartupController only needs it within TryStart, where we can just
   pass it in.

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

components/browser_sync/profile_sync_service.cc

@@ -347,7 +347,7 @@ void ProfileSyncService::Initialize() {
       !IsFirstSetupComplete()) {
     startup_controller_->TryStartImmediately();
   } else {
-    startup_controller_->TryStart();
+    startup_controller_->TryStart(/*setup_in_progress=*/false);
   }
 }
 
@@ -434,7 +434,7 @@ void ProfileSyncService::AccountStateChanged() {
     DCHECK(!engine_);
   } else {
     DCHECK(!engine_);
-    startup_controller_->TryStart();
+    startup_controller_->TryStart(IsSetupInProgress());
   }
 }
 
@@ -1024,7 +1024,7 @@ void ProfileSyncService::OnActionableError(
       break;
     case syncer::RESET_LOCAL_SYNC_DATA:
       ShutdownImpl(syncer::DISABLE_SYNC);
-      startup_controller_->TryStart();
+      startup_controller_->TryStart(IsSetupInProgress());
       UMA_HISTOGRAM_ENUMERATION(
           "Sync.ClearServerDataEvents",
           syncer::CLEAR_SERVER_DATA_RESET_LOCAL_DATA_RECEIVED,
@@ -1054,7 +1054,7 @@ void ProfileSyncService::OnClearServerDataDone() {
   // Shutdown sync, delete the Directory, then restart, restoring the cached
   // nigori state.
   ShutdownImpl(syncer::DISABLE_SYNC);
-  startup_controller_->TryStart();
+  startup_controller_->TryStart(IsSetupInProgress());
   UMA_HISTOGRAM_ENUMERATION("Sync.ClearServerDataEvents",
                             syncer::CLEAR_SERVER_DATA_SUCCEEDED,
                             syncer::CLEAR_SERVER_DATA_MAX);
@@ -1207,7 +1207,7 @@ std::string ProfileSyncService::GetEngineInitializationStateString() const {
 
 bool ProfileSyncService::IsSetupInProgress() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return startup_controller_->IsSetupInProgress();
+  return outstanding_setup_in_progress_handles_ > 0;
 }
 
 bool ProfileSyncService::QueryDetailedSyncStatus(
@@ -1234,7 +1234,7 @@ bool ProfileSyncService::CanConfigureDataTypes() const {
 
 bool ProfileSyncService::IsFirstSetupInProgress() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return !IsFirstSetupComplete() && startup_controller_->IsSetupInProgress();
+  return !IsFirstSetupComplete() && IsSetupInProgress();
 }
 
 std::unique_ptr<syncer::SyncSetupInProgressHandle>
@@ -1242,8 +1242,7 @@ ProfileSyncService::GetSetupInProgressHandle() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (++outstanding_setup_in_progress_handles_ == 1) {
-    DCHECK(!startup_controller_->IsSetupInProgress());
-    startup_controller_->SetSetupInProgress(true);
+    startup_controller_->TryStart(/*setup_in_progress=*/true);
 
     NotifyObservers();
   }
@@ -1716,7 +1715,7 @@ void ProfileSyncService::OnSyncManagedPrefChange(bool is_sync_managed) {
     StopImpl(CLEAR_DATA);
   } else {
     // Sync is no longer disabled by policy. Try starting it up if appropriate.
-    startup_controller_->TryStart();
+    startup_controller_->TryStart(IsSetupInProgress());
   }
 }
 
@@ -2184,9 +2183,6 @@ void ProfileSyncService::OnSetupInProgressHandleDestroyed() {
   if (--outstanding_setup_in_progress_handles_ != 0)
     return;
 
-  DCHECK(startup_controller_->IsSetupInProgress());
-  startup_controller_->SetSetupInProgress(false);
-
   if (IsEngineInitialized())
     ReconfigureDatatypeManager();
   NotifyObservers();

components/sync/driver/startup_controller.cc

@@ -68,7 +68,6 @@ StartupController::StartupController(
       start_engine_callback_(std::move(start_engine)),
       bypass_setup_complete_(false),
       received_start_request_(false),
-      setup_in_progress_(false),
       weak_factory_(this) {}
 
 StartupController::~StartupController() {}
@@ -82,13 +81,6 @@ void StartupController::Reset() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
-void StartupController::SetSetupInProgress(bool setup_in_progress) {
-  setup_in_progress_ = setup_in_progress;
-  if (setup_in_progress_) {
-    TryStart();
-  }
-}
-
 void StartupController::StartUp(StartUpDeferredOption deferred_option) {
   const bool first_start = start_up_time_.is_null();
   if (first_start) {
@@ -113,7 +105,7 @@ void StartupController::StartUp(StartUpDeferredOption deferred_option) {
   }
 }
 
-void StartupController::TryStart() {
+void StartupController::TryStart(bool setup_in_progress) {
   if (!can_start_callback_.Run()) {
     return;
   }
@@ -125,7 +117,7 @@ void StartupController::TryStart() {
   //   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 (setup_in_progress) {
     StartUp(STARTUP_IMMEDIATE);
   } else if (sync_prefs_->IsFirstSetupComplete() || bypass_setup_complete_) {
     StartUp(received_start_request_ ? STARTUP_IMMEDIATE : STARTUP_DEFERRED);
@@ -135,7 +127,7 @@ void StartupController::TryStart() {
 void StartupController::TryStartImmediately() {
   received_start_request_ = true;
   bypass_setup_complete_ = true;
-  TryStart();
+  TryStart(/*setup_in_progress=*/false);
 }
 
 void StartupController::RecordTimeDeferred() {
@@ -157,7 +149,7 @@ void StartupController::OnFallbackStartupTimerExpired() {
   UMA_HISTOGRAM_ENUMERATION("Sync.Startup.DeferredInitTrigger",
                             TRIGGER_FALLBACK_TIMER, MAX_TRIGGER_VALUE);
   received_start_request_ = true;
-  TryStart();
+  TryStart(/*setup_in_progress=*/false);
 }
 
 StartupController::State StartupController::GetState() const {
@@ -192,7 +184,7 @@ void StartupController::OnDataTypeRequestsSyncStartup(ModelType type) {
                               TRIGGER_DATA_TYPE_REQUEST, MAX_TRIGGER_VALUE);
   }
   received_start_request_ = true;
-  TryStart();
+  TryStart(/*setup_in_progress=*/false);
 }
 
 }  // namespace syncer

components/sync/driver/startup_controller.h

@@ -37,7 +37,11 @@ class StartupController {
   ~StartupController();
 
   // Starts up sync if it is requested by the user and preconditions are met.
-  void TryStart();
+  // 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.
@@ -52,18 +56,10 @@ class StartupController {
 
   // Prepares this object for a new attempt to start sync, forgetting
   // whether or not preconditions were previously met.
-  // NOTE: This resets internal state managed by this class, but does not
-  // 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();
 
-  // Sets the setup in progress flag and tries to start sync if it's true.
-  void SetSetupInProgress(bool setup_in_progress);
-
   State GetState() const;
 
-  bool IsSetupInProgress() const { return setup_in_progress_; }
   base::Time start_engine_time() const { return start_engine_time_; }
 
  private:
@@ -103,13 +99,6 @@ class StartupController {
   // startup has been triggered.
   base::Time start_up_time_;
 
-  // If |true|, there is setup UI visible so we should not start downloading
-  // data types.
-  // Note: this is explicitly controlled by higher layers (UI) and is meant to
-  // reflect what the UI claims the setup state to be. Therefore, only set this
-  // due to explicit requests to do so via SetSetupInProgress.
-  bool setup_in_progress_;
-
   // The time at which we invoked the |start_engine_| callback. If this is
   // non-null, then |start_engine_| shouldn't be called again.
   base::Time start_engine_time_;

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();
+  controller()->TryStart(/*setup_in_progress=*/false);
   ExpectNotStarted();
 
   SetCanStart(true);
-  controller()->TryStart();
+  controller()->TryStart(/*setup_in_progress=*/false);
   ExpectNotStarted();
 }
 
@@ -93,7 +93,7 @@ TEST_F(StartupControllerTest, NoSetupComplete) {
 TEST_F(StartupControllerTest, DefersAfterFirstSetupComplete) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart();
+  controller()->TryStart(/*setup_in_progress=*/false);
   ExpectStartDeferred();
 }
 
@@ -110,7 +110,7 @@ TEST_F(StartupControllerTest, NoDeferralDataTypeTrigger) {
 TEST_F(StartupControllerTest, DataTypeTriggerInterruptsDeferral) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart();
+  controller()->TryStart(/*setup_in_progress=*/false);
   ExpectStartDeferred();
 
   controller()->OnDataTypeRequestsSyncStartup(SESSIONS);
@@ -128,7 +128,7 @@ TEST_F(StartupControllerTest, DataTypeTriggerInterruptsDeferral) {
 TEST_F(StartupControllerTest, FallbackTimer) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart();
+  controller()->TryStart(/*setup_in_progress=*/false);
   ExpectStartDeferred();
 
   base::RunLoop().RunUntilIdle();
@@ -147,14 +147,14 @@ TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
 
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart();
+  controller()->TryStart(/*setup_in_progress=*/false);
   ExpectStarted();
 }
 
 // Sanity check that the fallback timer doesn't fire before startup
 // conditions are met.
 TEST_F(StartupControllerTest, FallbackTimerWaits) {
-  controller()->TryStart();
+  controller()->TryStart(/*setup_in_progress=*/false);
   ExpectNotStarted();
   base::RunLoop().RunUntilIdle();
   ExpectNotStarted();
@@ -164,7 +164,7 @@ TEST_F(StartupControllerTest, FallbackTimerWaits) {
 TEST_F(StartupControllerTest, NoDeferralSetupInProgressTrigger) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->SetSetupInProgress(true);
+  controller()->TryStart(/*setup_in_progress=*/true);
   ExpectStarted();
 }
 
@@ -173,10 +173,10 @@ TEST_F(StartupControllerTest, NoDeferralSetupInProgressTrigger) {
 TEST_F(StartupControllerTest, SetupInProgressTriggerInterruptsDeferral) {
   sync_prefs()->SetFirstSetupComplete();
   SetCanStart(true);
-  controller()->TryStart();
+  controller()->TryStart(/*setup_in_progress=*/false);
   ExpectStartDeferred();
 
-  controller()->SetSetupInProgress(true);
+  controller()->TryStart(/*setup_in_progress=*/true);
   ExpectStarted();
 }
 
@@ -187,18 +187,4 @@ TEST_F(StartupControllerTest, ForceImmediateStartup) {
   ExpectStarted();
 }
 
-// Test that setup-in-progress tracking is persistent across a Reset.
-TEST_F(StartupControllerTest, ResetDuringSetup) {
-  SetCanStart(true);
-
-  // Simulate UI telling us setup is in progress.
-  controller()->SetSetupInProgress(true);
-
-  // This could happen if the UI triggers a stop-syncing permanently call.
-  controller()->Reset();
-
-  // From the UI's point of view, setup is still in progress.
-  EXPECT_TRUE(controller()->IsSetupInProgress());
-}
-
 }  // namespace syncer