Replace SyncService::State::WAITING_FOR_CONSENT by PENDING_DESIRED_CONFIGURATION

This makes more sense to describe the actual state of the SyncService (rather
than the reason for it). Presence or absence of consent isn't part of the
SyncService state.
It'll also make it easier to decouple Sync-the-machinery from
Sync-the-feature, since the concept of consent doesn't apply for the
machinery.

In practice, WAITING_FOR_CONSENT and PENDING_DESIRED_CONFIGURATION are almost
equivalent, but they are defined in different terms.

Bug: 839834, 856179
Change-Id: I5fc20a1359bd302cae233a5bce9b25bda2681e2b
Reviewed-on: https://chromium-review.googlesource.com/1141804
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576091}

components/browser_sync/profile_sync_service.cc

@@ -805,27 +805,26 @@ syncer::SyncService::State ProfileSyncService::GetState() const {
   // The DataTypeManager gets created once the engine is initialized.
   DCHECK(data_type_manager_);
 
-  if (!IsFirstSetupComplete()) {
-    DCHECK(data_type_manager_->state() != DataTypeManager::CONFIGURED);
-    return State::WAITING_FOR_CONSENT;
-  }
-
-#if DCHECK_IS_ON()
-  // At this point we should generally be able to configure our data types,
-  // except if a setup is currently in progress (i.e. the Sync settings UI is
-  // being shown). Note that if a setup is started after the data types have
-  // been configured, then they'll stay configured even though
-  // CanConfigureDataTypes will be false.
-  if (!IsSetupInProgress()) {
-    DCHECK(CanConfigureDataTypes());
-    // After data types *can* be configured, they must actually get configured,
-    // so the DataTypeManager should have gotten out of the initial STOPPED
-    // state. It can only go back to STOPPED in case of unrecoverable errors,
-    // for which we already checked above.
-    DCHECK_NE(data_type_manager_->state(), DataTypeManager::STOPPED);
-    DCHECK(IsSyncActive());
-  }
-#endif  // DCHECK_IS_ON()
+  // At this point we should usually be able to configure our data types (and
+  // once the data types can be configured, they must actually get configured).
+  // However, if the initial setup hasn't been completed, then we can't
+  // configure the data types. Also if a later (non-initial) setup happens to be
+  // in progress, we won't configure them right now.
+  if (data_type_manager_->state() == DataTypeManager::STOPPED) {
+    DCHECK(!CanConfigureDataTypes());
+    return State::PENDING_DESIRED_CONFIGURATION;
+  }
+
+  // The DataTypeManager shouldn't get configured (i.e. leave the STOPPED state)
+  // before the initial setup is complete.
+  DCHECK(IsFirstSetupComplete());
+
+  // Note that if a setup is started after the data types have been configured,
+  // then they'll stay configured even though CanConfigureDataTypes will be
+  // false.
+  DCHECK(CanConfigureDataTypes() || IsSetupInProgress());
+
+  DCHECK(IsSyncActive());
 
   if (data_type_manager_->state() != DataTypeManager::CONFIGURED) {
     return State::CONFIGURING;

components/browser_sync/profile_sync_service_startup_unittest.cc

@@ -214,7 +214,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) {
   EXPECT_EQ(syncer::SyncService::DISABLE_REASON_NONE,
             sync_service()->GetDisableReasons());
   EXPECT_FALSE(sync_service()->IsSyncActive());
-  EXPECT_EQ(syncer::SyncService::State::WAITING_FOR_CONSENT,
+  EXPECT_EQ(syncer::SyncService::State::PENDING_DESIRED_CONFIGURATION,
             sync_service()->GetState());
 
   // Setup is already in progress, so confirmation still isn't needed.
@@ -229,7 +229,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) {
   ASSERT_FALSE(sync_service()->IsSetupInProgress());
   EXPECT_TRUE(sync_service()->IsSyncConfirmationNeeded());
   EXPECT_FALSE(sync_service()->IsSyncActive());
-  EXPECT_EQ(syncer::SyncService::State::WAITING_FOR_CONSENT,
+  EXPECT_EQ(syncer::SyncService::State::PENDING_DESIRED_CONFIGURATION,
             sync_service()->GetState());
 
   // Marking first setup complete will let ProfileSyncService configure the
@@ -605,15 +605,16 @@ TEST_F(ProfileSyncServiceStartupTest, FullStartupSequenceFirstTime) {
       syncer::ModelTypeSet(), syncer::WeakHandle<syncer::JsBackend>(),
       syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), "test-guid",
       /*success=*/true);
-  EXPECT_EQ(syncer::SyncService::State::WAITING_FOR_CONSENT,
+  ASSERT_TRUE(sync_service()->IsEngineInitialized());
+  EXPECT_EQ(syncer::SyncService::State::PENDING_DESIRED_CONFIGURATION,
             sync_service()->GetState());
 
   // Once the user finishes the initial setup, the service can actually start
-  // configuring the data types. It won't actually call the DataTypeManager yet
-  // though, because setup is still considered in progress (we haven't released
-  // the setup-in-progress handle).
+  // configuring the data types. Just marking the initial setup as complete
+  // isn't enough though, because setup is still considered in progress (we
+  // haven't released the setup-in-progress handle).
   sync_service()->SetFirstSetupComplete();
-  EXPECT_EQ(syncer::SyncService::State::CONFIGURING,
+  EXPECT_EQ(syncer::SyncService::State::PENDING_DESIRED_CONFIGURATION,
             sync_service()->GetState());
 
   // Releasing the setup in progress handle lets the service actually configure

components/browser_sync/profile_sync_service_unittest.cc

@@ -385,14 +385,14 @@ TEST_F(ProfileSyncServiceTest, NeedsConfirmation) {
   // Note: At this point the engine *can* start, but nothing has kicked it off
   // (usually that happens via getting and then releasing a
   // SyncSetupInProgressHandle), so the state is still WAITING_FOR_START_REQUEST
-  // and not WAITING_FOR_CONSENT.
+  // and not PENDING_DESIRED_CONFIGURATION.
   EXPECT_EQ(syncer::SyncService::State::WAITING_FOR_START_REQUEST,
             service()->GetState());
 
   // Once we kick off initialization by getting and releasing a setup handle,
-  // the state goes to WAITING_FOR_CONSENT.
+  // the state goes to PENDING_DESIRED_CONFIGURATION.
   service()->GetSetupInProgressHandle();
-  EXPECT_EQ(syncer::SyncService::State::WAITING_FOR_CONSENT,
+  EXPECT_EQ(syncer::SyncService::State::PENDING_DESIRED_CONFIGURATION,
             service()->GetState());
 
   // The last sync time shouldn't be cleared.

components/sync/driver/about_sync_util.cc

@@ -201,8 +201,8 @@ std::string GetSummaryString(syncer::SyncService::State state,
       return "Start deferred";
     case syncer::SyncService::State::INITIALIZING:
       return "Initializing";
-    case syncer::SyncService::State::WAITING_FOR_CONSENT:
-      return "Waiting for initial setup";
+    case syncer::SyncService::State::PENDING_DESIRED_CONFIGURATION:
+      return "Pending desired configuration";
     case syncer::SyncService::State::CONFIGURING:
       return "Configuring data types";
     case syncer::SyncService::State::ACTIVE:

components/sync/driver/fake_sync_service.cc

@@ -55,7 +55,7 @@ syncer::SyncService::State FakeSyncService::GetState() const {
     return State::INITIALIZING;
   }
   if (!IsFirstSetupComplete()) {
-    return State::WAITING_FOR_CONSENT;
+    return State::PENDING_DESIRED_CONFIGURATION;
   }
   DCHECK(IsSyncActive());
   if (!configuration_done_) {

components/sync/driver/sync_service.h

@@ -122,9 +122,12 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
     START_DEFERRED,
     // The Sync engine is in the process of initializing.
     INITIALIZING,
-    // The Sync engine is initialized, but the user hasn't completed the initial
-    // Sync setup yet, so we won't actually configure the data types.
-    WAITING_FOR_CONSENT,
+    // The Sync engine is initialized, but the process of configuring the data
+    // types hasn't been started yet. This usually occurs if the user hasn't
+    // completed the initial Sync setup yet (i.e. IsFirstSetupComplete() is
+    // false), but it can also occur if a (non-initial) Sync setup happens to be
+    // ongoing while the Sync service is starting up.
+    PENDING_DESIRED_CONFIGURATION,
     // The Sync engine itself is up and running, but the individual data types
     // are being (re)configured. GetActiveDataTypes() will still be empty.
     CONFIGURING,

components/sync/driver/sync_service_utils.cc

@@ -45,7 +45,7 @@ UploadState GetUploadToGoogleState(const SyncService* sync_service,
     case SyncService::State::WAITING_FOR_START_REQUEST:
     case SyncService::State::START_DEFERRED:
     case SyncService::State::INITIALIZING:
-    case SyncService::State::WAITING_FOR_CONSENT:
+    case SyncService::State::PENDING_DESIRED_CONFIGURATION:
     case SyncService::State::CONFIGURING:
       return UploadState::INITIALIZING;