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>}

[Dan Beam, 2015/03/19 23:47:09]

nit: you don't really need types if it's a literal declaration (pretty sure this
is just redundant and the compiler already knows)

[Jeremy Klein, 2015/03/20 00:45:38]

Interesting. I think I'd rather be explicit for now for consistency, but it's
good to know that the compiler's type inference is at least good enough for this
:-).

    */
-  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',
+    '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.
+       *
+       * @attribute settings
+       * @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'));
+    },
+
+    /**
+     * 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)

[Dan Beam, 2015/03/19 23:47:09]

why not just do this via

  #if defined(OS_CHROMEOS)

in the C++?

[Jeremy Klein, 2015/03/20 00:45:37]

We will have something like this in the full settingsPrivate API in getAllPrefs,
but I didn't want to mess with the existing coreOptionsHandler since this stuff
is temporary until settingsPrivate is done.

+        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];
+        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) {

[Dan Beam, 2015/03/19 23:47:09]

this is not guaranteed to be in order

[Jeremy Klein, 2015/03/20 00:45:38]

I thought it was if it's just for an array. If not, that's pretty weak... 

[Dan Beam, 2015/03/20 01:11:56]

it's pretty weak:
http://stackoverflow.com/questions/280713/elements-order-in-a-for-in-loop

just use for (;;) or forEach

[Jeremy Klein, 2015/03/20 01:13:18]

Nah, seems OK:

"In short: Use an array if order is important to you."
pathPieces is an array.

[Dan Beam, 2015/03/20 01:25:20]

here be dragons

[Jeremy Klein, 2015/03/20 01:25:44]

Switched in case someone messes with Array.prototype.

+          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');
+    },
+
+    /**
+     * 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) {
+          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.
+     * @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';

[Dan Beam, 2015/03/19 23:47:09]

() not needed, also did this already exist?  because you can certainly set a
double pref with an integer...

[michaelpg, 2015/03/19 23:53:58]

It looked to me like calling setIntegerPref for a preference registered as a
double would fail in debug builds:
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se...

This would happen if setNumberPref_ is called for a pref registered as type
double with a value that happens to be a round number.

[Jeremy Klein, 2015/03/20 00:45:38]

Done.
Well right now the html explicitly decides whether a pref is a double or integer
pref:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

This won't matter with settingsPrivate since we'll just have one setNumericPref
function.

[Dan Beam, 2015/03/20 01:23:53]

JS can only have/send doubles.  it's up to the C++ to set the right type,
probably based on the pref name.

ultimately i hope this distinction dies or is less subtle.

[Jeremy Klein, 2015/03/20 01:25:44]

Yep, what I meant is that with the settingsPrivate API the distinction will go
away as far as the JS is concerned for exactly the reason you mentioned. For
now, we just have to deal with this :-/

[michaelpg, 2015/03/20 02:34:27]

The good news is we don't have any double prefs set in Settings. If we add one
then this will just start hitting NOTREACHED(). (although I don't know what
happens in release builds when a pref value has the wrong type)

+      chrome.send(setFn, [propertyPath, value]);
+    },
+
+    /**
+     * @param {string} propertyPath The path before the property names.
+     * @param {number} value The new value of the pref.
+     * @private
+     */
+    setArrayPref_: function(propertyPath, value) {
+      chrome.send('setListPref', [propertyPath, JSON.stringify(value)]);

[Dan Beam, 2015/03/19 23:47:09]

why are you stringifying? that already happens when passed over IPC

[Jeremy Klein, 2015/03/20 00:45:37]

Got this from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Is it not needed there either? Removed here.

+    },
+  });
+})();