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..4135d0114487e9558749e686b1f912bf67b50ca0 100644
--- a/chrome/browser/resources/settings/prefs/prefs.js
+++ b/chrome/browser/resources/settings/prefs/prefs.js
@@ -4,42 +4,151 @@
 
 /**
  * @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

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

this might be better as an interface if we can't find a sane default
equals/equalsValue implementation

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

?

the sane default is == (for all primitive types)

+   * @param {!PrefObject} prefObj

[Dan Beam, 2015/08/26 16:51:33]

PrefObject -> PrefData

[stevenjb, 2015/08/26 17:32:18]

What is PrefData? PrefObject is from settingsPrivate (and will soon need to be
chrome.settingsPrivate.PrefObject)

[Dan Beam, 2015/08/26 17:38:01]

it's my proposition for a better name

[stevenjb, 2015/08/26 18:07:09]

My point is that't you're proposing a change to an existing API which means new
externs and changes to C++ code also :P
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[michaelpg, 2015/08/26 23:32:15]

I don't see how PrefObject provides more information that PrefData. I would be
comfortable changing PrefObject to Pref in a later CL if that's more natural but
otherwise I'm inclined to leave it be.

+   */
+  function PrefState(prefObj) {

[Dan Beam, 2015/08/26 16:51:33]

PrefState -> Pref, PrefWrapper

[michaelpg, 2015/08/26 23:32:15]

Done: PrefState -> PrefWrapper.

+    this.value = prefObj.value;

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

why does this have to be public?

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

Doesn't. Done.

+  }
+
+  /**
+   * Checks if the value matches this pref's value.
+   * @param {*} value
+   * @return {boolean}
+   */
+  PrefState.prototype.equalsValue = function(value) {

[Dan Beam, 2015/08/26 16:51:33]

PrefState.prototype.equals = function(other) {
  return this.value == other.value;
};

[michaelpg, 2015/08/26 23:32:15]

As I mentioned in a previous comment, PrefState (PrefWrapper) is being compared
with a settingsPrivate.PrefObject. It would be misleading for a function called
"equals" to compare a property of two different types, wouldn't it?

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

construct a PrefWrapper() around the PrefState and compare

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

Done. I didn't have that because it seemed like extra work for no reason. But I
trust your judgment.

+    return this.value == value;

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

how do we prevent PrefStates of different types from comparing against
eachother?  (i.e. an integer pref of 1 compared to a list of ['1'] would ==
eachother.)  right now it seems like it's up to the comparison order.

maybe === is appropriate in this case?  do we have cases where the C++ gives
back '1' when the UI has 1 (or other types of stringification or type variance)?

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

Prefs are typed in their PrefRegistry as a specific base::Value subtype, so they
should not be doing this. IOW, it is sane for PrefState to assume the types are
the same.

+  };
+
+  /**
+   * @constructor
+   * @extends {PrefState}
+   * @param {!PrefObject} prefObj
+   */
+  function ListPrefState(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();
+  }
+
+  ListPrefState.__proto__ = PrefState.prototype;

[Dan Beam, 2015/08/26 16:51:33]

ListPrefState.prototype = {
  __proto__: PrefState.prototype,

  ... less ClassName.prototype cruft ...
};

[michaelpg, 2015/08/26 23:32:15]

Done.

+
+  /** @override */
+  ListPrefState.prototype.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.
+   */
+  ListPrefState.prototype.arraysEqual_ = function(arr1, arr2) {

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

could this be generalized to work for primitives as well?

+    if (!arr1 || !arr2)

[Dan Beam, 2015/08/26 16:51:33]

empty arrays are truthy, btw

is it valid to get here without an array?

[stevenjb, 2015/08/26 17:32:18]

PrefObject.value should never be null, so I believe we can/should assume that
PrefState (or PrefObject) .value is also never null. That way we don't have to
worry about whether null == [].

[michaelpg, 2015/08/26 23:32:15]

Seems unlikely but it depends on how prefs are structured. 

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

this opens a lot of holes.  0 == [''] for example

+      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 (val1 instanceof Array && !this.arraysEqual_(val1, val2))

[Dan Beam, 2015/08/26 16:51:33]

nit: Array.isArray(val1)

[michaelpg, 2015/08/26 23:32:15]

Done.

+        return false;
+      else if (val1 != val2)
+        return false;
+    }
+    return true;

[Dan Beam, 2015/08/26 16:51:33]

why not

  return arr1.join() == arr2.join();

if we don't need to support nesting, or:

  return JSON.stringify(arr1) == JSON.stringify(arr2);

if we do?

[stevenjb, 2015/08/26 17:32:18]

The C++ coder in me says "Eww". :) I know it's pretty unlikely, but wouldn't the
first equate ["A","","B"] with ["A,","B"]? JSON.stringify should be robust since
we rely on it for prefs serialization, but I might be a little concerned with
the performance (but probably it doesn't matter).

[michaelpg, 2015/08/26 23:32:15]

Not sure what you're going for: the second isn't a valid array. But these two
non-equivalent arrays would be joined to the same string:

["A", ",B"], ["A,", "B"]
Yeah, it's kind of slow.
Yeah, it kind of doesn't.

[michaelpg, 2015/08/26 23:32:15]

JSON.stringify doesn't guarantee ordering of object property keys, so arrays of
dictionaries may not compare correctly.

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

how does your current code handle arrays of dictionaries?

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

It doesn't. So I'll remove this and we can restart this discussion later if we
add it back in.

+  };
+
   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 PrefState objects representing the state of the
