Settings[DICE]: changing sync-everything doesn't impact on-disk prefs

This CL stops toggling "sync everything" from overriding all individual
prefs' on-disk value with "true" (see reasoning in bug report).

Bug: 815018
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: If6287376009b200355426113618933af9ad78243
Reviewed-on: https://chromium-review.googlesource.com/935836Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542929}

chrome/browser/resources/settings/people_page/sync_browser_proxy.js

@@ -190,6 +190,13 @@ cr.define('settings', function() {
      */
     setSyncDatatypes(syncPrefs) {}
 
+    /**
+     * Sets the syncAllDataTypes pref.
+     * @param {boolean} syncEverything
+     * @return {!Promise<!settings.PageStatus>}
+     */
+    setSyncEverything(syncEverything) {}
+
     /**
      * Sets the sync encryption options.
      * @param {!settings.SyncPrefs} syncPrefs
@@ -277,6 +284,11 @@ cr.define('settings', function() {
           'SyncSetupSetDatatypes', JSON.stringify(syncPrefs));
     }
 
+    /** @override */
+    setSyncEverything(syncEverything) {
+      return cr.sendWithPromise('SyncSetupSetSyncEverything', syncEverything);
+    }
+
     /** @override */
     setSyncEncryption(syncPrefs) {
       return cr.sendWithPromise(

chrome/browser/resources/settings/people_page/sync_page.js

@@ -14,24 +14,6 @@ const RadioButtonNames = {
   ENCRYPT_WITH_PASSPHRASE: 'encrypt-with-passphrase',
 };
 
-/**
- * Names of the individual data type properties to be cached from
- * settings.SyncPrefs when the user checks 'Sync All'.
- * @type {!Array<string>}
- */
-const SyncPrefsIndividualDataTypes = [
-  'appsSynced',
-  'extensionsSynced',
-  'preferencesSynced',
-  'autofillSynced',
-  'typedUrlsSynced',
-  'themesSynced',
-  'bookmarksSynced',
-  'passwordsSynced',
-  'tabsSynced',
-  'paymentsIntegrationEnabled',
-];
-
 /**
  * @fileoverview
  * 'settings-sync-page' is the settings page containing sync settings.
@@ -121,13 +103,6 @@ Polymer({
    */
   unloadCallback_: null,
 
-  /**
-   * Caches the individually selected synced data types. This is used to
-   * be able to restore the selections after checking and unchecking Sync All.
-   * @private {?Object}
-   */
-  cachedSyncPrefs_: null,
-
   /** @override */
   created: function() {
     this.browserProxy_ = settings.SyncBrowserProxyImpl.getInstance();
@@ -231,25 +206,8 @@ Polymer({
    * @private
    */
   onSyncAllDataTypesChanged_: function(event) {
-    if (event.target.checked) {
-      this.set('syncPrefs.syncAllDataTypes', true);
-
-      // Cache the previously selected preference before checking every box.
-      this.cachedSyncPrefs_ = {};
-      for (const dataType of SyncPrefsIndividualDataTypes) {
-        // These are all booleans, so this shallow copy is sufficient.
-        this.cachedSyncPrefs_[dataType] = this.syncPrefs[dataType];
-
-        this.set(['syncPrefs', dataType], true);
-      }
-    } else if (this.cachedSyncPrefs_) {
-      // Restore the previously selected preference.
-      for (const dataType of SyncPrefsIndividualDataTypes) {
-        this.set(['syncPrefs', dataType], this.cachedSyncPrefs_[dataType]);
-      }
-    }
-
-    this.onSingleSyncDataTypeChanged_();
+    this.browserProxy_.setSyncEverything(event.target.checked)
+        .then(this.handlePageStatusChanged_.bind(this));
   },
 
   /**

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

@@ -219,6 +219,10 @@ void PeopleHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "SyncSetupSetDatatypes",
       base::Bind(&PeopleHandler::HandleSetDatatypes, base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "SyncSetupSetSyncEverything",
+      base::Bind(&PeopleHandler::HandleSetSyncEverything,
+                 base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "SyncSetupSetEncryption",
       base::Bind(&PeopleHandler::HandleSetEncryption, base::Unretained(this)));
@@ -441,6 +445,30 @@ void PeopleHandler::HandleSetDatatypes(const base::ListValue* args) {
     ProfileMetrics::LogProfileSyncInfo(ProfileMetrics::SYNC_CHOOSE);
 }
 
+// This function is different from HandleSetDatatypes, in that it only sets
+// the syncAllDatatypes without overriding other prefs' on-disk values.
+void PeopleHandler::HandleSetSyncEverything(const base::ListValue* args) {
+  DCHECK(!sync_startup_tracker_);
+
+  CHECK_EQ(2U, args->GetSize());
+  const base::Value& callback_id = args->GetList()[0];
+  bool sync_everything = args->GetList()[1].GetBool();
+
+  ProfileSyncService* service = GetSyncService();
+  // If the sync engine has shutdown for some reason, just close the sync
+  // dialog.
+  if (!service || !service->IsEngineInitialized()) {
+    CloseSyncSetup();
+    ResolveJavascriptCallback(callback_id, base::Value(kDonePageStatus));
+    return;
+  }
+
+  service->OnUserChangedSyncEverythingOnly(sync_everything);
+  ResolveJavascriptCallback(callback_id, base::Value(kConfigurePageStatus));
+
+  ProfileMetrics::LogProfileSyncInfo(ProfileMetrics::SYNC_CUSTOMIZE);
+}
+
 #if BUILDFLAG(ENABLE_DICE_SUPPORT)
 void PeopleHandler::HandleGetStoredAccounts(const base::ListValue* args) {
   CHECK_EQ(1U, args->GetSize());

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

@@ -153,6 +153,7 @@ class PeopleHandler : public SettingsPageUIHandler,
   void OnDidClosePage(const base::ListValue* args);
   void HandleSetDatatypes(const base::ListValue* args);
   void HandleSetEncryption(const base::ListValue* args);
+  void HandleSetSyncEverything(const base::ListValue* args);
   void HandleShowSetupUI(const base::ListValue* args);
   void HandleAttemptUserExit(const base::ListValue* args);
   void HandleStartSignin(const base::ListValue* args);

chrome/test/data/webui/settings/people_page_sync_page_test.js

@@ -153,6 +153,11 @@ cr.define('settings_people_page_sync_page', function() {
     });
 
     test('SettingIndividualDatatypes', function() {
+      // Simulate syncAllDataType being true.
+      const expected = getSyncAllPrefs();
+      expected.syncAllDataTypes = true;
+      cr.webUIListenerCallback('sync-prefs-changed', expected);
+
       const syncAllDataTypesControl = syncPage.$.syncAllDataTypesControl;
       assertFalse(syncAllDataTypesControl.disabled);
       assertTrue(syncAllDataTypesControl.checked);
@@ -168,32 +173,25 @@ cr.define('settings_people_page_sync_page', function() {
       // Uncheck the Sync All control.
       MockInteractions.tap(syncAllDataTypesControl);
 
-      function verifyPrefs(prefs) {
-        const expected = getSyncAllPrefs();
-        expected.syncAllDataTypes = false;
-        assertEquals(JSON.stringify(expected), JSON.stringify(prefs));
-
-        cr.webUIListenerCallback('sync-prefs-changed', expected);
-
-        // Assert that all the individual datatype controls are enabled.
-        for (const control of datatypeControls) {
-          assertFalse(control.disabled);
-          assertTrue(control.checked);
-        }
-
-        browserProxy.resetResolver('setSyncDatatypes');
-
-        // Test an arbitrarily-selected control (extensions synced control).
-        MockInteractions.tap(datatypeControls[3]);
-        return browserProxy.whenCalled('setSyncDatatypes').then(
-            function(prefs) {
-              const expected = getSyncAllPrefs();
-              expected.syncAllDataTypes = false;
-              expected.extensionsSynced = false;
-              assertEquals(JSON.stringify(expected), JSON.stringify(prefs));
-            });
-      }
-      return browserProxy.whenCalled('setSyncDatatypes').then(verifyPrefs);
+      return browserProxy.whenCalled('setSyncEverything')
+          .then(syncEverything => {
+            assertFalse(syncEverything);
+
+            // Assert that all the individual datatype controls are enabled.
+            for (const control of datatypeControls) {
+              assertFalse(control.disabled);
+              assertTrue(control.checked);
+            }
+
+            // Test an arbitrarily-selected control (extensions synced control).
+            MockInteractions.tap(datatypeControls[3]);
+            return browserProxy.whenCalled('setSyncDatatypes')
+                .then(function(prefs) {
+                  assertFalse(datatypeControls[3].checked);
+                  expected.extensionsSynced = false;
+                  assertEquals(JSON.stringify(expected), JSON.stringify(prefs));
+                });
+          });
     });
 
     test('RadioBoxesEnabledWhenUnencrypted', function() {

chrome/test/data/webui/settings/test_sync_browser_proxy.js

@@ -17,6 +17,7 @@ class TestSyncBrowserProxy extends TestBrowserProxy {
       'signOut',
       'startSignIn',
       'startSyncingWithEmail',
+      'setSyncEverything',
     ]);
 
     /** @private {number} */
@@ -92,4 +93,10 @@ class TestSyncBrowserProxy extends TestBrowserProxy {
     this.methodCalled('setSyncEncryption', syncPrefs);
     return Promise.resolve(this.encryptionResponse);
   }
+
+  /** @override */
+  setSyncEverything(syncEverything) {
+    this.methodCalled('setSyncEverything', syncEverything);
+    return Promise.resolve(settings.PageStatus.CONFIGURE);
+  }
 }
\ No newline at end of file

components/browser_sync/profile_sync_service.cc

@@ -1527,6 +1527,20 @@ void ProfileSyncService::OnUserChoseDatatypes(
   ChangePreferredDataTypes(chosen_types);
 }
 
+void ProfileSyncService::OnUserChangedSyncEverythingOnly(bool sync_everything) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!engine_ && !HasUnrecoverableError()) {
+    NOTREACHED();
+    return;
+  }
+
+  sync_prefs_.SetKeepEverythingSynced(sync_everything);
+  UpdateSelectedTypesHistogram(sync_everything, GetPreferredDataTypes());
+  if (data_type_manager_)
+    data_type_manager_->ResetDataTypeErrors();
+  ReconfigureDatatypeManager();
+}
+
 void ProfileSyncService::ChangePreferredDataTypes(
     syncer::ModelTypeSet preferred_types) {
   DCHECK(thread_checker_.CalledOnValidThread());

components/browser_sync/profile_sync_service.h

@@ -130,6 +130,8 @@ namespace browser_sync {
 //
 //   Sync configuration is accomplished via the following APIs:
 //    * OnUserChoseDatatypes(): Set the data types the user wants to sync.
+//    * OnUserChangedSyncEverythingOnly(): Set only the keepEverythingSynced
+//        value.
 //    * SetDecryptionPassphrase(): Attempt to decrypt the user's encrypted data
 //        using the passed passphrase.
 //    * SetEncryptionPassphrase(): Re-encrypt the user's data using the passed
@@ -314,6 +316,11 @@ class ProfileSyncService : public syncer::SyncServiceBase,
                        callback) override;
   syncer::GlobalIdMapper* GetGlobalIdMapper() const override;
 
+  // Changes only the KeepEverythingSynced value.
+  // TODO(crbug/820625): Refactor sync code for more robust way to get/set
+  // preferred datatypes.
+  void OnUserChangedSyncEverythingOnly(bool sync_everything);
+
   // Add a sync type preference provider. Each provider may only be added once.
   void AddPreferenceProvider(syncer::SyncTypePreferenceProvider* provider);
   // Remove a sync type preference provider. May only be called for providers