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

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 ddb38be8ee478326abb67fec7a1cc1ffb9c149f1..28cf704be18385f5f001f7078004e59cf0b0ba36 100644
--- a/chrome/browser/resources/settings/prefs/prefs.js
+++ b/chrome/browser/resources/settings/prefs/prefs.js
@@ -4,12 +4,12 @@
 
 /**
  * @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:
  *
@@ -136,6 +136,120 @@
       },
 
       /**
+       * 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<*>}
@@ -147,19 +261,23 @@
       },
     },
 
-    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));
@@ -169,10 +287,11 @@
 
     /**
      * 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;
 
@@ -213,9 +332,8 @@
      */
     onSettingsPrivatePrefsFetched_: function(prefs) {
       this.updatePrefs_(prefs);
-
-      CrSettingsPrefs.isInitialized = true;
-      document.dispatchEvent(new Event(CrSettingsPrefs.INITIALIZED));
+      // Prefs are now initialized.
+      CrSettingsPrefs.setInitialized();
     },
 
     /**
@@ -248,11 +366,13 @@
         // 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;
@@ -280,5 +400,14 @@
       }
       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;
+    },
   });
 })();