+       * Chrome pref store.
+       * @type {Object<PrefState>}
+       */
+      settingsPrivateState_: {
+        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
+     */
+    prefsChanged_: function(change) {
+      if (!CrSettingsPrefs.isInitialized)
+        return;
+
+      var key = this.getPrefKeyFromPath_(change.path);
+      var prefState = this.settingsPrivateState_[key];
+      if (!prefState)
+        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 (!prefState.equalsValue(prefObj.value)) {

[stevenjb, 2015/08/26 17:32:18]

nit: Early exit would better match the comment.

[michaelpg, 2015/08/26 23:32:15]

Done.

+        chrome.settingsPrivate.setPref(
+            key,
+            prefObj.value,
+            /* pageId */ '',
+            /* callback */ this.setPrefCallback_.bind(this, key));
+      }
     },
 
     /**
@@ -47,8 +156,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 +166,106 @@
      * @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.
+     */
+    setPrefCallback_: function(key, success) {
+      if (!success) {

[Dan Beam, 2015/08/26 16:51:33]

if (success)
  return;

...

[michaelpg, 2015/08/26 23:32:15]

Done.

+        // Get the current pref value from chrome.settingsPrivate to ensure the
+        // UI stays up to date.
+        chrome.settingsPrivate.getPref(
+            key,
+            function(pref) {
+              assert(pref);
+              this.updatePrefs_([pref]);
+            }.bind(this));

[Dan Beam, 2015/08/26 16:51:33]

chrome.settingsPrivate.getPref(key, function(pref) {
  this.updatePrefs_([pref]);
}.bind(this));

[michaelpg, 2015/08/26 23:32:15]

I don't think that's more readable but it's more compact, so, done.

Note that if the callback is called without a pref object we'll throw uncaught
exceptions further up the stack, but the API should also log an error in that
case so we should be ok.

+      }
+    },
 
     /**
-     * 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];
+    updatePrefs_: function(prefs) {
+      prefs.forEach(function(newPrefObj) {
+        // Set or update the PrefState in settingsPrivateState_ with the
+        // PrefObject from settingsPrivate.
+        this.setPrefState_(newPrefObj);
 
-          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']);
-        }
+        // 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.
+     * Returns the key of the preference specified by the path. The path may
+     * include tokens after the key, e.g. 'prefs.[key].value'.
+     * @param {string} path
+     * @return {string}

[Dan Beam, 2015/08/26 16:51:33]

all _ -> @private

[michaelpg, 2015/08/26 23:32:15]

Done.

      */
-    shouldUpdateListPrefValue_: function(root, newValue) {
-      if (root.value == null ||
-          root.value.length != newValue.length) {
-        return true;
-      }
+    getPrefKeyFromPath_: function(path) {

[Dan Beam, 2015/08/26 16:51:33]

i have no idea what this does, maybe give some concrete examples?

[michaelpg, 2015/08/26 23:32:15]

Done.

+      var tokens = path.split('.');

[Dan Beam, 2015/08/26 16:51:33]

path.split('.').slice(1);

[michaelpg, 2015/08/26 23:32:15]

Done.

+      var key = '';
 
-      for (let i = 0; i < newValue.length; i++) {
-        if (root.value != null && root.value[i] != newValue[i]) {
-          return true;
-        }
+      // Skip the first token, which refers to the member variable (this.prefs).
+      for (let i = 1; i < tokens.length; i++) {
+        if (key)
+          key += '.';
+        key += tokens[i];
+        // The settingsPrivateState_ keys match the pref keys.
+        if (this.settingsPrivateState_[key] != undefined)
+          return key;
       }
+      return '';
+    },
 

[Dan Beam, 2015/08/26 16:51:33]

doc

[michaelpg, 2015/08/26 23:32:15]

Done.

-      return false;
+    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.
+        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);
+
+        // 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);
+

[stevenjb, 2015/08/26 17:32:18]

nit: Add comment that notification will occur here.

[michaelpg, 2015/08/26 23:32:15]

Done.

+        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/
-     * @private
+     * Creates a PrefState object from a chrome.settingsPrivate pref and adds it
+     * to settingsPrivateState_.
+     * @param {!PrefObject} PrefObject received from chrome.settingsPrivate.
      */
-    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() {});
-      });
+    setPrefState_: function(prefObj) {
+      var prefState;
+      if (prefObj.type == chrome.settingsPrivate.PrefType.LIST)
+        prefState = new ListPrefState(prefObj);
+      else
+        prefState = new PrefState(prefObj);

[Dan Beam, 2015/08/26 16:51:33]

nit: ternary

var prefState = prefObj.type == chrome.settingsPrivate.PrefType.LIST ?
    new ListPrefState(prefObj) : new PrefState(prefObj);

[michaelpg, 2015/08/26 23:32:15]

If we add object prefs, we'd have to change this back.

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

we're not yet.  do it then

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

Done.

+      this.settingsPrivateState_[prefObj.key] = prefState;
     },
   });
 })();