PeopleHandler: (re)start Sync-the-feature if necessary

If Sync-the-feature is disabled because of a "Reset Sync" action on
the dashboard, then the Sync entry in chrome://settings shows an error
message, and clicking on it should re-enable Sync.
The UI logic checks for this state via SyncService::IsEngineInitialized.
However, if Sync-the-transport is running, then the engine can be
initialized even though Sync-the-feature is still disabled. In this
situation, the user has no way to turn Sync back on.

This CL fixes this by adding a check for DISABLE_REASON_USER_CHOICE
which on desktop corresponds to a dashboard clear. It also adds tests.

Bug: 881934
Change-Id: I6216564ac6fbb9efff988a2ebd09d532c82985cd
Reviewed-on: https://chromium-review.googlesource.com/1224379Reviewed-by: Scott Chen <scottchen@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591141}

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

@@ -671,11 +671,25 @@ void PeopleHandler::HandleShowSetupUI(const base::ListValue* args) {
   if (sync_startup_tracker_)
     return;
 
-  if (!service->IsEngineInitialized()) {
+  if (!service->IsEngineInitialized() ||
+      service->HasDisableReason(
+          syncer::SyncService::DISABLE_REASON_USER_CHOICE)) {
     // Requesting the sync service to start may trigger call to PushSyncPrefs.
     // Setting up the startup tracker beforehand correctly signals the
     // re-entrant call to early exit.
-    sync_startup_tracker_.reset(new SyncStartupTracker(profile_, this));
+    sync_startup_tracker_ =
+        std::make_unique<SyncStartupTracker>(profile_, this);
+    // RequestStart() does two things:
+    // 1) If DISABLE_REASON_USER_CHOICE is set (meaning that Sync was reset via
+    //    the dashboard), clears it.
+    // 2) Pokes the sync service to start *immediately*, i.e. bypass deferred
+    //    startup.
+    // It's possible that both of these are already the case, i.e. the engine is
+    // already in the process of initializing, in which case RequestStart() will
+    // effectively do nothing. It's also possible that the sync service is
+    // already running in standalone transport mode and so the engine is already
+    // initialized. In that case, this will trigger the service to switch to
+    // full Sync-the-feature mode.
     service->RequestStart();
 
     // See if it's even possible to bring up the sync engine - if not

chrome/browser/ui/webui/settings/people_handler.h

@@ -88,7 +88,6 @@ class PeopleHandler : public SettingsPageUIHandler,
       PeopleHandlerTest,
       DisplayConfigureWithEngineDisabledAndSyncStartupCompleted);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, HandleSetupUIWhenSyncDisabled);
-  FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, SelectCustomEncryption);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest,
                            ShowSetupCustomPassphraseRequired);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, ShowSetupEncryptAll);
@@ -102,9 +101,7 @@ class PeopleHandler : public SettingsPageUIHandler,
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, ShowSigninOnAuthError);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, ShowSyncSetup);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, ShowSyncSetupWhenNotSignedIn);
-  FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, SuccessfullySetPassphrase);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, TestSyncEverything);
-  FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, TestSyncNothing);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, TestSyncAllManually);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, TestPassphraseStillRequired);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, TestSyncIndividualTypes);
@@ -117,13 +114,13 @@ class PeopleHandler : public SettingsPageUIHandler,
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerNonCrosTest,
                            UnrecoverableErrorInitializingSync);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerNonCrosTest, GaiaErrorInitializingSync);
-  FRIEND_TEST_ALL_PREFIXES(PeopleHandlerNonCrosTest, HandleCaptcha);
-  FRIEND_TEST_ALL_PREFIXES(PeopleHandlerNonCrosTest, HandleGaiaAuthFailure);
-  FRIEND_TEST_ALL_PREFIXES(PeopleHandlerNonCrosTest,
-                           SubmitAuthWithInvalidUsername);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerFirstSigninTest, DisplayBasicLogin);
   FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest,
                            AcquireSyncBlockerWhenLoadingSyncSettingsSubpage);
