Replace chrome://network implementation with networkConfig API

chrome/browser/resources/chromeos/network/network_config.js

Index: chrome/browser/resources/chromeos/network/network_config.js
diff --git a/chrome/browser/resources/chromeos/network/network_config.js b/chrome/browser/resources/chromeos/network/network_config.js
new file mode 100644
index 0000000000000000000000000000000000000000..204cff1a6f034f4141e0e18e7efa647df47adf5d
--- /dev/null
+++ b/chrome/browser/resources/chromeos/network/network_config.js
@@ -0,0 +1,117 @@
+// Copyright 2014 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.
+
+'use strict';
+
+// This object provides a similar API to chrome.networkingPrivate. It simulates
+// the extension callback model by storing callbacks in a member object and
+// invoking them when the corresponding method is called by Chrome in response
+// to a chrome.send call.

[xiyuan, 2014/05/21 21:22:28]

nit: Convert this to a @fileoverview jsdoc.

[stevenjb, 2014/05/23 21:50:59]

Done.

+
+var networkConfig = {
+  /**
+   * Return the property associated with a key (which may reference a
+   * sub-object).
+   *
+   * @param {Object} properties The object containing the network properties

[xiyuan, 2014/05/21 21:22:28]

nit: end with a '.'

[stevenjb, 2014/05/23 21:50:59]

Done.

+   * @param {string} key The ONC key for the property

[pneubeck (no reviews), 2014/05/22 16:24:20]

nit: mention an example compound key like 'WiFi.SignalStrength'

[stevenjb, 2014/05/23 21:50:59]

Done.

+   * @return {Mixed} The value associated with the property

[xiyuan, 2014/05/21 21:22:28]

nit: Mixed is not a proper type name. Is it more like
{(undefined|string|Object)}?

[stevenjb, 2014/05/23 21:50:59]

I copied this from somewhere else. It could be
{(undefined|string|int|bool|Object|Array)}. Is that the proper form? (Seems
unnecessarily verbose).

[xiyuan, 2014/05/23 22:12:19]

In this case, let's use {*}.

[stevenjb, 2014/05/27 15:58:35]

Done.

+   */
+  getValueFromProperties: function(properties, key) {
+    if (!key) {
+      console.error('Empty key');
+      return undefined;
+    }
+    var dot = key.indexOf('.');
+    if (dot > 0) {
+      var key1 = key.substring(0, dot);
+      var key2 = key.substring(dot + 1);
+      var subobject = properties[key1];
+      if (subobject)
+        return this.getValueFromProperties(subobject, key2);
+    }
+    return properties[key];

[pneubeck (no reviews), 2014/05/22 16:24:20]

IIRC you wanted to have some error logging / throw exception if |properties| is
not an object or does not contain |key|?

[stevenjb, 2014/05/23 21:50:59]

I was originally trying to enforce valid ONC names to protect against typos, but
decide that would be difficult to maintain. If we do add that we should probably
add a separate ONC module that can do validated lookups in JS.

+  },
+
+  /**
+   * Generate a unique id for 'callback' and store it for future retrieval.
+   *
+   * @param {function} callback The associated callback
+   * @return {integer} The id of the callback
+   */
+  callbackId: 1,
+  callbackMap: {},
+  storeCallback: function(callback) {

[pneubeck (no reviews), 2014/05/22 16:24:20]

(this could also be resolved in a follow up)

this and some other members should be marked private (was it @private ?) and
renamed to _* (or *_ maybe).

also expose only the public members by using a local scope like you do in
network_ui.js or
even more consistent with other js would be using cr.define()

[stevenjb, 2014/05/23 21:50:59]

Renamed _, added @private.

Not sure what you mean by 'use a local scope'. 
cr.define looks interesting, but it's not clear to me if that can be used to
create an object that tracks state?

[pneubeck (no reviews), 2014/05/24 13:38:53]

Yes, it can keep state, because the argument to cr.define is executed once and
the result is stored. Thus, the returned functions can reference whatever state
they want.

E.g. the following should work just fine:
cr.define('xyz', function() {
  var x = 0;
  function nextX() {
    return x++;
  }
  return {
    nextX: nextX
  };
});
xyz.nextX()   // -> 0
xyz.nextX()   // -> 1
...

[stevenjb, 2014/05/27 15:58:35]

I'm still not entirely clear on how this pattern works; I want to understand it
better before blindly copying it. Since you mentioned you were OK with this as a
follow up I will wait.

+    var id = networkConfig.callbackId++;

[xiyuan, 2014/05/21 21:22:28]

nit: networkConfig -> this here and other places.

[stevenjb, 2014/05/23 21:50:59]

Done.

+    networkConfig.callbackMap[id] = callback;
+    return id;
+  },
+
+  /**
+   * Retrieve the callback associated with |id| and remove it from the map.
+   *
+   * @param {integer} id The id of the callback
+   * @return {function} The associated callback
+   */
+  retrieveCallback: function(id) {
+    var callback = networkConfig.callbackMap[id];
+    delete networkConfig.callbackMap[id];
+    return callback;
+  },
+
+  /**
+   * Callback invoked by Chrome
+   *
+   * @param {string} args A list of arguments passed to the callback. The first

[xiyuan, 2014/05/21 21:22:28]

nit: string -> Array

[stevenjb, 2014/05/23 21:50:59]

Done.

+   *   entry must be the callbackId passed to chrome.send.
+   */
+  chromeCallbackSuccess: function(args) {
+    var callbackId = args.shift();
+    var callback = networkConfig.retrieveCallback(callbackId);
+    networkConfig.lastError = '';
+    if (callback)
+      callback.apply(null, args);
+    else
+      console.error('Callback not found for id: ' + callbackId);
+  },
+
+  /**
+   * Error Callback invoked by Chrome. Sets lastError and logs to the console.
+   *
+   * @param {string} args A list of arguments. The first entry must be the

[xiyuan, 2014/05/21 21:22:28]

nit: string -> Array

[stevenjb, 2014/05/23 21:50:59]

Done.

+   *   callbackId passed to chrome.send.
+   */
+  lastError: '',
+  chromeCallbackError: function(args) {
+    var callbackId = args.shift();
+    networkConfig.lastError = args.shift();
+    console.error('Callback error: "' + networkConfig.lastError);
+    // We still invoke the callback, but with null args. The callback should
+    // check networkConfig.lastError and handle that.
+    var callback = networkConfig.retrieveCallback(callbackId);
+    if (callback)
+      callback.apply(null, null);
+  },
+
+  /**
+   * Implement networkingPrivate.getProperties
+   *
+   * @param {string} type The type of networks to return

[xiyuan, 2014/05/21 21:22:28]

This document needs to be updated.

[stevenjb, 2014/05/23 21:50:59]

Done.

+   */
+  getProperties: function(guid, callback) {
+    var callbackId = networkConfig.storeCallback(callback);
+    chrome.send('networkConfig.getProperties', [callbackId, guid]);
+  },
+
+  /**
+   * Implement networkingPrivate.getNetworks
+   *
+   * @param {string} type The type of networks to return

[xiyuan, 2014/05/21 21:22:28]

This document needs to be updated.

[stevenjb, 2014/05/23 21:50:59]

Done.

+   */
+  getNetworks: function(filter, callback) {
+    var callbackId = networkConfig.storeCallback(callback);
+    chrome.send('networkConfig.getNetworks', [callbackId, filter]);
+  }
+

[xiyuan, 2014/05/21 21:22:28]

nit: nuke empty line

[stevenjb, 2014/05/23 21:50:59]

Done.

+};