MD-Settings: convert cr-settings-prefs to a singleton model

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

 /* Copyright 2015 The Chromium Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file. */
 
 /**
  * @fileoverview
- * '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.
+ * 'cr-settings-prefs' exposes a sinlgeton model of Chrome settings and
+ * preferences, which listens to changes to Chrome prefs whitelisted in
+ * chrome.settingsPrivate. When changing prefs in this element's 'prefs'
+ * property via the UI, the model 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 prefs="{{prefs}}"></cr-settings-prefs>
  *    <cr-settings-a11y-page prefs="{{prefs}}"></cr-settings-a11y-page>
  *
  * @group Chrome Settings Elements
  * @element cr-settings-prefs
  */
 
@@ skipping 106 matching lines @@
     properties: {
       /**
        * Object containing all preferences, for use by Polymer controls.
        */
       prefs: {
         type: Object,
         notify: true,
       },
 
       /**
+       * Shared private state.
+       * @type {!Element}
+       */
+      prefsPrivate_: {
+        type: Object,
+        value: document.createElement('cr-settings-prefs-private'),
+      },
+    },
+
+    observers: [
+      'prefsChanged_(prefs.*)',
+    ],
+
+    /** @override */
+    ready: function() {
+      this.startListening_();
+      this.prefsPrivate_.initialize();

[Dan Beam, 2015/09/21 22:25:20]

nit: reverse these if possible

[michaelpg, 2015/09/22 00:34:07]

Done.

+    },
+
+    /**
+     * Binds this.prefs to the cr-settings-prefs-private's shared prefs once
+     * preferences are initialized.
+     * @private
+     */
+    startListening_: function() {
+      CrSettingsPrefs.initialized.then(
+          // Squelch to prevent prefsChanged_ from notifying prefsPrivate_.
+          this.squelching_.bind(this, function() {
+            this.prefs = this.prefsPrivate_.prefs;
+            // Adding multiple listeners would create excessive duplicate work.
+            assert(!this.listening_);

[Dan Beam, 2015/09/21 22:25:20]

there seems to be a graceful fix for this: just unlisten() before any listen()

[michaelpg, 2015/09/22 00:34:07]

Done.

+            this.listening_ = true;
+            this.listen(
+                this.prefsPrivate_, 'prefs-changed', 'prefsPrivateChanged_');
+          }));

[Dan Beam, 2015/09/21 22:25:20]

CrSettingsPrefs.initialized.then(function() {
  this.squelching_(function() {
     // this is easier to read, IMO
  });
}.bind(this));

[michaelpg, 2015/09/22 00:34:06]

Done.

+    },
+
+    /**
+     * Stops listening for changes to cr-settings-prefs-private's shared prefs.
+     * @private
+     */
+    stopListening_: function() {
+      if (this.listening_) {

[Dan Beam, 2015/09/21 22:25:20]

is this if really necessary?

[michaelpg, 2015/09/22 00:34:07]

Done.

+        this.unlisten(
+            this.prefsPrivate_, 'prefs-changed', 'prefsPrivateChanged_');
+        this.listening_ = false;
+      }
+    },
+
+    /**
+     * Handles changes reported by prefsPrivate_.
+     * @private
+     */
+    prefsPrivateChanged_: function(e) {
+      // Squelch because we've defeated Polymer's internal dirty-checking.
+      this.squelching_(function() {
+        // Forward notification to host.
+        this.fire(e.type, e.detail, {bubbles: false});
+      });
+    },
+
+    /**
+     * Forwards changes to this.prefs to cr-settings-prefs-private.
+     * @private
+     */
+    prefsChanged_: function(info) {
+      // Squelch to avoid informing prefsPrivate_ of changes that came from it
+      // and re-processing changes made in other instances of this element.
+      if (!this.squelch_)
+        this.prefsPrivate_.fire('prefs-changed', info, {bubbles: false});
+    },
+
+    /**
+     * Sets a "squelch" switch before calling the function, so functions can
+     * return early when the switch is active.
+     * @param {!function()} fn
+     * @private
+     */
+    squelching_: function(fn) {

[Dan Beam, 2015/09/21 22:25:20]

runWhileIgnoringChanges_ IMO

[michaelpg, 2015/09/22 00:34:06]

Done.

+      this.squelch_ = true;

[Dan Beam, 2015/09/21 22:25:20]

this.ignoreChanges_ or this.ignorePrefChanges_

[michaelpg, 2015/09/22 00:34:07]

Done.

+      fn.call(this);
+      // We can unset squelch_ now because change notifications are synchronous.
+      this.squelch_ = false;

[Dan Beam, 2015/09/21 22:25:20]

instead of setting a flag, just stop listening during this time

[michaelpg, 2015/09/22 00:34:07]

I'm not sure how that would work... the interaction between events gets
complicated. I don't want to manually listen to and unlisten to prefs, so the
flag is useful in prefsChanged_. And calling unlisten() followed by listen()
would result in a new listener if we weren't listening before the squelch call.

also: each call to listen creates a new event handler, which it adds to the DOM
node as well as a private WeakMap; unlisten undoes all that. I'd want to
consider what the overhead of doing that on every change would be.

[Dan Beam, 2015/09/22 06:12:31]

any time I see "listen to events" followed by "but ignore sometimes", I think
"why not just listen for events only sometimes".  because there's currently no
public API for "isListening" in Polymer[1], I guess you're off the hook (as it'd
make the code just as bad to have a .listening_ flag, IMO)

