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..7f0b2684a560ec9bf663e972f70ecd3ee3512777 100644
--- a/chrome/browser/resources/settings/prefs/prefs.js
+++ b/chrome/browser/resources/settings/prefs/prefs.js
@@ -19,6 +19,12 @@
 (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.');
+  };
+
   Polymer({
     is: 'cr-settings-prefs',
 
@@ -36,6 +42,7 @@
     /** @override */
     created: function() {
       CrSettingsPrefs.isInitialized = false;
+      this.committed_ = {};

[michaelpg, 2015/08/19 02:09:53]

Feel free to suggest a better name for "our belief of the state of Chrome's
internal pref store".

[stevenjb, 2015/08/19 17:26:31]

It looks like this is a cache of the most recent pref store update received from
settingsPrivate, so maybe name it something like settingsCache_.

 
       chrome.settingsPrivate.onPrefsChanged.addListener(
           this.onPrefsChanged_.bind(this));
@@ -48,7 +55,8 @@
      * @private
      */
     onPrefsChanged_: function(prefs) {
-      this.updatePrefs_(prefs, false);
+      if (CrSettingsPrefs.isInitialized)
+        this.updatePrefs_(prefs);
     },
 
     /**
@@ -57,105 +65,167 @@
      * @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.
      * @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('.');
-
-        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;
+        // Cache a copy of the value so we know it came from settingsPrivate.
+        if (prefObj.type == chrome.settingsPrivate.PrefType.LIST)
+          this.committed_[prefObj.key] = prefObj.value.slice();
+        else
+          this.committed_[prefObj.key] = prefObj.value;

[Dan Beam, 2015/08/19 03:15:38]

make a getValue() or getLocalValue() that PrefObjects can specialize so these
can be treated the same

[stevenjb, 2015/08/19 17:26:31]

Also, this assumes knowledge that prefObj.value is never an Object, which I
guess is reasonable, but wasn't immediately clear to me. I tend to agree that a
PrefObject wrapper might be useful for this.

+
+        // Check if the pref exists already.
+        var pref = /** @type {(!PrefObject|undefined)} */(
+            this.get(prefObj.key, this.prefStore));
+        if (pref) {
+          // Check if the values are different.

[Dan Beam, 2015/08/19 03:15:38]

make a PrefObject#equals() so you can do this regardless of type

+          if (pref.type == chrome.settingsPrivate.PrefType.LIST) {
+            // Two arrays might have the "same" values, so don't just use "==".
+            if (this.arraysEqual_(pref.value, prefObj.value))
+              return;
+          } else if (pref.value == prefObj.value) {
+            return;
           }
 
-          this.set(path, prefObj[objKey]);
-        }
-
-        if (shouldObserve) {
-          Object.observe(root, this.propertyChangeCallback_, ['update']);
+          // If we're changing the value to a new array, we need to observe
+          // the array.
+          if (prefObj.value instanceof Array)
+            this.observeListPrefValue_(prefObj);

[Dan Beam, 2015/08/19 03:15:38]

make PrefObject#observe(callback)

+          this.set('prefStore.' + pref.key + '.value', prefObj.value);
+        } else {
+          let node = this.prefStore;
+          let tokens = prefObj.key.split('.').slice(0, -1);
+
+          // Crawl the pref tree, generating objects where necessary.
+          tokens.forEach(function(token) {

[Dan Beam, 2015/08/19 03:15:38]

nit: maybe massage cr.exportPath() take a |target| argument?  (an object to make
sure a property path exists upon)

[stevenjb, 2015/08/19 17:26:31]

I see that prefs.html includes cr.html, but it doesn't appear to use it. I would
slightly prefer to keep that dependency out of prefs and move it to any pages
that use it (e.g. appearance).

The reason for this is that I still hope at some point to be able to build an
app that we can run on Chrome OS in kiosk mode for setting a subset of device
level preferences (e.g. Internet and Display). There is still some
componentization required to make this possible, but I would prefer not to make
this task any more difficult.

(I do concede that since we do not have any immediate plans to do this that we
shouldn't make more work for ourselves at this point to support it, but between
the choice of modifying cr.exportPath and a bit of code duplication, in this
particular case I would lean towards code duplication).

[Dan Beam, 2015/08/19 18:54:11]

we could also make a generic merge() or mergeObjects() and call it from
cr.exportPath() on window

+            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);
+
+          // Register the observer before this.set(); setting the pref in the
+          // prefStore could trigger a change in prefObj as a side effect.
+          this.observePref_(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.
+     * Registers an observer on the PrefObject using Object.observe.
+     * @param {!PrefObject} prefObj The preference to observe.
      */
-    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;
-        }
-      }
+    observePref_: function(prefObj) {
+      Object.observe(prefObj, this.propertyChangeCallback_.bind(this),
+          ['update']);
+      // If the value is an array, observe the array itself as well.
+      if (prefObj.type == chrome.settingsPrivate.PrefType.LIST)
+        this.observeListPrefValue_(prefObj);
+    },
 
-      return false;
+    /**
+     * Observes the PrefObject's value for PrefObject type LIST.
+     * @param {!PrefObject} prefObj The preference whose value to observe.
+     */
+    observeListPrefValue_: function(prefObj) {

[Dan Beam, 2015/08/19 03:15:38]

when is this called without observePref_ being triggered?

+      assert(prefObj.type == chrome.settingsPrivate.PrefType.LIST);
+      Object.observe(prefObj.value,
+                     this.arrayChangeCallback_.bind(this, prefObj.key),
+                     ['update', 'splice']);
     },
 
     /**
      * 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/
+     * @param {!Array<!{object: PrefObject, name: string}>} changes
+     *     Array of change records.
      * @private
      */
     propertyChangeCallback_: 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 setting for now, not
-        // disabled, etc.
+        // UI should only be able to change the value of a preference for now.

[stevenjb, 2015/08/19 17:26:31]

It's not immediately clear that "value" refers to a specific property, e.g.:

// UI should only be able to change the PrefObject.value property.

(I think this is probably true "for ever" not just "for now"?)

         assert(change.name == 'value');
+        let newValue = change.object.value;
+        assert(newValue != undefined);
+
+        // 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.)
+        var prefValue = this.committed_[change.object.key];
+        if (prefValue instanceof Array) {
+          if (this.arraysEqual_(prefValue, newValue))
+            return;
+        } else if (prefValue == newValue) {
+          return;
+        }
 
-        let newValue = change.object[change.name];
-        assert(newValue !== undefined);
+        // Save the value we're going to set, so we don't respond to the
+        // change event sent by settingsPrivate once we call setPref.
 
         chrome.settingsPrivate.setPref(
-            change.object['key'],
+            change.object.key,
             newValue,
             /* pageId */ '',
-            /* callback */ function() {});
-      });
+            /* callback */ verifySetPref);
+      }, this);
+    },
+
+    /**
+     * Called when an array value of a pref changes.
+     * @param {string} key The name of the pref.
+     * @param {!Array<{object: !Array}>} changes The list of change records.
+     */
+    arrayChangeCallback_: function(key, changes) {
+      // The underlying array is the same for all changes.
+      var value = changes[0].object;
+
+      // If settingsPrivate already has this value, do nothing.
+      if (this.arraysEqual_(this.committed_[key], value))
+        return;
+      chrome.settingsPrivate.setPref(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.
+      */
+    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 (val1 instanceof Array && !this.arraysEqual_(val1, val2))
+          return false;
+        else if (val1 != val2)
+          return false;
+      }
+      return true;
     },
   });
 })();