Refactor prefs.js for MD-Settings

chrome/browser/resources/settings/prefs/prefs.js

Index: chrome/browser/resources/settings/prefs/prefs.js
diff --git a/chrome/browser/resources/settings/prefs/prefs.js b/chrome/browser/resources/settings/prefs/prefs.js
index b68ccea3c5c9b26ee7608800145cf458189e34af..42b90bb98493f78572648706d573e1698f329480 100644
--- a/chrome/browser/resources/settings/prefs/prefs.js
+++ b/chrome/browser/resources/settings/prefs/prefs.js
@@ -4,42 +4,156 @@
 
 /**
  * @fileoverview
- * 'cr-settings-prefs' is an element which serves as a model for
- * interaction with settings which are stored in Chrome's
- * Preferences.
+ * 'cr-settings-prefs' models Chrome settings and preferences, listening for
+ * changes to Chrome prefs whitelisted in chrome.settingsPrivate.
+ * When changing prefs in this element's 'prefs' property via the UI, this
+ * element tries to set those preferences in Chrome. Whether or not the calls to
+ * settingsPrivate.setPref succeed, 'prefs' is eventually consistent with the
+ * Chrome pref store.
  *
  * Example:
  *
- *    <cr-settings-prefs id="prefs"></cr-settings-prefs>
- *    <cr-settings-a11y-page prefs="{{this.$.prefs}}"></cr-settings-a11y-page>
+ *    <cr-settings-prefs prefs="{{prefs}}"></cr-settings-prefs>
+ *    <cr-settings-a11y-page prefs="{{prefs}}"></cr-settings-a11y-page>
  *
  * @group Chrome Settings Elements
- * @element cr-settings-a11y-page
+ * @element cr-settings-prefs
  */
 (function() {
   'use strict';
 
+  /**
+   * Pref state object. Copies values of PrefObjects received from
+   * settingsPrivate to determine when pref values have changed.
+   * This prototype works for primitive types, but more complex types should
+   * override these functions.
+   * @constructor
+   * @param {!PrefObject} prefObj
+   */
+  function PrefWrapper(prefObj) {
+    this.value = prefObj.value;
+  }
+
+  /**
+   * Checks if the value matches this pref's value.
+   * @param {*} value
+   * @return {boolean}
+   */
+  PrefWrapper.prototype.equalsValue = function(value) {
+    return this.value == value;
+  };
+
+  /**
+   * @constructor
+   * @extends {PrefWrapper}
+   * @param {!PrefObject} prefObj
+   */
+  function ListPrefWrapper(prefObj) {
+    // Copy the array so changes to prefObj aren't reflected in this.value.
+    // TODO(michaelpg): Do a deep copy to support nested lists and objects.
+    this.value = prefObj.value.slice();
+  }
+
+  ListPrefWrapper.prototype = {
+    __proto__: PrefWrapper.prototype,
+
+    /** @override */
+    equalsValue: function(value) {
+      // Two arrays might have the same values, so don't just use "==".
+      return this.arraysEqual_(this.value, value);
+    },
+
+    /**
+     * Tests whether two arrays contain the same data (true if primitive
+     * elements are equal and array elements contain the same data).
+     * @param {Array} arr1
+     * @param {Array} arr2
+     * @return {boolean} True if the arrays contain similar values.
+     * @private
+     */
+    arraysEqual_: function(arr1, arr2) {
+      if (!arr1 || !arr2)
+        return arr1 == arr2;
+      if (arr1.length != arr2.length)
+        return false;
+      for (let i = 0; i < arr1.length; i++) {
+        var val1 = arr1[i];
+        var val2 = arr2[i];
+        assert(typeof val1 != 'object' && typeof val2 != 'object',
+               'Objects are not supported.');
+        if (Array.isArray(val1) && !this.arraysEqual_(val1, val2))
+          return false;
+        else if (val1 != val2)
+          return false;
+      }
+      return true;
+    },
+  };
+
   Polymer({
     is: 'cr-settings-prefs',
 
     properties: {
       /**
-       * Object containing all preferences.
+       * Object containing all preferences, for use by Polymer controls.
        */
-      prefStore: {
+      prefs: {
         type: Object,
         value: function() { return {}; },
         notify: true,
       },
+
+      /**
+       * Map of pref keys to PrefWrapper objects representing the state of the
+       * Chrome pref store.
+       * @type {Object<PrefWrapper>}
+       * @private
+       */
+      settingsPrivateState_: {
+        type: Object,
+        value: function() { return {}; },

[Dan Beam, 2015/08/28 00:08:41]

value: {},  // does this have issues across prototypes?

[michaelpg, 2015/08/28 01:52:48]

it has issues across objects using this prototype, yes. If we create two
<cr-settings-prefs>, each will have a reference to the same object.
https://www.polymer-project.org/1.0/docs/devguide/properties.html#configure-v...

We have no reason to make multiple <cr-settings-prefs> instances, but I don't
think we should ignore the possibility unless we also want to enforce a
singleton policy.

+      },
     },
 
+    observers: [
+      'prefsChanged_(prefs.*)',
+    ],
+
     /** @override */
     created: function() {
       CrSettingsPrefs.isInitialized = false;
 
       chrome.settingsPrivate.onPrefsChanged.addListener(
-          this.onPrefsChanged_.bind(this));
-      chrome.settingsPrivate.getAllPrefs(this.onPrefsFetched_.bind(this));
+          this.onSettingsPrivatePrefsChanged_.bind(this));
+      chrome.settingsPrivate.getAllPrefs(
+          this.onSettingsPrivatePrefsFetched_.bind(this));
+    },
+
+    /**
+     * Polymer callback for changes to this.prefs.
+     * @param {!{path: string, value: *}} change
+     * @private
+     */
+    prefsChanged_: function(change) {
+      if (!CrSettingsPrefs.isInitialized)
+        return;
+
+      var key = this.getPrefKeyFromPath_(change.path);
+      var prefState = this.settingsPrivateState_[key];
+      if (!prefState)
+        return;

[Dan Beam, 2015/08/28 00:08:41]

\n

[michaelpg, 2015/08/28 01:52:48]

Done.

+      var prefObj = this.get(key, this.prefs);
+
+      // If settingsPrivate already has this value, do nothing. (Otherwise,
+      // a change event from settingsPrivate could make us call
+      // settingsPrivate.setPref and potentially trigger an IPC loop.)
+      if (prefState.equalsValue(prefObj.value))
+        return;

[Dan Beam, 2015/08/28 00:08:41]

\n

[michaelpg, 2015/08/28 01:52:48]

Done.

+      chrome.settingsPrivate.setPref(
+          key,
+          prefObj.value,
+          /* pageId */ '',
+          /* callback */ this.setPrefCallback_.bind(this, key));
     },
 
     /**
@@ -47,8 +161,9 @@
      * @param {!Array<!PrefObject>} prefs The prefs that changed.
      * @private
      */
-    onPrefsChanged_: function(prefs) {
-      this.updatePrefs_(prefs, false);
+    onSettingsPrivatePrefsChanged_: function(prefs) {
+      if (CrSettingsPrefs.isInitialized)
+        this.updatePrefs_(prefs);
     },
 
     /**
@@ -56,106 +171,115 @@
      * @param {!Array<!PrefObject>} prefs
      * @private
      */
-    onPrefsFetched_: function(prefs) {
-      this.updatePrefs_(prefs, true);
+    onSettingsPrivatePrefsFetched_: function(prefs) {
+      this.updatePrefs_(prefs);
 
       CrSettingsPrefs.isInitialized = true;
       document.dispatchEvent(new Event(CrSettingsPrefs.INITIALIZED));
     },
 
+    /**
+     * Checks the result of calling settingsPrivate.setPref.
+     * @param {string} key The key used in the call to setPref.
+     * @param {boolean} success True if setting the pref succeeded.
+     * @private
+     */
+    setPrefCallback_: function(key, success) {
+      if (success)
+        return;

[Dan Beam, 2015/08/28 00:08:41]

\n

[michaelpg, 2015/08/28 01:52:48]

Done.

+      // Get the current pref value from chrome.settingsPrivate to ensure the
+      // UI stays up to date.
+      chrome.settingsPrivate.getPref(key, function(pref) {
+        this.updatePrefs_([pref]);
+      }.bind(this));
+    },
 
     /**
-     * Updates the settings model with the given prefs.
+     * Updates the prefs model with the given prefs.
      * @param {!Array<!PrefObject>} prefs
-     * @param {boolean} shouldObserve Whether each of the prefs should be
-     *     observed.
      * @private
      */
-    updatePrefs_: function(prefs, shouldObserve) {
-      prefs.forEach(function(prefObj) {
-        let root = this.prefStore;
-        let tokens = prefObj.key.split('.');
-
-        assert(tokens.length > 0);
-
-        for (let i = 0; i < tokens.length; i++) {
-          let token = tokens[i];
-
-          if (!root.hasOwnProperty(token)) {
-            let path = 'prefStore.' + tokens.slice(0, i + 1).join('.');
-            this.set(path, {});
-          }
-          root = root[token];
-        }
-
-        // NOTE: Do this copy rather than just a re-assignment, so that the
-        // observer fires.
-        for (let objKey in prefObj) {
-          let path = 'prefStore.' + prefObj.key + '.' + objKey;
-
-          // Handle lists specially. We don't want to call this.set()
-          // unconditionally upon updating a list value, since even its contents
-          // are the same as the old list, doing this set() may cause an
-          // infinite update cycle (http://crbug.com/498586).
-          if (objKey == 'value' &&
-              prefObj.type == chrome.settingsPrivate.PrefType.LIST &&
-              !this.shouldUpdateListPrefValue_(root, prefObj['value'])) {
-            continue;
-          }
+    updatePrefs_: function(prefs) {
+      prefs.forEach(function(newPrefObj) {
+        // Set or update the PrefWrapper in settingsPrivateState_ with the
+        // PrefObject from settingsPrivate.
+        this.setPrefWrapper_(newPrefObj);

[Dan Beam, 2015/08/28 00:08:41]

change the name of setPrefWrapper_ if... it's not a setter that takes a type of
PrefWrapper

[michaelpg, 2015/08/28 01:52:48]

Done.

 
-          this.set(path, prefObj[objKey]);
-        }
-
-        if (shouldObserve) {
-          Object.observe(root, this.propertyChangeCallback_, ['update']);
-        }
+        // Set or update the pref in |prefs|. This triggers observers in the UI,
+        // which update controls associated with the pref.
+        this.setPref_(newPrefObj);
       }, this);
     },
 
-
     /**
-     * @param {Object} root The root object for a pref that contains a list
-     *     value.
-     * @param {!Array} newValue The new list value.
-     * @return {boolean} Whether the new value is different from the one in
-     *     root, thus necessitating a pref update.
+     * Given a 'property-changed' path, returns the key of the preference the
+     * path refers to. E.g., if the path of the changed property is
+     * 'prefs.search.suggest_enabled.value', the key of the pref that changed is
+     * 'search.suggest_enabled'.
+     * @param {string} path
+     * @return {string}
+     * @private
      */
-    shouldUpdateListPrefValue_: function(root, newValue) {
-      if (root.value == null ||
-          root.value.length != newValue.length) {
-        return true;
-      }
+    getPrefKeyFromPath_: function(path) {
+      // Skip the first token, which refers to the member variable (this.prefs).
+      var tokens = path.split('.').slice(1);
+      var key = '';
 
-      for (let i = 0; i < newValue.length; i++) {
-        if (root.value != null && root.value[i] != newValue[i]) {
-          return true;
-        }
+      for (let token of tokens) {

[Dan Beam, 2015/08/28 00:08:41]

nit:

for (let i = 0; i < tokens.length; ++i) {
  let key = tokens.slice(0, i).join('.');
  if (this.settingsPrivateState_[key] != undefined)
    return key;
}

[michaelpg, 2015/08/28 01:52:48]

Done, but for i from [1, tokens.length].

+        if (key)
+          key += '.';
+        key += token;
+        // The settingsPrivateState_ keys match the pref keys.
+        if (this.settingsPrivateState_[key] != undefined)
+          return key;
       }
+      return '';
+    },
+
+    /**
+     * Sets or updates the pref denoted by newPrefObj.key in the publicy exposed
+     * |prefs| property.
+     * @param {PrefObject} newPrefObj The pref object to update the pref with.
+     * @private
+     */
+    setPref_: function(newPrefObj) {
+      // Check if the pref exists already in the Polymer |prefs| object.
+      if (this.get(newPrefObj.key, this.prefs)) {
+        // Update just the value, notifying listeners of the change.
+        this.set('prefs.' + newPrefObj.key + '.value', newPrefObj.value);
+      } else {
+        // Add the pref to |prefs|.
+        let node = this.prefs;
+        let tokens = newPrefObj.key.split('.').slice(0, -1);

[Dan Beam, 2015/08/28 00:08:41]

tokens -> keyParts

[michaelpg, 2015/08/28 01:52:48]

Done.

 
-      return false;
+        // Crawl the pref tree, generating objects where necessary.
+        tokens.forEach(function(token) {
+          if (!node.hasOwnProperty(token)) {
+            // Don't use Polymer.Base.set because the property update events
+            // won't be useful until the actual pref is set below.
+            node[token] = {};
+          }
+          node = node[token];
+        }, this);

[Dan Beam, 2015/08/28 00:08:41]

why not use?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

[michaelpg, 2015/08/28 01:52:48]

Done, thanks.

+
+        // Set the actual preference, notifying listeners of the change.
+        this.set('prefs.' + newPrefObj.key, newPrefObj);
+      }
     },
 
     /**
-     * Called when a property of a pref changes.
-     * @param {!Array<!Object>} changes An array of objects describing changes.
-     *     @see http://www.html5rocks.com/en/tutorials/es7/observe/
+     * Creates a PrefWrapper object from a chrome.settingsPrivate pref and adds

[Dan Beam, 2015/08/28 00:08:41]

{create,make}PrefWrapper_?

general nit: if the doc comment varies from the method name, one should probably
win

[michaelpg, 2015/08/28 01:52:48]

Done.

+     * the PrefWrapper to settingsPrivateState_.
+     * @param {!PrefObject} PrefObject received from chrome.settingsPrivate.
      * @private
      */
-    propertyChangeCallback_: function(changes) {
-      changes.forEach(function(change) {
-        // UI should only be able to change the value of a setting for now, not
-        // disabled, etc.
-        assert(change.name == 'value');
-
-        let newValue = change.object[change.name];
-        assert(newValue !== undefined);
-
-        chrome.settingsPrivate.setPref(
-            change.object['key'],
-            newValue,
-            /* pageId */ '',
-            /* callback */ function() {});
-      });
+    setPrefWrapper_: function(prefObj) {
+      var prefState;
+      if (prefObj.type == chrome.settingsPrivate.PrefType.LIST)
+        prefState = new ListPrefWrapper(prefObj);
+      else
+        prefState = new PrefWrapper(prefObj);
+      this.settingsPrivateState_[prefObj.key] = prefState;
     },
   });
 })();