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..4f28116267ec65b44de96a6f1e2cb44ac2f3a980 100644
--- a/chrome/browser/resources/settings/prefs/prefs.js
+++ b/chrome/browser/resources/settings/prefs/prefs.js
@@ -19,6 +19,178 @@
 (function() {
   'use strict';
 
+  // TODO(michaelpg): Remove once prefs.js is complete and well-tested. Used
+  // to catch errors like crbug.com/521884.
+  var verifySetPref = function(result) {
+    assert(result, 'Failed to set preference.');
+  };
+
+  /**
+   * @constructor
+   * @param {!PrefObject} prefObj

[Dan Beam, 2015/08/21 17:13:19]

PrefObject -> PrefData

[michaelpg, 2015/08/26 06:02:26]

Why? PrefObject is defined in the settings_private.js externs and is the type
that this function should be passed.

+   */
+  function Pref(prefObj) {

[Dan Beam, 2015/08/21 17:13:18]

prefObj -> data

[Dan Beam, 2015/08/21 17:13:18]

this.data_ = data;

+    this.key = prefObj.key;
+    this.update(prefObj);

[Dan Beam, 2015/08/21 17:13:18]

Object.observe(this.data_, this.dataChanged_.bind(this), ['update']);

[michaelpg, 2015/08/26 06:02:27]

Object.observe is being deprecated

+  }
+
+    // Check if the values are different.
+  Pref.prototype.equals = function(oldValue, newValue) {

[Dan Beam, 2015/08/21 17:13:19]

Pref.prototype.equals = function(otherPref) {
  return this.type == otherPref.type && this.value == otherPref.value;
};

[michaelpg, 2015/08/26 06:02:26]

It's comparing PrefState and PrefObject, so I think it makes more sense to
compare the values themselves. Updated to make that explicit.

+    return oldValue == newValue;
+  };
+
+  /**
+   * Update the pref with the value of prefObj when set by settingsPrivate.
+   * @return {boolean} True if the value has changed since last set by Polymer.
+   */
+  Pref.prototype.update = function(prefObj) {
+    // TODO: separate.
+    //
+    this.cachedValue = prefObj.value;
+    if (this.equals(this.value, prefObj.value))

[Dan Beam, 2015/08/21 17:13:18]

if (this.equals(prefObj))

+      return false;
+
+    this.value = prefObj.value;
+    return true;
+  };
+
+  /**
+   * Registers an observer on the PrefObject using Object.observe.
+   * @param {!PrefObject} prefObj The preference to observe.
+   */
+  Pref.prototype.observe = function(prefObj) {

[Dan Beam, 2015/08/21 17:13:18]

Pref.prototype.observe_ = function() {
  Object.observe(this.data_, this.dataChanged_.bind(this), ['update']);
};

+    Object.observe(prefObj, this.prefChangeCallback_.bind(this),
+        ['update']);
+  };
+
+  /**
+   * Called when a property of a pref changes.
+   * @param {!Array<!{object: PrefObject, name: string}>} changes
+   *     Array of change records.
+   * @private
+   */
+  Pref.prototype.prefChangeCallback_ = function(changes) {
+    // Rarely, multiple changes could refer to the same value (e.g., toggling
+    // a boolean preference twice), causing redundant work. We could ignore
+    // all but the last such change, but that would muddle the data flow.
+    changes.forEach(function(change) {
+      // UI should only be able to change the value of a preference.
+      assert(change.name == 'value');
+      let newValue = change.object.value;
+      assert(newValue != undefined);
+
+      // Update the known value for the sake of this.update.
+      this.value = newValue;
+      // (this isn't observed, the original prefObj in prefStore is)
+
+      // If settingsPrivate already has this value, do nothing. (Otherwise,
+      // a change event from settingsPrivate could make us call
+      // settingsPrivate.setPref. This can start an infinite IPC loop if there
+      // are multiple Settings windows and the pref is changed.)
+      // todo
+      if (this.equals(this.cachedValue, newValue))
+        return;
+
+      chrome.settingsPrivate.setPref(
+          this.key,
+          newValue,
+          /* pageId */ '',
+          /* callback */ verifySetPref);
+    }, this);
+  };
+
+  Pref.prototype.observeValue = function(prefObj) {
+    // prefObj is already observed, and value is a primitive.
+  };
+
+  /**
+   * @constructor
+   * @extends {ListPref}
+   * @param {!PrefObject} prefObj
+   */
+  function ListPref(prefObj) {
+    List.call(this, prefObj);

[Dan Beam, 2015/08/21 17:13:19]

Pref.call?

[michaelpg, 2015/08/26 06:02:26]

Changed.

+  }
+
+  ListPref.__proto__ = Pref.prototype;

[Dan Beam, 2015/08/21 17:13:19]

nit:

  ListPref.prototype = {
    __proto__: Pref.prototype,
    ...
  };

[michaelpg, 2015/08/26 06:02:27]

OK. I'll take a closer look at this tomorrow.

+
+  /** @override */
+  ListPref.prototype.equals = function(oldValue, newValue) {
+    if (this.type == chrome.settingsPrivate.PrefType.LIST) {
+      // Two arrays might have the "same" values, so don't just use "==".
+      return this.arraysEqual_(oldValue, newValue);
+    }
+  };
+
+  /** @override */
+  ListPref.prototype.observeValue = function(prefObj) {
+    // If the value changed to a new array, observe the new array.
+    Object.observe(prefObj.value,
+                   this.valueChangeCallback_.bind(this, this.key),
+                   ['update', 'splice']);
+  };
+
+  /** @override */
+  ListPref.prototype.update = function(prefObj) {
+    // Retain a copy of the array for settingsPrivate comparison purposes.
+    this.cachedValue = prefObj.value.slice();
+
+    // functional equivalence
+    var changed = this.equals(this.value, prefObj.value);
+    // identity
+    this.value = prefObj.value;
+    return changed;
+  };
+
+  /**
+   * Called when the array value is changed.
+   * @param {!Array<{object: !Array}>} changes The list of change records.
+   */
+  Pref.prototype.valueChangeCallback_ = function(changes) {
+    // The underlying array is the same for all changes.
+    var value = changes[0].object;
+    // TODO: the array could no longer be the set property...
+
+    // this.value should already refer to the prefObj.value in prefStore,
+    // so nothing to do here
+    // (if the value itself is set to a different array, we'll have observed
+    // that and handled it in ListPref.prototype.prefChangeCallback)
+    // TODO: verify setPref won't be called in both functions (the functions
+    // can't be called on the same change)
+
+    // If settingsPrivate already has this value, do nothing.
+    if (this.arraysEqual_(this.cachedValue, value))
+      return;
+    chrome.settingsPrivate.setPref(this.key, value,
+                                   /* pageId */ '',
+                                   /* callback */ verifySetPref);
+  };
+
+  /**
+   * 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.
+   */
+  ListPref.prototype.arraysEqual_ = function(arr1, arr2) {

[Dan Beam, 2015/08/21 17:13:19]

maybe crazy question, but could ListPref contain an Array<Pref>?

that way, we could just do:

  if (this.data_.length != otherPref.data_.length)
    return false;

  for (var i = 0; i < this.data_.length; ++i) {
    if (!this.data_[i].equals(otherPref.data_[i]))
      return false;
  }

  return true;

[michaelpg, 2015/08/26 06:02:26]

Take a look at the new CL and let me know what you think -- we're comparing
apples and oranges :-\

+    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 (val1 instanceof Array && !this.arraysEqual_(val1, val2))
+        return false;
+      else if (val1 != val2)
+        return false;
+    }
+    return true;
+  };
+
   Polymer({
     is: 'cr-settings-prefs',
 
@@ -36,6 +208,9 @@
     /** @override */
     created: function() {
       CrSettingsPrefs.isInitialized = false;
+      // Private counterpart to prefStore, used to interact with
+      // settingsPrivate.
+      this.prefCache_ = {};

[Dan Beam, 2015/08/21 17:13:18]

nit: this.prefs_ or this.state_ if this is long-lived

or this.pending_ if they're only while inflight

[michaelpg, 2015/08/26 06:02:27]

-> this.settingsPrivateState_

 
       chrome.settingsPrivate.onPrefsChanged.addListener(
           this.onPrefsChanged_.bind(this));
@@ -48,7 +223,8 @@
      * @private
      */
     onPrefsChanged_: function(prefs) {
-      this.updatePrefs_(prefs, false);
+      if (CrSettingsPrefs.isInitialized)
+        this.updatePrefs_(prefs);
     },
 
     /**
@@ -57,105 +233,72 @@
      * @private
      */
     onPrefsFetched_: function(prefs) {
-      this.updatePrefs_(prefs, true);
+      this.updatePrefs_(prefs);
 
       CrSettingsPrefs.isInitialized = true;
       document.dispatchEvent(new Event(CrSettingsPrefs.INITIALIZED));
     },
 
-
     /**
-     * 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) {
+    updatePrefs_: function(prefs) {
       prefs.forEach(function(prefObj) {
-        let root = this.prefStore;
-        let tokens = prefObj.key.split('.');
+        var cachedPref = this.getCachedPref_(prefObj);
 
-        assert(tokens.length > 0);
+        // Cache a copy of the value from settingsPrivate.
+        var changed = cachedPref.update(prefObj);
 
-        for (let i = 0; i < tokens.length; i++) {
-          let token = tokens[i];
+        // Check if the pref exists already in the Polymer pref object.
+        var pref = /** @type {(!PrefObject|undefined)} */(
+            this.get(prefObj.key, this.prefStore));
+        if (pref) {
+          if (cachedPref.equals(prefObj))
+            return;
 
-          if (!root.hasOwnProperty(token)) {
-            let path = 'prefStore.' + tokens.slice(0, i + 1).join('.');
-            this.set(path, {});
-          }
-          root = root[token];
-        }
+          cachedPref.observeValue(prefObj);
+          this.set('prefStore.' + pref.key + '.value', prefObj.value);
+        } else {
+          let node = this.prefStore;
+          let tokens = prefObj.key.split('.').slice(0, -1);
 
-        // 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]);
-        }
+          // 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);
 
-        if (shouldObserve) {
-          Object.observe(root, this.propertyChangeCallback_, ['update']);
+          // Register the observer before this.set(); setting the pref in the
+          // prefStore could trigger a change in prefObj as a side effect.
+          cachedPref.observe(prefObj);
+          this.set('prefStore.' + prefObj.key, prefObj);
         }
       }, 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.
+     * @param {!PrefObject} prefObj
      */
-    shouldUpdateListPrefValue_: function(root, newValue) {
-      if (root.value == null ||
-          root.value.length != newValue.length) {
-        return true;
-      }
-
-      for (let i = 0; i < newValue.length; i++) {
-        if (root.value != null && root.value[i] != newValue[i]) {
-          return true;
-        }
-      }
+    getCachedPref_: function(prefObj) {
+      var cachedPref = this.prefs_[prefObj.key];
+      if (cachedPref)
+        return cachedPref;
 
-      return false;
-    },
+      // Create a cached pref for the PrefObject.
+      if (prefObj.type == chrome.settingsPrivate.PrefType.LIST)
+        cachedPref = new ListPref(prefObj);
+      cachedPref = new Pref(prefObj);
 
-    /**
-     * 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
-     */
-    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() {});
-      });
+      // Set the cached pref.
+      this.prefs_[prefObj.key] = cachedPref;
+      return cachedPref;
     },
+
   });
 })();