Fetch and actually set prefs (using chrome.send for now).

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 a13e03bb51337ba72f621648fa9bbaba0212be2a..cdb22e87a7ea9e4652e9882a0a302daacab351d5 100644
--- a/chrome/browser/resources/settings/prefs/prefs.js
+++ b/chrome/browser/resources/settings/prefs/prefs.js
@@ -16,96 +16,204 @@
  * @group Chrome Settings Elements
  * @element cr-settings-a11y-page
  */
-Polymer('cr-settings-prefs', {
-  publish: {
-    /**
-     * Accessibility preferences state.
-     *
-     * @attribute settings
-     * @type CrSettingsPrefs.Settings
-     * @default null
-     */
-    settings: null,
-  },
-
-  /** @override */
-  created: function() {
-    'use strict';
-
-    this.settings = {};
-    this.initializeA11y_();
-    this.initializeDownloads_();
-    var observer = new ObjectObserver(this.settings);
-    observer.open(this.propertyChangeCallback_.bind(this, 'settings'));
-
-    // For all Object properties of settings, create an ObjectObserver.
-    for (let key in this.settings) {
-      if (typeof this.settings[key] == 'object') {
-        let keyObserver = new ObjectObserver(this.settings[key]);
-        keyObserver.open(
-            this.propertyChangeCallback_.bind(this, 'settings.' + key));
-      }
-    }
-  },
+(function() {
+  'use strict';
 
   /**
-   * Initializes some defaults for the a11y settings.
-   * @private
+   * A list of all pref paths used on all platforms in the UI.
+   * TODO(jlklein): This is a temporary workaround that needs to be removed
+   * once settingsPrivate is implemented with the fetchAll function. We will
+   * not need to tell the settingsPrivate API which prefs to fetch.
+   * @const {!Array<string>}
    */
-  initializeA11y_: function() {
-    this.settings.a11y = {
-      enableMenu: true,
-      largeCursorEnabled: false,
-      highContrastEnabled: false,
-      stickyKeysEnabled: false,
-      screenMagnifier: false,
-      enableTapDragging: false,
-      autoclick: false,
-      autoclickDelayMs: 200,
-      virtualKeyboard: false,
-    };
-
-    this.settings.touchpad = {
-      enableTapDragging: false,
-    };
-
-    // ChromeVox is enbaled/disabled via the 'settings.accessibility' boolean
-    // pref.
-    this.settings.accessibility = false;
-
-    // TODO(jlklein): Actually pull the data out of prefs and initialize.
-  },
+  var PREFS_TO_FECTH = [
+    'download.default_directory',
+    'download.prompt_for_download',
+  ];
 
   /**
-   * Initializes some defaults for the downloads settings.
-   * @private
+   * A list of all CrOS-only pref paths used in the UI.
+   * TODO(jlklein): This is a temporary workaround that needs to be removed
+   * once settingsPrivate is implemented with the fetchAll function. We will
+   * not need to tell the settingsPrivate API which prefs to fetch.
+   * @const {!Array<string>}
    */
-  initializeDownloads_: function() {
-    this.settings.download = {
-      downloadLocation: '',
-      promptForDownload: false,
-    };
-  },
+  var CROS_ONLY_PREFS = [
+    'settings.a11y.enable_menu',
+    'settings.a11y.large_cursor_enabled',

[michaelpg, 2015/03/19 23:39:07]

nit: alphabetize.

[Jeremy Klein, 2015/03/20 00:03:15]

Done.

+    'settings.a11y.high_contrast_enabled',
+    'settings.a11y.sticky_keys_enabled',
+    'settings.accessibility',
+    'settings.a11y.screen_magnifier',
+    'settings.touchpad.enable_tap_dragging',
+    'settings.a11y.autoclick',
+    'settings.a11y.autoclick_delay_ms',
+    'settings.a11y.virtual_keyboard',
+  ];
 
-  /**
-   * @param {string} propertyPath The path before the property names.
-   * @param {!Array<string>} added An array of keys which were added.
-   * @param {!Array<string>} removed An array of keys which were removed.
-   * @param {!Array<string>} changed An array of keys of properties whose
-   *     values changed.
-   * @param {function(string) : *} getOldValueFn A function which takes a
-   *     property name and returns the old value for that property.
-   * @private
-   */
-  propertyChangeCallback_: function(
-      propertyPath, added, removed, changed, getOldValueFn) {
-    Object.keys(changed).forEach(function(property) {
-      console.log(
-          `${propertyPath}.${property}`,
-          `old : ${getOldValueFn(property)}`,
-          `newValue : ${changed[property]}`);
-
-      // TODO(jlklein): Actually set the changed property back to prefs.
-    });
-   },
-});
+  Polymer('cr-settings-prefs', {
+    publish: {
+      /**
+       * Object containing all prefereces.

[michaelpg, 2015/03/19 23:39:07]

preferences

[Jeremy Klein, 2015/03/20 00:03:14]

Done.

+       *
+       * @attribute settings

[michaelpg, 2015/03/19 23:39:07]

existential: this element is called prefs, but everything inside it is called
settings. what are we really? why are we here?

[Jeremy Klein, 2015/03/20 00:03:15]

*sigh*... Yeah, you're right. I could rename this to prefs, but that also seems
confusing because the ui will have prefs.pref.foo.bar. data? model?

Got any other suggestions for clearing this up?

[michaelpg, 2015/03/20 00:52:54]

I just hoped we'd have better definitions because of how random the current
settings page seems. But it doesn't help that we're also having to coexist with
the existing settings.

Let's not worry about it now. If it gets really confusing we can define better
terms later.

[Jeremy Klein, 2015/03/20 00:58:35]

SGTM

+       * @type CrSettingsPrefs.Settings
+       * @default null
+       */
+      settings: null,
+    },
+
+    /** @override */
+    created: function() {
+      this.settings = {};
+      this.fetchSettings_();
+      var observer = new ObjectObserver(this.settings);
+      observer.open(this.propertyChangeCallback_.bind(this, 'settings'));

[michaelpg, 2015/03/19 23:39:07]

Why is this here? propertyChangeCallback_ says it is "Called when a property of
a pref changes".

[Jeremy Klein, 2015/03/20 00:03:14]

Yeah this is left over. Removed.

+    },
+
+    /**
+     * Fetches all settings from settingsPrivate.
+     * TODO(jlklein): Implement using settingsPrivate when it's available.
+     * @private
+     */
+    fetchSettings_: function() {
+      // *Sigh* We need to export the function name to global scope.
+      var callbackName = 'CrSettingsPrefs_onPrefsFetched';
+      window[callbackName] = this.onPrefsFetched_.bind(this);
+      var prefsToFetch = PREFS_TO_FECTH;
+      if (cr.isChromeOS)
+        prefsToFetch.concat(CROS_ONLY_PREFS);
+
+      chrome.send('fetchPrefs', [callbackName].concat(prefsToFetch));
+    },
+
+    /**
+     * Fetches all settings from settingsPrivate.
+     * @param {!Object} dict Map of fetched property values.
+     * @private
+     */
+    onPrefsFetched_: function(dict) {
+      this.parsePrefDict_('', dict);
+    },
+
+    /**
+     * Helper function for parsing the prefs dict and constructing Preference
+     * objects.
+     * @param {string} prefix The namespace prefix of the pref.
+     * @param {!Object} dict Map with preference values.
+     * @private
+     */
+    parsePrefDict_: function(prefix, dict) {
+      for (let prefName in dict) {
+        let value = dict[prefName];

[michaelpg, 2015/03/19 23:39:08]

"value" confused me because later it represents a pref, not a pref value. How
about something more generic, like "obj" or "dict"?

[Jeremy Klein, 2015/03/20 00:03:15]

Going with prefObj.

[michaelpg, 2015/03/20 00:52:54]

Cool, I think this is much clearer.

+        if (!this.isPrefObject_(value)) {
+          this.parsePrefDict_(prefix + prefName + '.', value);
+          continue;
+        }
+
+        // value is a pref object. Construct the path to pref using prefix,
+        // add the pref to this.settings, and observe changes.
+        let root = this.settings;
+        let pathPieces = prefix.slice(0, -1).split('.');
+        for (let i in pathPieces) {
+          root[pathPieces[i]] = root[pathPieces[i]] || {};
+          root = root[pathPieces[i]];
+        }
+
+        root[prefName] = value;
+        let keyObserver = new ObjectObserver(value);
+        keyObserver.open(
+            this.propertyChangeCallback_.bind(this, prefix + prefName));
+      }
+    },
+
+    /**
+     * Determines whether the passed object is a pref object from Chrome.
+     * @param {*} rawPref The object to check.
+     * @return {boolean} True if the passes object is a pref.
+     * @private
+     */
+    isPrefObject_: function(rawPref) {
+      return rawPref && typeof rawPref == 'object' &&
+          rawPref.hasOwnProperty('value') &&
+          rawPref.hasOwnProperty('disabled');

[michaelpg, 2015/03/19 23:39:08]

so this assumes we don't have settings called "foo.bar.value" and
"foo.bar.disabled", right? 

[Jeremy Klein, 2015/03/20 00:03:14]

Yeah. When the settingsPrivate stuff is finalized, we'll have more details to
look at here in the prefObject so there will be a much lower chance of false
positives.

+    },
+
+    /**
+     * Called when a property of a pref changes.
+     * @param {string} propertyPath The path before the property names.
+     * @param {!Array<string>} added An array of keys which were added.
+     * @param {!Array<string>} removed An array of keys which were removed.
+     * @param {!Array<string>} changed An array of keys of properties whose
+     *     values changed.
+     * @param {function(string) : *} getOldValueFn A function which takes a
+     *     property name and returns the old value for that property.
+     * @private
+     */
+    propertyChangeCallback_: function(
+        propertyPath, added, removed, changed, getOldValueFn) {
+      for (let property in changed) {
+        // UI should only be able to change the value of a setting for now, not
+        // disabled, etc.
+        assert(property == 'value');
+
+        let newValue = changed[property];
+        switch (typeof newValue) {

[michaelpg, 2015/03/19 23:39:07]

is a type of undefined possible?

[Jeremy Klein, 2015/03/20 00:03:14]

I don't think it's possible for a new value. Do you see any way that the UI
could set something to undefined?

[michaelpg, 2015/03/20 00:52:54]

I'm sure it will happen eventually. I'm just worried it'd be hard to debug.
Could we maybe have an assert or a console.error as a default case?

[Jeremy Klein, 2015/03/20 00:58:35]

Added assert.

+          case 'boolean':
+            this.setBooleanPref_(
+                propertyPath, /** @type {boolean} */ (newValue));
+            break;
+          case 'number':
+            this.setNumberPref_(
+                propertyPath, /** @type {number} */ (newValue));
+            break;
+          case 'string':
+            this.setStringPref_(
+                propertyPath, /** @type {string} */ (newValue));
+            break;
+          case 'object':
+            assertInstanceof(newValue, Array);
+            this.setArrayPref_(
+                propertyPath, /** @type {!Array} */ (newValue));
+        }
+      }
+    },
+
+    /**
+     * @param {string} propertyPath The path before the property names.

[michaelpg, 2015/03/19 23:39:07]

From the perspective of propertyChangeCallback_ this comment makes sense, but in
general shouldn't this be the full path of the setting? 

[Jeremy Klein, 2015/03/20 00:03:14]

Done.

+     * @param {boolean} value The new value of the pref.
+     * @private
+     */
+    setBooleanPref_: function(propertyPath, value) {
+      chrome.send('setBooleanPref', [propertyPath, value]);
+    },
+
+    /**
+     * @param {string} propertyPath The path before the property names.
+     * @param {string} value The new value of the pref.
+     * @private
+     */
+    setStringPref_: function(propertyPath, value) {
+      chrome.send('setStringPref', [propertyPath, value]);
+    },
+
+    /**
+     * @param {string} propertyPath The path before the property names.
+     * @param {number} value The new value of the pref.
+     * @private
+     */
+    setNumberPref_: function(propertyPath, value) {
+      var setFn = (value % 1 == 0) ? 'setIntegerPref' : 'setDoublePref';
+      chrome.send(setFn, [propertyPath, value]);

[michaelpg, 2015/03/19 23:39:07]

I suspect this will blow up if you try to call setIntegerPref for a preference
registered as a double. Have you tested it for that case?

[Jeremy Klein, 2015/03/20 00:03:14]

Hmm, good point. I haven't tried that. I'd rather not worry about this problem
now because settingsPrivate will just have a "setNumericPref" anyway and we
won't need to worry about it.

+stevenjb@chromium.org and +orenb@chromium.org to confirm. 

+    },
+
+    /**
+     * @param {string} propertyPath The path before the property names.
+     * @param {number} value The new value of the pref.

[michaelpg, 2015/03/19 23:39:08]

Array

[Jeremy Klein, 2015/03/20 00:03:15]

Done.

+     * @private
+     */
+    setArrayPref_: function(propertyPath, value) {
+      chrome.send('setListPref', [propertyPath, JSON.stringify(value)]);
+    },
+  });
+})();