[1]
https://github.com/Polymer/polymer/blob/22daec6f16453254a0077044077723f28d373...
prove the difference or don't worry yet, imo

+    },
+
+    /**
+     * Uninitializes this element to remove it from tests. Also resets
+     * cr-settings-prefs-private, allowing newly created elements to
+     * re-initialize it.
+     */
+    resetForTesting: function() {
+      this.stopListening_();
+      this.prefsPrivate_.resetForTesting();
+    },
+  });
+
+  /**
+   * Privately used element that contains, listens to and updates the shared
+   * prefs state.
+   */
+  Polymer({
+    is: 'cr-settings-prefs-private',
+
+    properties: {
+      /**
+       * Object containing all preferences, for use by Polymer controls.
+       * @type {(Object|undefined)}
+       */
+      prefs: {
+        type: Object,
+        notify: true,
+      },
+
+      /**
        * Map of pref keys to values representing the state of the Chrome
        * pref store as of the last update from the API.
        * @type {Object<*>}
        * @private
        */
       lastPrefValues_: {
         type: Object,
         value: function() { return {}; },
       },
     },
 
-    observers: [
-      'prefsChanged_(prefs.*)',
-    ],
+    // Listen for the manually fired prefs-changed event.
+    listeners: {
+      'prefs-changed': 'prefsChanged_',
+    },
 
     settingsApi_: chrome.settingsPrivate,
 
-    /** @override */
-    ready: function() {
+    initialize: function() {
+      // Only initialize once (or after resetForTesting() is called).
+      if (this.initialized_)
+        return;
+      this.initialized_ = true;
+
       // Set window.mockApi to pass a custom settings API, i.e. for tests.
       // TODO(michaelpg): don't use a global.
       if (window.mockApi)
         this.settingsApi_ = window.mockApi;
-      CrSettingsPrefs.isInitialized = false;
 
       this.settingsApi_.onPrefsChanged.addListener(
           this.onSettingsPrivatePrefsChanged_.bind(this));
       this.settingsApi_.getAllPrefs(
           this.onSettingsPrivatePrefsFetched_.bind(this));
     },
 
     /**
      * Polymer callback for changes to this.prefs.
-     * @param {!{path: string, value: *}} change
+     * @param {!CustomEvent} e
+     * @param {!{path: string}} change
      * @private
      */
-    prefsChanged_: function(change) {
+    prefsChanged_: function(e, change) {
       if (!CrSettingsPrefs.isInitialized)
         return;
 
       var key = this.getPrefKeyFromPath_(change.path);
       var prefStoreValue = this.lastPrefValues_[key];
 
       var prefObj = /** @type {chrome.settingsPrivate.PrefObject} */(
           this.get(key, this.prefs));
 
       // If settingsPrivate already has this value, do nothing. (Otherwise,
@@ skipping 20 matching lines @@
         this.updatePrefs_(prefs);
     },
 
     /**
      * Called when prefs are fetched from settingsPrivate.
      * @param {!Array<!chrome.settingsPrivate.PrefObject>} prefs
      * @private
      */
     onSettingsPrivatePrefsFetched_: function(prefs) {
       this.updatePrefs_(prefs);
-
-      CrSettingsPrefs.isInitialized = true;
-      document.dispatchEvent(new Event(CrSettingsPrefs.INITIALIZED));
+      // Prefs are now initialized.
+      CrSettingsPrefs.setInitialized();
     },
 
     /**
      * 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)
@@ skipping 12 matching lines @@
      * @private
      */
     updatePrefs_: function(newPrefs) {
       // Use the existing prefs object or create it.
       var prefs = this.prefs || {};
       newPrefs.forEach(function(newPrefObj) {
         // Use the PrefObject from settingsPrivate to create a copy in
         // lastPrefValues_ at the pref's key.
         this.lastPrefValues_[newPrefObj.key] = deepCopy(newPrefObj.value);
 
-        // Add the pref to |prefs|.
-        cr.exportPath(newPrefObj.key, newPrefObj, prefs);
-        // If this.prefs already exists, notify listeners of the change.
-        if (prefs == this.prefs)
-          this.notifyPath('prefs.' + newPrefObj.key, newPrefObj);
+        if (!deepEqual(this.get(newPrefObj.key, prefs), newPrefObj)) {
+          // Add the pref to |prefs|.
+          cr.exportPath(newPrefObj.key, newPrefObj, prefs);
+          // If this.prefs already exists, notify listeners of the change.
+          if (prefs == this.prefs)
+            this.notifyPath('prefs.' + newPrefObj.key, newPrefObj);
+        }
       }, this);
       if (!this.prefs)
         this.prefs = prefs;
     },
 
     /**
      * 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
      */
     getPrefKeyFromPath_: function(path) {
       // Skip the first token, which refers to the member variable (this.prefs).
       var parts = path.split('.');
       assert(parts.shift() == 'prefs');
 
       for (let i = 1; i <= parts.length; i++) {
         let key = parts.slice(0, i).join('.');
         // The lastPrefValues_ keys match the pref keys.
         if (this.lastPrefValues_.hasOwnProperty(key))
           return key;
       }
       return '';
     },
+
+    /**
+     * Resets the element so it can be re-initialized with a new prefs state.
+     */
+    resetForTesting: function() {
+      this.prefs = undefined;
+      this.lastPrefValues_ = {};
+      this.initialized_ = false;
+    },
   });
 })();