[sync] Separate sign in from sync on Desktop Chrome

chrome/browser/resources/sync_setup_overlay.js

Index: chrome/browser/resources/sync_setup_overlay.js
diff --git a/chrome/browser/resources/sync_setup_overlay.js b/chrome/browser/resources/sync_setup_overlay.js
index 7a5f4d4fd09816457e26b018d4ee0c6420180f85..1e4eda00f1bccd0c6d9de8cb795a8d32deb25ffd 100644
--- a/chrome/browser/resources/sync_setup_overlay.js
+++ b/chrome/browser/resources/sync_setup_overlay.js
@@ -35,6 +35,27 @@ cr.define('options', function() {
   // email address.
   var lastEmailAddress_ = '';
 
+  // An object used as a cache of the arguments passed in while initially
+  // displaying the advanced sync settings dialog. Used to switch between the
+  // options in the main drop-down menu. Reset when the dialog is closed.
+  var syncConfigureArgs_ = null;
+
+  // A dictionary used as a cache of the checked states of the sync data type
+  // checkboxes. Used while switching between the options in the main drop-down
+  // menu of the advanced sync settings dialog. Reset when the dialog is closed.
+  var dataTypeBoxes_ = {};

[Andrew T Wilson (Slow), 2013/05/14 16:48:27]

Can you describe what the dictionary holds (what are the keys, what are the
values, at least examples + types)? Also, how is this initialized (lets say that
sync is already set to CHOOSE_WHAT_TO_SYNC when you bring up the dialog, then
you set the dropdown to SYNC_EVERYTHING, then you go back to CHOOSE_WHAT_TO_SYNC
-- this should restore the checkboxes based on what's in this data structure. At
what point does this data structure get initialized)?

[Raghu Simha, 2013/05/15 02:09:54]

Done. I've beefed up this comment to include what you suggest here. There's also
a detailed description of how cache is used at the top of showConfigure_().

+
+  /**
+   * The user's selection in the synced data type drop-down menu, as an index.
+   * @enum {number}
+   * @const
+   */
+  var DataTypeSelection = {
+    SYNC_EVERYTHING: 0,
+    CHOOSE_WHAT_TO_SYNC: 1,
+    SYNC_NOTHING: 2
+  };
+
   /**
    * SyncSetupOverlay class
    * Encapsulated handling of the 'Sync Setup' overlay page.
@@ -95,6 +116,8 @@ cr.define('options', function() {
     },
 
     closeOverlay_: function() {
+      this.syncConfigureArgs_ = null;
+      this.dataTypeBoxes_ = {};
       OptionsPage.closeOverlay();
     },
 
@@ -145,16 +168,56 @@ cr.define('options', function() {
       $('sync-custom-passphrase').hidden = !visible;
     },
 
-    checkAllDataTypeCheckboxes_: function() {
-      // Only check the visible ones (since there's no way to uncheck
-      // the invisible ones).
+    /**
+     * Sets the checked state of the individual sync data type checkboxes in the
+     * advanced sync settings dialog.
+     * @param {boolean} value True for checked, false for unchecked.
+     * @private
+     */
+    checkAllDataTypeCheckboxes_: function(value) {
+      // Only check / uncheck the visible ones (since there's no way to uncheck
+      // / check the invisible ones).
       var checkboxes = $('choose-data-types-body').querySelectorAll(
           '.sync-type-checkbox:not([hidden]) input');
       for (var i = 0; i < checkboxes.length; i++) {
-        checkboxes[i].checked = true;
+        checkboxes[i].checked = value;
       }
     },
 
+    /**
+     * Restores the checked states of the sync data type checkboxes in the
+     * advanced sync settings dialog when "Choose what to sync" is selected.
+     * Required because all the checkboxes are checked when "Sync everything" is
+     * selected, and unchecked when "Sync nothing" is selected.
+     * @private
+     */
+    restoreDataTypeCheckboxes_: function() {
+      if ('bookmarksSynced' in dataTypeBoxes_)

[Andrew T Wilson (Slow), 2013/05/14 16:48:27]

What does it mean to not have 'bookmarksSynced' in this dictionary?

[Raghu Simha, 2013/05/15 02:09:54]

This is to prevent us from trying to modify any hidden checkboxes, which aren't
cached in the first place. Some data types are not registered under all
circumstances, and we don't want to mess with their values here. I've updated
the comment to mention this.

I've also replaced the implementation with a simpler version that uses the
existing "id" attribute instead of brute-forcing the list of checkboxes.

+        $('bookmarks-checkbox').checked = dataTypeBoxes_['bookmarksSynced'];
+      if ('preferencesSynced' in dataTypeBoxes_)
+        $('preferences-checkbox').checked = dataTypeBoxes_['preferencesSynced'];
+      if ('themesSynced' in dataTypeBoxes_)
+        $('themes-checkbox').checked = dataTypeBoxes_['themesSynced'];
+      if ('passwordsSynced' in dataTypeBoxes_)
+        $('passwords-checkbox').checked = dataTypeBoxes_['passwordsSynced'];
+      if ('autofillSynced' in dataTypeBoxes_)
+        $('autofill-checkbox').checked = dataTypeBoxes_['autofillSynced'];
+      if ('extensionsSynced' in dataTypeBoxes_)
+        $('extensions-checkbox').checked = dataTypeBoxes_['extensionsSynced'];
+      if ('typedUrlsSynced' in dataTypeBoxes_)
+        $('typed-urls-checkbox').checked = dataTypeBoxes_['typedUrlsSynced'];
+      if ('appsSynced' in dataTypeBoxes_)
+        $('apps-checkbox').checked = dataTypeBoxes_['appsSynced'];
+      if ('tabsSynced' in dataTypeBoxes_)
+        $('tabs-checkbox').checked = dataTypeBoxes_['tabsSynced'];
+    },
+
+    /**
+     * Enables / grays out the sync data type checkboxes in the advanced
+     * settings dialog.
+     * @param {boolean} enabled True for enabled, false for grayed out.
+     * @private
+     */
     setDataTypeCheckboxesEnabled_: function(enabled) {
       var checkboxes = $('choose-data-types-body').querySelectorAll('input');
       for (var i = 0; i < checkboxes.length; i++) {
@@ -162,10 +225,22 @@ cr.define('options', function() {
       }
     },
 
-    setCheckboxesToKeepEverythingSynced_: function(value) {
-      this.setDataTypeCheckboxesEnabled_(!value);
-      if (value)
-        this.checkAllDataTypeCheckboxes_();
+    /**
+     * Sets the state of the sync data type checkboxes based on whether "Sync
+     * everything", "Choose what to sync", or "Sync nothing" are selected in the
+     * drop-down menu of the advanced settings dialog.
+     * @param {cr.DataTypeSelection} selectedIndex Index of user's selection.
+     * @private
+     */
+    setDataTypeCheckboxes_: function(selectedIndex) {
+      if (selectedIndex == DataTypeSelection.CHOOSE_WHAT_TO_SYNC) {
+        this.setDataTypeCheckboxesEnabled_(true);
+        this.restoreDataTypeCheckboxes_();
+      } else {
+        this.setDataTypeCheckboxesEnabled_(false);
+        this.checkAllDataTypeCheckboxes_(selectedIndex ==
+                                         DataTypeSelection.SYNC_EVERYTHING);
+      }
     },
 
     // Returns true if none of the visible checkboxes are checked.
@@ -208,8 +283,9 @@ cr.define('options', function() {
       // Trying to submit, so hide previous errors.
       $('error-text').hidden = true;
 
-      var syncAll = $('sync-select-datatypes').selectedIndex == 0;
-      if (!syncAll && this.noDataTypesChecked_()) {
+      var chooseWhatToSync = $('sync-select-datatypes').selectedIndex ==
+                             DataTypeSelection.CHOOSE_WHAT_TO_SYNC;
+      if (chooseWhatToSync && this.noDataTypesChecked_()) {
         $('error-text').hidden = false;
         return;
       }
@@ -257,8 +333,13 @@ cr.define('options', function() {
 
       // These values need to be kept in sync with where they are read in
       // SyncSetupFlow::GetDataTypeChoiceData().
+      var syncAll = $('sync-select-datatypes').selectedIndex ==
+                    DataTypeSelection.SYNC_EVERYTHING;
+      var syncNothing = $('sync-select-datatypes').selectedIndex ==
+                        DataTypeSelection.SYNC_NOTHING;
       var result = JSON.stringify({
         'syncAllDataTypes': syncAll,
+        'syncNothing': syncNothing,

[Andrew T Wilson (Slow), 2013/05/14 16:48:27]

nit: I'm slightly nervous about this because it's redundant with
syncAllDataTypes (i.e. what does it mean to have syncNothing = true and
syncAllDataTypes=true at the same time?) - this is a sign that perhaps these
variables should be combined into one?

[Raghu Simha, 2013/05/15 02:09:54]

It's impossible for both values to be true, since we initialize them using
$('sync-select-datatypes').selectedIndex, which can make one or the other true,
but not both.

The reason the values are not redundant is that the drop-down can have a third
value, which is CHOOSE_WHAT_TO_SYNC, in which case both booleans are false.

I did consider eliminating the two booleans and passing in an enum to the C++
layer, but it involved unnecessarily rewriting a bunch of code in
sync_setup_handler and other layers. The value of syncAll is used all over the
place in sync setup code, while syncNothing is used in just one spot, in order
to disable sync. I figured it would be less disruptive to send in two booleans
than to refactor all the set up code to use an enum.

         'bookmarksSynced': syncAll || $('bookmarks-checkbox').checked,
         'preferencesSynced': syncAll || $('preferences-checkbox').checked,
         'themesSynced': syncAll || $('themes-checkbox').checked,
@@ -293,7 +374,7 @@ cr.define('options', function() {
 
       var self = this;
       this.animateDisableLink_($('customize-link'), disabled, function() {
-        self.showCustomizePage_(null, true);
+        self.showCustomizePage_(null, DataTypeSelection.SYNC_EVERYTHING);
       });
     },
 
@@ -325,57 +406,98 @@ cr.define('options', function() {
     },
 
     /**
-     * Shows or hides the Sync data type checkboxes in the advanced
-     * configuration screen.
+     * Shows or hides the sync data type checkboxes in the advanced sync
+     * settings dialog. Also initializes |dataTypeBoxes_| with their values, and
+     * makes their onclick handlers update |dataTypeBoxes_|.
      * @param {Object} args The configuration data used to show/hide UI.
      * @private
      */
     setChooseDataTypesCheckboxes_: function(args) {
       var datatypeSelect = $('sync-select-datatypes');
-      datatypeSelect.selectedIndex = args.syncAllDataTypes ? 0 : 1;
+      datatypeSelect.selectedIndex = args.syncAllDataTypes ?
+                                         DataTypeSelection.SYNC_EVERYTHING :
+                                         DataTypeSelection.CHOOSE_WHAT_TO_SYNC;
 
       $('bookmarks-checkbox').checked = args.bookmarksSynced;
+      dataTypeBoxes_['bookmarksSynced'] = args.bookmarksSynced;
+      $('bookmarks-checkbox').onclick = function() {
+        dataTypeBoxes_['bookmarksSynced'] = $('bookmarks-checkbox').checked;

[Andrew T Wilson (Slow), 2013/05/14 16:48:27]

I'm OK with this, but I'm wondering if there's some way to eliminate some of
this boilerplate, maybe by defining a "prefName" attribute on the DOM element.
This would let you do this:

$('bookmarks-checkbox').onclick = handleDataTypeClick;

function handleDataTypeClick() {
  dataTypeBoxes_[this.prefName] = this.checked;
}

That way every checkbox could use the same onclick handler.

Anyhow, you don't have to do this, but if you're interested in removing some
boilerplate you could really simplify this code.

Alternatively, you could just have a single onclick handler that just rebuilt
dataTypeBoxes_ entirely by querying the various DOM elements rather than setting
only a single attribute on each click.

[Raghu Simha, 2013/05/15 02:09:54]

The DOM elements already have an "id" attribute that I can use.
Good idea. Done.
This is possible, but I don't believe we'll gain much by adding a single onclick
handler, because it will still involve enumerating the list of checkboxes and
checking if they are hidden or not. Also, Content Security Policy forbids event
handlers from being defined inline in html, so we'll have to do it in
JavaScript.

Having said that, the latest patch has considerably less boilerplate. Let me
know what you think.

+      };
+
       $('preferences-checkbox').checked = args.preferencesSynced;
+      dataTypeBoxes_['preferencesSynced'] = args.preferencesSynced;
+      $('preferences-checkbox').onclick = function() {
+        dataTypeBoxes_['preferencesSynced'] = $('preferences-checkbox').checked;
+      };
+
       $('themes-checkbox').checked = args.themesSynced;
+      dataTypeBoxes_['themesSynced'] = args.themesSynced;
+      $('themes-checkbox').onclick = function() {
+        dataTypeBoxes_['themesSynced'] = $('themes-checkbox').checked;
+      };
 
       if (args.passwordsRegistered) {
         $('passwords-checkbox').checked = args.passwordsSynced;
+        dataTypeBoxes_['passwordsSynced'] = args.passwordsSynced;
+        $('passwords-checkbox').onclick = function() {
+          dataTypeBoxes_['passwordsSynced'] = $('passwords-checkbox').checked;
+        };
         $('passwords-item').hidden = false;
       } else {
         $('passwords-item').hidden = true;
       }
       if (args.autofillRegistered) {
         $('autofill-checkbox').checked = args.autofillSynced;
+        dataTypeBoxes_['autofillSynced'] = args.autofillSynced;
+        $('autofill-checkbox').onclick = function() {
+          dataTypeBoxes_['autofillSynced'] = $('autofill-checkbox').checked;
+        };
         $('autofill-item').hidden = false;
       } else {
         $('autofill-item').hidden = true;
       }
       if (args.extensionsRegistered) {
         $('extensions-checkbox').checked = args.extensionsSynced;
+        dataTypeBoxes_['extensionsSynced'] = args.extensionsSynced;
+        $('extensions-checkbox').onclick = function() {
+          dataTypeBoxes_['extensionsSynced'] = $('extensions-checkbox').checked;
+        };
         $('extensions-item').hidden = false;
       } else {
         $('extensions-item').hidden = true;
       }
       if (args.typedUrlsRegistered) {
         $('typed-urls-checkbox').checked = args.typedUrlsSynced;
+        dataTypeBoxes_['typedUrlsSynced'] = args.typedUrlsSynced;
+        $('typed-urls-checkbox').onclick = function() {
+          dataTypeBoxes_['typedUrlsSynced'] = $('typed-urls-checkbox').checked;
+        };
         $('omnibox-item').hidden = false;
       } else {
         $('omnibox-item').hidden = true;
       }
       if (args.appsRegistered) {
         $('apps-checkbox').checked = args.appsSynced;
+        dataTypeBoxes_['appsSynced'] = args.appsSynced;
+        $('apps-checkbox').onclick = function() {
+          dataTypeBoxes_['appsSynced'] = $('apps-checkbox').checked;
+        };
         $('apps-item').hidden = false;
       } else {
         $('apps-item').hidden = true;
       }
       if (args.tabsRegistered) {
         $('tabs-checkbox').checked = args.tabsSynced;
+        dataTypeBoxes_['tabsSynced'] = args.tabsSynced;
+        $('tabs-checkbox').onclick = function() {
+          dataTypeBoxes_['tabsSynced'] = $('tabs-checkbox').checked;
+        };
         $('tabs-item').hidden = false;
       } else {
         $('tabs-item').hidden = true;
       }
 
-      this.setCheckboxesToKeepEverythingSynced_(args.syncAllDataTypes);
+      this.setDataTypeCheckboxes_(datatypeSelect.selectedIndex);
     },
 
     setEncryptionRadios_: function(args) {
@@ -417,9 +539,31 @@ cr.define('options', function() {
     showConfigure_: function(args) {
       var datatypeSelect = $('sync-select-datatypes');
       var self = this;
+
+      // Cache the sync config args so they can be reused when we transition
+      // between the drop-down menu items in the advanced settings dialog.
+      if (args)
+        this.syncConfigureArgs_ = args;
+
+      // Once the advanced sync settings dialog is visible, we transition
+      // between its drop-down menu items as follows:
+      // "Sync everything": Show encryption and passphrase sections, and disable
+      // and check all data type checkboxes.
+      // "Sync everything": Hide encryption and passphrase sections, and disable
+      // and uncheck all data type checkboxes.
+      // "Choose what to sync": Show encryption and passphrase sections, enable
+      // data type checkboxes, and restore their checked state to the last time
+      // the "Choose what to sync" was selected while the dialog was still up.
       datatypeSelect.onchange = function() {
-        var syncAll = this.selectedIndex == 0;
-        self.setCheckboxesToKeepEverythingSynced_(syncAll);
+        if (this.selectedIndex == DataTypeSelection.SYNC_NOTHING) {
+          self.showSyncNothingPage_();
+        } else {
+          self.showCustomizePage_(self.syncConfigureArgs_, this.selectedIndex);
+          if (this.selectedIndex == DataTypeSelection.SYNC_EVERYTHING)
+            self.checkAllDataTypeCheckboxes_(true);
+          else
+            self.restoreDataTypeCheckboxes_();
+        }
       };
 
       this.resetPage_('sync-setup-configure');
@@ -437,14 +581,16 @@ cr.define('options', function() {
 
         this.useEncryptEverything_ = args.encryptAllData;
 
-        // Whether to display the 'Sync everything' confirmation page or the
-        // customize data types page.
-        var syncAllDataTypes = args.syncAllDataTypes;
+        // Determine whether to display the 'OK, sync everything' confirmation
+        // dialog or the advanced sync settings dialog.
         this.usePassphrase_ = args.usePassphrase;
         this.keystoreEncryptionEnabled_ = args.keystoreEncryptionEnabled;
         if (args.showSyncEverythingPage == false || this.usePassphrase_ ||
-            syncAllDataTypes == false || args.showPassphrase) {
-          this.showCustomizePage_(args, syncAllDataTypes);
+            args.syncAllDataTypes == false || args.showPassphrase) {
+          var index = args.syncAllDataTypes ?
+                          DataTypeSelection.SYNC_EVERYTHING :
+                          DataTypeSelection.CHOOSE_WHAT_TO_SYNC;
+          this.showCustomizePage_(args, index);
         } else {
           this.showSyncEverythingPage_();
         }
@@ -469,7 +615,7 @@ cr.define('options', function() {
       $('sync-select-datatypes').selectedIndex = 0;
 
       // The default state is to sync everything.
-      this.setCheckboxesToKeepEverythingSynced_(true);
+      this.setDataTypeCheckboxes_(DataTypeSelection.SYNC_EVERYTHING);
 
       // Encrypt passwords is the default, but don't set it if the previously
       // synced account is already set to encrypt everything.
@@ -490,6 +636,30 @@ cr.define('options', function() {
     },
 
     /**
+     * Reveals the UI for when the user chooses not to sync any data types.
+     * This happens when the user signs in and selects "Sync nothing" in the
+     * advanced sync settings dialog.
+     * @private
+     */
+    showSyncNothingPage_: function() {
+      // Reset the selection to 'Sync nothing'.
+      $('sync-select-datatypes').selectedIndex = 2;

[Andrew T Wilson (Slow), 2013/05/14 16:48:27]

Consider using something other than '2' here.

[Raghu Simha, 2013/05/15 02:09:54]

Forgot to use the new enum at this spot. Done.

+
+      // Uncheck and disable the individual data type checkboxes.
+      this.checkAllDataTypeCheckboxes_(false);
+      this.setDataTypeCheckboxesEnabled_(false);
+
+      // Hide the encryption section.
+      $('customize-sync-encryption').hidden = true;
+      $('customize-sync-encryption-new').hidden = true;
+      $('sync-custom-passphrase-container').hidden = true;
+      $('sync-existing-passphrase-container').hidden = true;
+
+      // Hide the "use default settings" link.
+      $('use-default-link').hidden = true;
+    },
+
+    /**
      * Reveals the UI for entering a custom passphrase during initial setup.
      * This happens if the user has previously enabled a custom passphrase on a
      * different machine.
@@ -528,8 +698,13 @@ cr.define('options', function() {
       $('passphrase').focus();
     },
 
-    /** @private */
-    showCustomizePage_: function(args, syncEverything) {
+    /**
+     * Displays the advanced sync setting dialog, and pre-selects either the
+     * "Sync everything" or the "Choose what to sync" drop-down menu item.
+     * @param {cr.DataTypeSelection} index Index of item to pre-select.
+     * @private
+     */
+    showCustomizePage_: function(args, index) {
       $('confirm-sync-preferences').hidden = true;
       $('customize-sync-preferences').hidden = false;
 
@@ -548,12 +723,9 @@ cr.define('options', function() {
 
       $('sync-existing-passphrase-container').hidden = true;
 
-      // If the user has selected the 'Customize' page on initial set up, it's
-      // likely he intends to change the data types. Select the
-      // 'Choose data types' option in this case.
-      var index = syncEverything ? 0 : 1;
       $('sync-select-datatypes').selectedIndex = index;
-      this.setDataTypeCheckboxesEnabled_(!syncEverything);
+      this.setDataTypeCheckboxesEnabled_(
+          index == DataTypeSelection.CHOOSE_WHAT_TO_SYNC);
 
       // The passphrase input may need to take over focus from the OK button, so
       // set focus before that logic.
@@ -1027,6 +1199,10 @@ cr.define('options', function() {
   };
 
   // These methods are for general consumption.
+  SyncSetupOverlay.closeOverlay = function() {
+    SyncSetupOverlay.getInstance().closeOverlay_();
+  };
+
   SyncSetupOverlay.showErrorUI = function() {
     SyncSetupOverlay.getInstance().showErrorUI_();
   };