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..6813cd677b663645387fd5756d446fb1a609b2a0 100644
--- a/chrome/browser/resources/settings/prefs/prefs.js
+++ b/chrome/browser/resources/settings/prefs/prefs.js
@@ -4,42 +4,157 @@
 
 /**
  * @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.key_ = prefObj.key_;
+    this.value_ = prefObj.value;
+  }
+
+  /**
+   * Checks if other's value equals this.value_. Both prefs wrapped by the
+   * PrefWrappers should have the same keys.
+   * @param {PrefWrapper} other
+   * @return {boolean}
+   */
+  PrefWrapper.prototype.equals = function(other) {
+    assert(this.key_ == other.key_);
+    return this.value_ == other.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
+     * @param {ListPrefWrapper} other
+     * @return {boolean}
+     */
+    equals: function(other) {
+      assert(this.key_ == other.key_);
+      // Two arrays might have the same values, so don't just use "==".

[stevenjb, 2015/08/28 17:35:20]

This comment doesn't make much sense to me. IIUC, Array/Object == tests that
both are instances of the same object, so it should be clear that a special
comparator is needed. i.e. the comment just confuses me :)
Also, since this function is otherwise trivial and arraysEqual_ is called only
once, maybe combine them?

[michaelpg, 2015/08/28 18:51:45]

Done.

+      return this.arraysEqual_(this.value_, other.value_);
+    },
+
+    /**
+     * Tests whether two arrays contain the same primitives.
+     * @param {Array} arr1
+     * @param {Array} arr2
+     * @return {boolean} True if the arrays contain equal values.
+     * @private
+     */
+    arraysEqual_: function(arr1, arr2) {
+      if (arr1.length != arr2.length)
+        return false;
+      for (let i = 0; i < arr1.length; i++) {
+        if (arr1[i] != arr2[i])
+          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
+       */
+      settingsPrivateWrappers_: {

[Dan Beam, 2015/08/28 02:05:36]

nit: wrappers_ or prefWrappers_

[stevenjb, 2015/08/28 17:35:21]

FWIW I personally prefer the more explicit name to identify the purpose of the
second dictionary.

[michaelpg, 2015/08/28 18:51:45]

Done.

[michaelpg, 2015/08/28 18:51:45]

Acknowledged.

+        type: Object,
+        value: function() { return {}; },
+      },
     },
 
+    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 prefWrapper = this.settingsPrivateWrappers_[key];

[Dan Beam, 2015/08/28 02:05:36]

evidence that settingsPrivateWrappers_ should be named prefWrappers_ :)

[michaelpg, 2015/08/28 18:51:45]

Done.

+      if (!prefWrapper)

[Dan Beam, 2015/08/28 02:05:36]

out of curiosity, when should this happen?  never?  can we assert() if so?

[stevenjb, 2015/08/28 17:35:21]

IIUC, this would happen if UI referenced a pref that doesn't exist in
settingsPrivate. That being the case, while it might be a little annoying for
development, it would prevent non-functional UI elements, so I agree that we
should assert here.

[michaelpg, 2015/08/28 18:51:46]

<cr-settings-pref-tracker> detects this case more reliably, so an assert here
would at best be redundant.

+        return;
+
+      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 (prefWrapper.equals(this.createPrefWrapper_(prefObj)))
+        return;
+
+      chrome.settingsPrivate.setPref(
+          key,
+          prefObj.value,
+          /* pageId */ '',
+          /* callback */ this.setPrefCallback_.bind(this, key));
     },
 
     /**
@@ -47,8 +162,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 +172,100 @@
      * @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;
+
+      // 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;
-          }
-
-          this.set(path, prefObj[objKey]);
-        }
-
-        if (shouldObserve) {
-          Object.observe(root, this.propertyChangeCallback_, ['update']);
-        }
+    updatePrefs_: function(prefs) {
+      prefs.forEach(function(newPrefObj) {
+        // Use the PrefObject from settingsPrivate to create a PrefWrapper in
+        // settingsPrivateWrappers_ at the pref's key.
+        this.settingsPrivateWrappers_[newPrefObj.key] =
+            this.createPrefWrapper_(newPrefObj);
+
+        // 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

[Dan Beam, 2015/08/28 02:05:36]

why can this just be

  return path.split('.').slice(1, -1);

is there not always a "prefs." and ".value"?

i don't love the automagic object property searching

[stevenjb, 2015/08/28 17:35:20]

Until / unless we support dictionary objects, I think Dan is correct, and
simpler is better. We already verify that the path exists in
settingsPrivateWrappers where this gets called, so it would probably be simpler
to eliminate this altogether.

[Dan Beam, 2015/08/28 17:42:05]

can't**
we could also do:

  var parts = path.split('.');
  assert(parts.shift() == 'prefs');
  assert(parts.pop() == 'value');
  return parts.join('.');

[michaelpg, 2015/08/28 18:51:45]

There is always a "prefs." and a ".value", but after the ".value" there may be
".splices" or ".length" for arrays splices, or \.\d (".0", ".1", etc.) for
changes to array elements.

+     * '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);
 
-      for (let i = 0; i < newValue.length; i++) {
-        if (root.value != null && root.value[i] != newValue[i]) {
-          return true;
-        }
+      for (let i = 1; i <= tokens.length; i++) {
+        let key = tokens.slice(0, i).join('.');
+        // The settingsPrivateWrappers_ keys match the pref keys.
+        if (this.settingsPrivateWrappers_[key] != undefined)
+          return key;
       }
+      return '';
+    },
 
-      return false;
+    /**
+     * 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);

[Dan Beam, 2015/08/28 02:05:36]

it's unfortunate that this auto expanding object stuff doesn't happen for us in
Polymer.  but i guess then you wouldn't be able to create properties with dots
in them.

[michaelpg, 2015/08/28 18:51:46]

yup. in reality we should be transforming dots to some other separator, but we
decided not to for a few reasons: this maintains the ability to grep for
preference keys, and is easier for people modifying settings pages to understand
(at the expense of those of us modifying the core prefs.js stuff).

+      } else {
+        // Add the pref to |prefs|. cr.exportPath doesn't use Polymer.Base.set,
+        // which is OK because the nested property update events aren't useful.
+        cr.exportPath(newPrefObj.key, newPrefObj, this.prefs);
+        // Notify listeners of the change at the preference key.
+        this.notifyPath('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.
+     * @param {!PrefObject} PrefObject received from chrome.settingsPrivate.
+     * @return {PrefWrapper} An object containing a copy of the PrefObject's
+     *     value.
      * @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() {});
-      });
+    createPrefWrapper_: function(prefObj) {
+      return (prefObj.type == chrome.settingsPrivate.PrefType.LIST) ?

[Dan Beam, 2015/08/28 02:05:36]

nit: remove extra ()

[michaelpg, 2015/08/28 18:51:45]

Done.

+          new ListPrefWrapper(prefObj) : new PrefWrapper(prefObj);
     },
   });
 })();