+  FRIEND_TEST_ALL_PREFIXES(PeopleHandlerTest, RestartSyncAfterDashboardClear);
+  FRIEND_TEST_ALL_PREFIXES(
+      PeopleHandlerTest,
+      RestartSyncAfterDashboardClearWithStandaloneTransport);
 
   // SettingsPageUIHandler implementation.
   void RegisterMessages() override;

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

@@ -505,6 +505,56 @@ TEST_F(PeopleHandlerTest, DisplayConfigureWithEngineDisabledAndSigninFailed) {
       LoginUIServiceFactory::GetForProfile(profile())->current_login_ui());
 }
 
+TEST_F(PeopleHandlerTest, RestartSyncAfterDashboardClear) {
+  // Clearing sync from the dashboard results in DISABLE_REASON_USER_CHOICE
+  // being set.
+  ON_CALL(*mock_pss_, GetDisableReasons())
+      .WillByDefault(Return(syncer::SyncService::DISABLE_REASON_USER_CHOICE));
+  ON_CALL(*mock_pss_, IsFirstSetupComplete()).WillByDefault(Return(true));
+  ON_CALL(*mock_pss_, GetTransportState())
+      .WillByDefault(Return(syncer::SyncService::TransportState::DISABLED));
+
+  // Attempting to open the setup UI should restart sync.
+  EXPECT_CALL(*mock_pss_, RequestStart()).WillOnce([&]() {
+    // RequestStart() clears DISABLE_REASON_USER_CHOICE, and immediately starts
+    // initialzing the engine.
+    ON_CALL(*mock_pss_, GetDisableReasons())
+        .WillByDefault(Return(syncer::SyncService::DISABLE_REASON_NONE));
+    ON_CALL(*mock_pss_, GetTransportState())
+        .WillByDefault(
+            Return(syncer::SyncService::TransportState::INITIALIZING));
+  });
+
+  handler_->HandleShowSetupUI(nullptr);
+  ExpectPageStatusChanged(PeopleHandler::kSpinnerPageStatus);
+}
+
+TEST_F(PeopleHandlerTest,
+       RestartSyncAfterDashboardClearWithStandaloneTransport) {
+  // Clearing sync from the dashboard results in DISABLE_REASON_USER_CHOICE
+  // being set. However, the sync engine has restarted in standalone transport
+  // mode.
+  ON_CALL(*mock_pss_, GetDisableReasons())
+      .WillByDefault(Return(syncer::SyncService::DISABLE_REASON_USER_CHOICE));
+  ON_CALL(*mock_pss_, IsFirstSetupComplete()).WillByDefault(Return(true));
+  ON_CALL(*mock_pss_, GetTransportState())
+      .WillByDefault(Return(syncer::SyncService::TransportState::ACTIVE));
+
+  // Attempting to open the setup UI should re-enable sync-the-feature.
+  EXPECT_CALL(*mock_pss_, RequestStart()).WillOnce([&]() {
+    // RequestStart() clears DISABLE_REASON_USER_CHOICE. Since the engine is
+    // already running, it just gets reconfigured.
+    ON_CALL(*mock_pss_, GetDisableReasons())
+        .WillByDefault(Return(syncer::SyncService::DISABLE_REASON_NONE));
+    ON_CALL(*mock_pss_, GetTransportState())
+        .WillByDefault(
+            Return(syncer::SyncService::TransportState::CONFIGURING));
+  });
+
+  handler_->HandleShowSetupUI(nullptr);
+  ExpectPageStatusChanged(PeopleHandler::kSpinnerPageStatus);
+}
+
 // Tests that signals not related to user intention to configure sync don't
 // trigger sync engine start.
 TEST_F(PeopleHandlerTest, OnlyStartEngineWhenConfiguringSync) {