Remove SyncService::TransportState::WAITING_FOR_START_REQUEST

After https://crrev.com/c/1430089, this state is not meaningful
anymore. Yay, one less!

Semi-relatedly, this CL also moves some NotifyObservers() calls, to
avoid notifying while we're in an intermediate (and thus potentially
invalid) state.

Bug: 839834, 935908
Change-Id: I6093ba3bd44452990ba9b0b8926b3c6e308c90ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1481418Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638482}

chrome/browser/ui/webui/settings/people_handler_unittest.cc

@@ -443,8 +443,8 @@ TEST_F(PeopleHandlerTest,
       .WillByDefault(Return(true));
   // Sync engine is stopped initially, and will start up.
   ON_CALL(*mock_pss_, GetTransportState())
-      .WillByDefault(Return(
-          syncer::SyncService::TransportState::WAITING_FOR_START_REQUEST));
+      .WillByDefault(
+          Return(syncer::SyncService::TransportState::START_DEFERRED));
   EXPECT_CALL(*mock_pss_->GetUserSettingsMock(), SetSyncRequested(true));
   SetDefaultExpectationsForConfigPage();
 

components/browser_sync/profile_sync_service.cc

@@ -538,8 +538,8 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
 void ProfileSyncService::Shutdown() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  ShutdownImpl(syncer::BROWSER_SHUTDOWN);
   NotifyShutdown();
+  ShutdownImpl(syncer::BROWSER_SHUTDOWN);
 
   // All observers must be gone now: All KeyedServices should have unregistered
   // their observers already before, in their own Shutdown(), and all others
@@ -701,9 +701,12 @@ syncer::SyncService::TransportState ProfileSyncService::GetTransportState()
 
   if (!engine_initialized_) {
     switch (startup_controller_->GetState()) {
+        // TODO(crbug.com/935523): If the engine is allowed to start, then we
+        // should generally have kicked off the startup process already, so
+        // NOT_STARTED should be impossible. But we can temporarily be in this
+        // state between shutting down and starting up again (e.g. during the
+        // NotifyObservers() call in ShutdownImpl()).
       case syncer::StartupController::State::NOT_STARTED:
-        DCHECK(!engine_);
-        return TransportState::WAITING_FOR_START_REQUEST;
       case syncer::StartupController::State::STARTING_DEFERRED:
         DCHECK(!engine_);
         return TransportState::START_DEFERRED;
@@ -1523,8 +1526,6 @@ void ProfileSyncService::OnFirstSetupCompletePrefChange(
 
 void ProfileSyncService::OnSyncRequestedPrefChange(bool is_sync_requested) {
   if (is_sync_requested) {
-    NotifyObservers();
-
     // If the Sync engine was already initialized (probably running in transport
     // mode), just reconfigure.
     if (engine_initialized_) {
@@ -1534,6 +1535,8 @@ void ProfileSyncService::OnSyncRequestedPrefChange(bool is_sync_requested) {
       // reasons remaining, in which case this will effectively do nothing.
       startup_controller_->TryStart(/*force_immediate=*/true);
     }
+
+    NotifyObservers();
   } else {
     // This will notify the observers.
     if (is_stopping_and_clearing_) {

components/browser_sync/profile_sync_service_startup_unittest.cc

@@ -194,10 +194,8 @@ TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) {
 
   SimulateTestUserSignin();
 
-  // Now we're signed in, so the engine can start. There's already a setup in
-  // progress, so we don't go into the WAITING_FOR_START_REQUEST state. Engine
-  // initialization is immediate in this test, so we also bypass the
-  // INITIALIZING state.
+  // Now we're signed in, so the engine can start. Engine initialization is
+  // immediate in this test, so we bypass the INITIALIZING state.
   EXPECT_TRUE(sync_service()->IsEngineInitialized());
   EXPECT_EQ(syncer::SyncService::DISABLE_REASON_NONE,
             sync_service()->GetDisableReasons());

components/sync/driver/about_sync_util.cc

@@ -206,8 +206,6 @@ std::string GetTransportStateString(syncer::SyncService::TransportState state) {
   switch (state) {
     case syncer::SyncService::TransportState::DISABLED:
       return "Disabled";
-    case syncer::SyncService::TransportState::WAITING_FOR_START_REQUEST:
-      return "Waiting for start request";
     case syncer::SyncService::TransportState::START_DEFERRED:
       return "Start deferred";
     case syncer::SyncService::TransportState::INITIALIZING:

components/sync/driver/sync_service.cc

@@ -30,7 +30,6 @@ bool SyncService::CanSyncFeatureStart() const {
 bool SyncService::IsEngineInitialized() const {
   switch (GetTransportState()) {
     case TransportState::DISABLED:
-    case TransportState::WAITING_FOR_START_REQUEST:
     case TransportState::START_DEFERRED:
     case TransportState::INITIALIZING:
       return false;
@@ -49,7 +48,6 @@ bool SyncService::IsSyncFeatureActive() const {
   }
   switch (GetTransportState()) {
     case TransportState::DISABLED:
-    case TransportState::WAITING_FOR_START_REQUEST:
     case TransportState::START_DEFERRED:
     case TransportState::INITIALIZING:
     case TransportState::PENDING_DESIRED_CONFIGURATION:

components/sync/driver/sync_service.h

@@ -85,15 +85,6 @@ class SyncService : public KeyedService {
     // Sync is inactive, e.g. due to enterprise policy, or simply because there
     // is no authenticated user.
     DISABLED,
-    // Sync can start in principle, but nothing has prodded it to actually do it
-    // yet. Note that during subsequent browser startups, Sync starts
-    // automatically, i.e. no prod is necessary, but during the first start Sync
-    // does need a kick. This usually happens via starting (not finishing!) the
-    // initial setup, or via a call to SyncUserSettings::SetSyncRequested.
-    // TODO(crbug.com/839834): Check whether this state is necessary, or if Sync
-    // can just always start up if all conditions are fulfilled (that's what
-    // happens in practice anyway).
-    WAITING_FOR_START_REQUEST,
     // Sync's startup was deferred, so that it doesn't slow down browser
     // startup. Once the deferral time (usually 10s) expires, or something
     // requests immediate startup, Sync will actually start.

components/sync/driver/sync_service_utils.cc

@@ -45,7 +45,6 @@ UploadState GetUploadToGoogleState(const SyncService* sync_service,
     case SyncService::TransportState::DISABLED:
       return UploadState::NOT_ACTIVE;
 
-    case SyncService::TransportState::WAITING_FOR_START_REQUEST:
     case SyncService::TransportState::START_DEFERRED:
     case SyncService::TransportState::INITIALIZING:
     case SyncService::TransportState::PENDING_DESIRED_CONFIGURATION:

components/sync/driver/sync_service_utils_unittest.cc

@@ -30,7 +30,7 @@ TEST(SyncServiceUtilsTest, UploadToGoogleDisabledIfSyncNotAllowed) {
   // disabled anymore (though not necessarily active yet).
   service.SetDisableReasons(syncer::SyncService::DISABLE_REASON_NONE);
   service.SetTransportState(
-      syncer::SyncService::TransportState::WAITING_FOR_START_REQUEST);
+      syncer::SyncService::TransportState::START_DEFERRED);
 
   EXPECT_NE(UploadState::NOT_ACTIVE,
             GetUploadToGoogleState(&service, syncer::BOOKMARKS));
@@ -41,7 +41,7 @@ TEST(SyncServiceUtilsTest,
   TestSyncService service;
   service.SetDisableReasons(syncer::SyncService::DISABLE_REASON_NONE);
   service.SetTransportState(
-      syncer::SyncService::TransportState::WAITING_FOR_START_REQUEST);
+      syncer::SyncService::TransportState::START_DEFERRED);
   service.SetPreferredDataTypes(ProtocolTypes());
   service.SetActiveDataTypes(ProtocolTypes());
   service.SetEmptyLastCycleSnapshot();