Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
260083007:
Replace chrome://network implementation with networkConfig API (Closed)
|
Created:
6 years, 7 months ago by stevenjb Modified:
6 years, 7 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReplace chrome://network implementation with networkConfig API
This CL does the following:
* Moves chrome://network resources into their own directory
* Separates 'requestNetworkInfo' into two separate methods:
** NetworkUI.getNetworkLog
** networkConfig.getNetworks
* Moves networkConfig methods into their own module
** networkConfig emulates a subset of the chrome.networkingPrivate extension API
** This module will later also be used by the internet options JS code
BUG=366365
For c/b/browser_resources.grd:
R=armansito@chromium.org, pneubeck@chromium.org, xiyuan@chromium.org
TBR=sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273007
Patch Set 1 #
Total comments: 33
Patch Set 2 : Use callback id map, add getProperties #
Total comments: 46
Patch Set 3 : Feedback #
Total comments: 6
Patch Set 4 : Rebase #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Elim ONC consts, feedback #Patch Set 8 : Fix Error handling #
Total comments: 68
Patch Set 9 : Rebase +Feedback #
Total comments: 2
Patch Set 10 : Rebase + Address feedback #Messages
Total messages: 24 (0 generated)
I don't own anything, but chrome/browser changes make sense to me w/ some questions. https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network/network_config.js:34: EAP: 'EAP.EAP' indent off https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.html:61: <td>Onc Source</td> Why? ONC is uppercase everywhere else in English, e.g. onc_spec.html. https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.js (left): https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:10: 'Name', 'Type', 'State', 'Profile', 'Connectable', Why remove Profile? https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:269: * Sends a refresh request. Update comment (and maybe function name) https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:55: const base::ListValue* value) const { Can you add a comment explaining what value is expected? https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:58: NetworkTypePattern type_pattern = onc::NetworkTypePatternFromOncType(type); What sorcery is this? https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:81: const base::ListValue* value) const { same as 55 https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/network_ui.cc (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_ui.cc:11: #include "base/json/json_string_value_serializer.h" where is this used?
https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/network_config_message_handler.h (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.h:19: // networkingPrivate methods as needed. what's the reason for having a class that emulates networkingPrivate methods? For testing?
https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.js (left): https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:10: 'Name', 'Type', 'State', 'Profile', 'Connectable', On 2014/04/29 19:51:00, Michael Giuffrida wrote: > Why remove Profile? Profile isn't applicable to visible networks, only to favorites (in the next section). https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:58: NetworkTypePattern type_pattern = onc::NetworkTypePatternFromOncType(type); On 2014/04/29 19:51:00, Michael Giuffrida wrote: > What sorcery is this? See NetworkTypePattern in shill_property_util.h https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/network_config_message_handler.h (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.h:19: // networkingPrivate methods as needed. On 2014/04/29 21:26:45, armansito wrote: > what's the reason for having a class that emulates networkingPrivate methods? > For testing? The long term goal is to move all Settings to one or more component extensions. During the long transition, we want to develop Settings UI code that can be integrated into other apps (e.g. Kiosk apps) to allow some level of network configuration UI without running Chrome. This class provides that transition layer. The idea is that the JS code can be copied, with the simple addition of 'networkConfig = chrome.networkingPrivate'. See https://docs.google.com/a/google.com/document/d/1EmDW5a-tayNoGeinBO0dmXSnabNX... for more details.
https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/network_config_message_handler.h (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.h:19: // networkingPrivate methods as needed. On 2014/04/29 21:44:03, stevenjb wrote: > On 2014/04/29 21:26:45, armansito wrote: > > what's the reason for having a class that emulates networkingPrivate methods? > > For testing? > > The long term goal is to move all Settings to one or more component extensions. > During the long transition, we want to develop Settings UI code that can be > integrated into other apps (e.g. Kiosk apps) to allow some level of network > configuration UI without running Chrome. > This class provides that transition layer. The idea is that the JS code can be > copied, with the simple addition of 'networkConfig = chrome.networkingPrivate'. > See > https://docs.google.com/a/google.com/document/d/1EmDW5a-tayNoGeinBO0dmXSnabNX... > for more details. I see. So the idea is for the two hooks (the networkingPrivate API implementation and network_config.js) to use the same backend (i.e. NetworkHandler)? I'm not super familiar with the networkingPrivate implementation but it seems to me that the two APIs can pretty much share their entire C++ back end. Sorry if this doesn't make sense, I'm just trying to make sense of it.
lgtm
I reviewed the ONC changes only, so far. https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... File components/onc/docs/onc_spec.html (right): https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:359: <span class="type">ConnectionState</span> ConnectionState -> string ? https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:361: The current connection state for this network, provided by the system. This and the other comments in this file could be done later; maybe create a bug for them. This one should contain the list of possible states and their meaning. (see onc_translator_shill_to_onc.cc:300) https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:368: <span class="type">ErrorState</span> -> string ? https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:370: The current system dependent error state for this network. Either this is english-humanreadable or we should also list the possible error states. No error = this field not present ? https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:580: <span class="type">SignalStrength</span> -> integer ? https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:582: The current signal strength for this network, provided by the system. Should mention the range and the value if the network is not visible. E.g. "Must be a number in the range [0, 100]. If the network is not visible this field will be set to '0' or not present. https://codereview.chromium.org/260083007/diff/1/components/onc/onc_constants.h File components/onc/onc_constants.h (right): https://codereview.chromium.org/260083007/diff/1/components/onc/onc_constants... components/onc/onc_constants.h:70: ONC_EXPORT extern const char kErrorState[]; missing an entry in onc_signature.cc (just search for ConnectionState)
We're now missing Favorites, but I added GetProperties so that we can examine the complete set of properties for networks, which has been very useful while adding IPConfigs. I'll probably wait until favorites are back in before committing this, but that should be a very small addition at this point. No rush, but ptal when you have a moment. https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network/network_config.js:34: EAP: 'EAP.EAP' On 2014/04/29 19:51:00, Michael Giuffrida wrote: > indent off Done. https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.html (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.html:61: <td>Onc Source</td> On 2014/04/29 19:51:00, Michael Giuffrida wrote: > Why? ONC is uppercase everywhere else in English, e.g. onc_spec.html. Done. https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/network_ui/network_ui.js:269: * Sends a refresh request. On 2014/04/29 19:51:00, Michael Giuffrida wrote: > Update comment (and maybe function name) Done. https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:55: const base::ListValue* value) const { On 2014/04/29 19:51:00, Michael Giuffrida wrote: > Can you add a comment explaining what value is expected? Added to header. https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:81: const base::ListValue* value) const { On 2014/04/29 19:51:00, Michael Giuffrida wrote: > same as 55 Done. https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/network_ui.cc (right): https://codereview.chromium.org/260083007/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/network_ui.cc:11: #include "base/json/json_string_value_serializer.h" On 2014/04/29 19:51:00, Michael Giuffrida wrote: > where is this used? Done. https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... File components/onc/docs/onc_spec.html (right): https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:359: <span class="type">ConnectionState</span> On 2014/04/30 08:44:02, pneubeck wrote: > ConnectionState -> string ? Done. https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:361: The current connection state for this network, provided by the system. On 2014/04/30 08:44:02, pneubeck wrote: > This and the other comments in this file could be done later; maybe create a bug > for them. > > This one should contain the list of possible states and their meaning. > (see onc_translator_shill_to_onc.cc:300) Done. https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:368: <span class="type">ErrorState</span> On 2014/04/30 08:44:02, pneubeck wrote: > -> string ? Done. https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:370: The current system dependent error state for this network. On 2014/04/30 08:44:02, pneubeck wrote: > Either this is english-humanreadable or we should also list the possible error > states. > > No error = this field not present ? Done. https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:580: <span class="type">SignalStrength</span> On 2014/04/30 08:44:02, pneubeck wrote: > -> integer ? Done. https://codereview.chromium.org/260083007/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:582: The current signal strength for this network, provided by the system. On 2014/04/30 08:44:02, pneubeck wrote: > Should mention the range and the value if the network is not visible. > > E.g. "Must be a number in the range [0, 100]. If the network is not visible this > field will be set to '0' or not present. Done. https://codereview.chromium.org/260083007/diff/1/components/onc/onc_constants.h File components/onc/onc_constants.h (right): https://codereview.chromium.org/260083007/diff/1/components/onc/onc_constants... components/onc/onc_constants.h:70: ONC_EXPORT extern const char kErrorState[]; On 2014/04/30 08:44:02, pneubeck wrote: > missing an entry in onc_signature.cc (just search for ConnectionState) Done.
https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:23: Service: { The term 'Service' doesn't appear in ONC (so far). Maybe s/Service/NetworkConfiguration/ https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:26: NetworkTechnology: 'Cellular.NetworkTechnology', optional: since an access by path of a property dictionary will require splitting at the '.', you might also consider using an array of keys instead. I.e.: Service: { ... Cellular: { NetworkTechnology: ['Cellular', 'NetworkTechnology'] } } https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:39: OncSource: 'onc_source', add a comment: 'temporary' in the future we should something like 'Policies' being a subset of {DevicePolicy, UserPolicy} https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:69: getCallback: function(id) { get -> pop/take/remove https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:78: * @param {string} data A list of arguments passed to the callback optional: mention, first entry of list is callbackID https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:80: chromeCallback: function(data) { unify the naming of chromeCallback vs. chromeError. Maybe onSuccess/onError? https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:84: callback(data); callback.apply(null, data) https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:86: console.log('Callback not found for id: ' + callbackId); log -> error https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:92: * @param {string} data A list of arguments passed to the callback optional: mention, first entry of list is callbackID https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:99: console.log('Callback error: '' + error + '' for id: ' + callbackId); log -> error In case of an error, ext. APIs usually set chrome.runtime.lastError and call the callback. See chrome/renderer/resources/extensions/last_error.js https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:12: networkConfig.Service.ServicePath, optional: var Service = networkConfig.Service ... Service.ServicePath ... https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:153: var guid = state[networkConfig.Service.GUID]; state.GUID https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:155: guid = state[networkConfig.Service.ServicePath]; state.ServicePath https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:161: var dot = field.indexOf('.'); this doesn't work for arbitrarily nested fields. Please move the part 161 - 170 to a helper function 'getPropertyValue' (or getONCProperty...) I have the impression, that the network API should even return not only dictionaries, but objects which have this 'getValue(selector)' as a method and the selectors should be members of the API like networkConfig.Service. The advantage will be, that we can have the following semantics: state.getValue(NetworkConfiguration.WiFi.Security) === state.getValue(NetworkConfiguration.WiFi).getValue(NetworkConfiguration.WiFi.Security) === state.WiFi.getValue(NetworkConfiguration.WiFi.Security) === state.WiFi.Security I.e., we can reuse global selectors for local accesses. The way you implement is, is adding a (private) member 'selector' to each dictionary that contains its own selector (path from root to this dictionary), then: getValue(selector) { if (!this.selector isPrefixOf selector) throw exception remove this.selector.length keys from selector var cur = this for ( index i over selector ) cur = cur[selector[i]] return cur } https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:173: value = state[field[j]]; should use the helper function https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:197: for (var s in states) arrays should be traversed by index or with the forEach method e.g. http://stackoverflow.com/questions/500504/why-is-using-for-in-with-array-iter... https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:217: createStateTable('network-state-table', NETWORK_STATE_FIELDS, data[0]); data[0] -> data (note that onFavoriteNetworksReceived was different) https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:267: networkConfig.getProperties(guid, function(arg_list) { function(arg_list) -> function(state) https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:268: var state = arg_list[0]; remove https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:26: void CopyStringFromDictionary(const std::string& key, unused https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:66: NetworkTypePattern type_pattern = onc::NetworkTypePatternFromOncType(type); re Arman's question whether this could be shared with NetworkingPrivateAPI: Could you move the inner implementation chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:280-306 to a static function and share it? https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/network_ui.cc (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/network_ui.cc:45: 0)); comment what '0' means
https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:23: Service: { On 2014/05/02 09:43:46, pneubeck wrote: > The term 'Service' doesn't appear in ONC (so far). > Maybe s/Service/NetworkConfiguration/ Or 'ONC'? https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:26: NetworkTechnology: 'Cellular.NetworkTechnology', On 2014/05/02 09:43:46, pneubeck wrote: > optional: > > since an access by path of a property dictionary will require splitting at the > '.', you might also consider using an array of keys instead. > I.e.: > > Service: { > ... > Cellular: { > NetworkTechnology: ['Cellular', 'NetworkTechnology'] > } > } I think the implementation of that would be more complicated than we need. This is a convenient shorthand for our 2-level dictionary. I did move the code to access the sub-dictionaries to this object. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:39: OncSource: 'onc_source', On 2014/05/02 09:43:46, pneubeck wrote: > add a comment: 'temporary' > > in the future we should something like 'Policies' being a subset of > {DevicePolicy, UserPolicy} Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:69: getCallback: function(id) { On 2014/05/02 09:43:46, pneubeck wrote: > get -> pop/take/remove remember/get -> store/retrieve https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:78: * @param {string} data A list of arguments passed to the callback On 2014/05/02 09:43:46, pneubeck wrote: > optional: mention, first entry of list is callbackID Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:80: chromeCallback: function(data) { On 2014/05/02 09:43:46, pneubeck wrote: > unify the naming of chromeCallback vs. chromeError. > Maybe onSuccess/onError? chromeCallbackSuccess / chromeCallbackError https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:84: callback(data); On 2014/05/02 09:43:46, pneubeck wrote: > callback.apply(null, data) Interesting. I'll need to change the callbacks to take individual args instead of an array, but that does seem better. Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:86: console.log('Callback not found for id: ' + callbackId); On 2014/05/02 09:43:46, pneubeck wrote: > log -> error Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:92: * @param {string} data A list of arguments passed to the callback On 2014/05/02 09:43:46, pneubeck wrote: > optional: mention, first entry of list is callbackID Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:99: console.log('Callback error: '' + error + '' for id: ' + callbackId); On 2014/05/02 09:43:46, pneubeck wrote: > log -> error > > In case of an error, ext. APIs usually set chrome.runtime.lastError and call the > callback. See chrome/renderer/resources/extensions/last_error.js Hmm, I'd assumed that was an 'Extensions' mechanism. I'm not sure I really like using a global here, it seems too easy for multiple errors to stomp over each other. Do you know of any examples using this mechanism outside of extensions? It's not clear to me looking at last_error.js how one would ensure that it is set on error or cleared on success outside of an extension. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:12: networkConfig.Service.ServicePath, On 2014/05/02 09:43:46, pneubeck wrote: > optional: > > var Service = networkConfig.Service > ... Service.ServicePath ... Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:153: var guid = state[networkConfig.Service.GUID]; On 2014/05/02 09:43:46, pneubeck wrote: > state.GUID Right, so, my intention here was to generate a runtime error if I used a property that doesn't exist, but I realize now that as written it won't, since JS will happily return undefined for state[undefined]. I changed these to use a helper method which logs an error if an undefined property is passed to it. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:155: guid = state[networkConfig.Service.ServicePath]; On 2014/05/02 09:43:46, pneubeck wrote: > state.ServicePath ditto https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:161: var dot = field.indexOf('.'); On 2014/05/02 09:43:46, pneubeck wrote: > this doesn't work for arbitrarily nested fields. > > Please move the part 161 - 170 to a helper function 'getPropertyValue' (or > getONCProperty...) > > I have the impression, that the network API should even return not only > dictionaries, but objects which have this 'getValue(selector)' as a method and > the selectors should be members of the API like networkConfig.Service. > The advantage will be, that we can have the following semantics: > > state.getValue(NetworkConfiguration.WiFi.Security) > === > state.getValue(NetworkConfiguration.WiFi).getValue(NetworkConfiguration.WiFi.Security) > === > state.WiFi.getValue(NetworkConfiguration.WiFi.Security) > === > state.WiFi.Security > > I.e., we can reuse global selectors for local accesses. > The way you implement is, is adding a (private) member 'selector' to each > dictionary that contains its own selector (path from root to this dictionary), > then: > > getValue(selector) { > if (!this.selector isPrefixOf selector) > throw exception > remove this.selector.length keys from selector > var cur = this > for ( index i over selector ) > cur = cur[selector[i]] > return cur > } I added a helper to networkConfig. I don't think we need to support arbitrarily nested fields, at least not until we have any. I'm sure what you wrote would work, but it seems more complicated than strictly necessary here, at least so far. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:173: value = state[field[j]]; On 2014/05/02 09:43:46, pneubeck wrote: > should use the helper function Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:197: for (var s in states) On 2014/05/02 09:43:46, pneubeck wrote: > arrays should be traversed by index or with the forEach method > > e.g. > http://stackoverflow.com/questions/500504/why-is-using-for-in-with-array-iter... Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:217: createStateTable('network-state-table', NETWORK_STATE_FIELDS, data[0]); On 2014/05/02 09:43:46, pneubeck wrote: > data[0] -> data > (note that onFavoriteNetworksReceived was different) 'data' works with the change to using 'apply' in network_config. Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:267: networkConfig.getProperties(guid, function(arg_list) { On 2014/05/02 09:43:46, pneubeck wrote: > function(arg_list) -> function(state) ditto https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network_ui/network_ui.js:268: var state = arg_list[0]; On 2014/05/02 09:43:46, pneubeck wrote: > remove Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:26: void CopyStringFromDictionary(const std::string& key, On 2014/05/02 09:43:46, pneubeck wrote: > unused Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:66: NetworkTypePattern type_pattern = onc::NetworkTypePatternFromOncType(type); On 2014/05/02 09:43:46, pneubeck wrote: > re Arman's question whether this could be shared with NetworkingPrivateAPI: > > Could you move the inner implementation > chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:280-306 > to a static function and share it? Sure. Done. https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/network_ui.cc (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/network_ui.cc:45: 0)); On 2014/05/02 09:43:46, pneubeck wrote: > comment what '0' means Done.
https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:23: Service: { On 2014/05/02 22:07:38, stevenjb wrote: > On 2014/05/02 09:43:46, pneubeck wrote: > > The term 'Service' doesn't appear in ONC (so far). > > Maybe s/Service/NetworkConfiguration/ > Or 'ONC'? > Fine with me, but then there should at least be a comment referring to ONC's type 'NetworkConfiguration' (as mentioned in the onc_spec.html) https://codereview.chromium.org/260083007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:99: console.log('Callback error: '' + error + '' for id: ' + callbackId); On 2014/05/02 22:07:38, stevenjb wrote: > On 2014/05/02 09:43:46, pneubeck wrote: > > log -> error > > > > In case of an error, ext. APIs usually set chrome.runtime.lastError and call > the > > callback. See chrome/renderer/resources/extensions/last_error.js > > Hmm, I'd assumed that was an 'Extensions' mechanism. I'm not sure I really like > using a global here, it seems too easy for multiple errors to stomp over each > other. Do you know of any examples using this mechanism outside of extensions? > It's not clear to me looking at last_error.js how one would ensure that it is > set on error or cleared on success outside of an extension. > > The global is only for reporting an error to the caller and not for internal errors (it should be fine to do this anyways). It's the idiomatic way for extension APIs and the networking API does/will do it that way. I just wanted to mention it, so that you can make this interface similar enough that the transition to networkingAPI is unproblematic. E.g. you could use a global like networkConfig.lastError. https://codereview.chromium.org/260083007/diff/40001/chrome/browser/resources... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/40001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:101: * @param {string} data A list of arguments passed to the callback. The first nit: data -> args (similarly below) https://codereview.chromium.org/260083007/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/260083007/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private.json:116: "description": "Gets the cached read-only properties of the network with id networkGuid. This is meant to be a higher performance function than getProperties, which requires a round trip to query the networking subsystem. It returns a subset of the properties returned by getProperties (see 'proerties')", nit: proerties -> properties https://codereview.chromium.org/260083007/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private.json:130: "description": "Result of the query for network properties. Includes the following values: GUID, Type, Name, ConnectionState, ErrorState, WiFi.Security, WiFi.SignalStrength, Cellular.NetworkTechnology, Cellular.ActivationState, Cellular.RoamingState. Cellular.OutOfCredits" nit: RoamingState. -> RoamingState, ( . vs. , ) https://codereview.chromium.org/260083007/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private.json:413: "description": "Returns captive portal status for the network with networkPath.", hm. shouldn't this and the first parameter be 'networkGuid' ? cf. getState(networkGuid) https://codereview.chromium.org/260083007/diff/40001/chromeos/network/onc/onc... File chromeos/network/onc/onc_translator_shill_to_onc.cc (right): https://codereview.chromium.org/260083007/diff/40001/chromeos/network/onc/onc... chromeos/network/onc/onc_translator_shill_to_onc.cc:462: scoped_ptr<base::ListValue> TranslateShillNetworkListToONC( This depends on NetworkHandlers although my intention for network/onc/ was the opposite (the network_state include was unused). Could you move it to something directly in chromeos/network ? E.g. network_util.*
OK, a lot of changes have been pulled out to separate CL's, this should now be a reasonable size for review. PTAL when you have a chance. +xiyuan@ for web UI owner. https://codereview.chromium.org/260083007/diff/40001/chrome/browser/resources... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/40001/chrome/browser/resources... chrome/browser/resources/chromeos/network/network_config.js:101: * @param {string} data A list of arguments passed to the callback. The first On 2014/05/08 09:21:43, pneubeck wrote: > nit: data -> args > (similarly below) Done.
https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:10: // to a chrome.send call. nit: Convert this to a @fileoverview jsdoc. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:17: * @param {Object} properties The object containing the network properties nit: end with a '.' https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:19: * @return {Mixed} The value associated with the property nit: Mixed is not a proper type name. Is it more like {(undefined|string|Object)}? https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:46: var id = networkConfig.callbackId++; nit: networkConfig -> this here and other places. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:66: * @param {string} args A list of arguments passed to the callback. The first nit: string -> Array https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:82: * @param {string} args A list of arguments. The first entry must be the nit: string -> Array https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:100: * @param {string} type The type of networks to return This document needs to be updated. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:110: * @param {string} type The type of networks to return This document needs to be updated. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:116: nit: nuke empty line https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:113: * @param {dictionary} state Property values for the network or favorite. Update the doc here. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:124: button.innerHTML = '+'; Use .textContent instead of .innerHTML Here and other places. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:166: if (field == 'GUID' && field[0] != '/') Did you mean "||" rather than "&&"? https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:193: * @param {dictionary} data A JSON structure of event log entries. nit: dictionary -> Object https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:226: var toggleExpandRow = function(btn, guid) { Add doc for |guid| https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:94: if (!arg_list->GetInteger(0, &callback_id) || nit: I would just CHECK, i.e. CHECK(arg_list->GetInteger(0, &callback_id)); https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:107: base::Unretained(this), callback_id), Are we sure this class would not go away when GetProperties invokes the callback? If not, we should use a WeakPtr (e.g. via WeakPtrFactory) here.
please also update the commit message only minor stuff, lgtm. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:18: * @param {string} key The ONC key for the property nit: mention an example compound key like 'WiFi.SignalStrength' https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:34: return properties[key]; IIRC you wanted to have some error logging / throw exception if |properties| is not an object or does not contain |key|? https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:45: storeCallback: function(callback) { (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() https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:31: 'OncSource' In network_util.cc you use 'onc_source' (I commented it there too). https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:144: * @param {string} stateFields The state fields to use for the row. string -> Array https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:152: var guid = networkConfig.getValueFromProperties(state, 'GUID'); equivalent to 'networkConfig.GUID' better use the simple form or add some error handling to getValueFromProperties if the property is missing. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:166: if (field == 'GUID' && field[0] != '/') what does the "field[0] != '/'" check mean? if correct, then please document what it's for and where the '/' comes from. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:177: * @param {Array.<Object>} stateFields The list of fields for the table. Array.<Object> -> Array or if it shall be precise: Array.<(string|Array.<string>)> https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:193: * @param {dictionary} data A JSON structure of event log entries. On 2014/05/21 21:22:28, xiyuan wrote: > nit: dictionary -> Object and every other such occurrence https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:202: * @param {dictionary} data A list of network state information for each dictionary -> Array.<Object> https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:212: * @param {dictionary} data A list of network state information for each dictionary -> Array.<Object> https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:65: NOTREACHED(); at least return; but as the "rule" is to not handle DCHECKs or NOTREACHEDs, and crashing the browser is not necessary, these should be LOG(ERROR)s. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:79: NetworkTypePattern pattern = onc::NetworkTypePatternFromOncType(type); nit: move to the other filter options above https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... File chromeos/network/network_util.cc (right): https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... chromeos/network/network_util.cc:185: onc_dictionary->SetString("profile_path", (*it)->profile_path()); SetString -> SetStringWithoutPathExpansion maybe I should really start an effort to rename that damn function :-) https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... chromeos/network/network_util.cc:188: onc_dictionary->SetString("onc_source", onc_source); In network_ui.js you use "OncSource", maybe out of sync? https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... File chromeos/network/network_util.h (right): https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... chromeos/network/network_util.h:104: // additional debugging properties. Maybe drop a note that |debugging_properties=true| will be used on official builds and not e.g. only in tests. Just to ensure that nobody is every leaking critical properties through this function.
https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:10: // to a chrome.send call. On 2014/05/21 21:22:28, xiyuan wrote: > nit: Convert this to a @fileoverview jsdoc. Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:17: * @param {Object} properties The object containing the network properties On 2014/05/21 21:22:28, xiyuan wrote: > nit: end with a '.' Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:18: * @param {string} key The ONC key for the property On 2014/05/22 16:24:20, pneubeck wrote: > nit: mention an example compound key like 'WiFi.SignalStrength' Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:19: * @return {Mixed} The value associated with the property On 2014/05/21 21:22:28, xiyuan wrote: > nit: Mixed is not a proper type name. Is it more like > {(undefined|string|Object)}? I copied this from somewhere else. It could be {(undefined|string|int|bool|Object|Array)}. Is that the proper form? (Seems unnecessarily verbose). https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:34: return properties[key]; On 2014/05/22 16:24:20, pneubeck wrote: > IIRC you wanted to have some error logging / throw exception if |properties| is > not an object or does not contain |key|? 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. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:45: storeCallback: function(callback) { On 2014/05/22 16:24:20, pneubeck wrote: > (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() 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? https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:46: var id = networkConfig.callbackId++; On 2014/05/21 21:22:28, xiyuan wrote: > nit: networkConfig -> this here and other places. Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:66: * @param {string} args A list of arguments passed to the callback. The first On 2014/05/21 21:22:28, xiyuan wrote: > nit: string -> Array Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:82: * @param {string} args A list of arguments. The first entry must be the On 2014/05/21 21:22:28, xiyuan wrote: > nit: string -> Array Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:100: * @param {string} type The type of networks to return On 2014/05/21 21:22:28, xiyuan wrote: > This document needs to be updated. Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:110: * @param {string} type The type of networks to return On 2014/05/21 21:22:28, xiyuan wrote: > This document needs to be updated. Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:116: On 2014/05/21 21:22:28, xiyuan wrote: > nit: nuke empty line Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network_ui/network_ui.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:31: 'OncSource' On 2014/05/22 16:24:20, pneubeck wrote: > In network_util.cc you use 'onc_source' (I commented it there too). I went back and fort with this. I'll stick with 'onc_source' since it's a non-standard debugging property. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:113: * @param {dictionary} state Property values for the network or favorite. On 2014/05/21 21:22:28, xiyuan wrote: > Update the doc here. Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:124: button.innerHTML = '+'; On 2014/05/21 21:22:28, xiyuan wrote: > Use .textContent instead of .innerHTML > Here and other places. Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:144: * @param {string} stateFields The state fields to use for the row. On 2014/05/22 16:24:20, pneubeck wrote: > string -> Array Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:152: var guid = networkConfig.getValueFromProperties(state, 'GUID'); On 2014/05/22 16:24:20, pneubeck wrote: > equivalent to 'networkConfig.GUID' > better use the simple form or add some error handling to getValueFromProperties > if the property is missing. Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:166: if (field == 'GUID' && field[0] != '/') On 2014/05/22 16:24:20, pneubeck wrote: > what does the "field[0] != '/'" check mean? > if correct, then please document what it's for and where the '/' comes from. This is leftover from when the GUID was sometimes a service path. Fixed. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:177: * @param {Array.<Object>} stateFields The list of fields for the table. On 2014/05/22 16:24:20, pneubeck wrote: > Array.<Object> -> Array > or if it shall be precise: > Array.<(string|Array.<string>)> Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:193: * @param {dictionary} data A JSON structure of event log entries. On 2014/05/22 16:24:20, pneubeck wrote: > On 2014/05/21 21:22:28, xiyuan wrote: > > nit: dictionary -> Object > > and every other such occurrence Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:202: * @param {dictionary} data A list of network state information for each On 2014/05/22 16:24:20, pneubeck wrote: > dictionary -> Array.<Object> Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:212: * @param {dictionary} data A list of network state information for each On 2014/05/22 16:24:20, pneubeck wrote: > dictionary -> Array.<Object> Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network_ui/network_ui.js:226: var toggleExpandRow = function(btn, guid) { On 2014/05/21 21:22:28, xiyuan wrote: > Add doc for |guid| Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:65: NOTREACHED(); On 2014/05/22 16:24:20, pneubeck wrote: > at least return; but as the "rule" is to not handle DCHECKs or NOTREACHEDs, and > crashing the browser is not necessary, these should be LOG(ERROR)s. This represents a (JS) coding error, so should crash if it happens. It's simpler than assigning each result to a bool and DCHECK(success) on each. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:79: NetworkTypePattern pattern = onc::NetworkTypePatternFromOncType(type); On 2014/05/22 16:24:20, pneubeck wrote: > nit: move to the other filter options above Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:94: if (!arg_list->GetInteger(0, &callback_id) || On 2014/05/21 21:22:28, xiyuan wrote: > nit: I would just CHECK, i.e. CHECK(arg_list->GetInteger(0, &callback_id)); 1) It's non-fatal but should never happen, so DCHECK (i.e. NOTREACHED) should be sufficient. 2) I really don't like embedding necessary code inside a macro. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:107: base::Unretained(this), callback_id), On 2014/05/21 21:22:28, xiyuan wrote: > Are we sure this class would not go away when GetProperties invokes the > callback? If not, we should use a WeakPtr (e.g. via WeakPtrFactory) here. Yes, yes we should. I must have copy/pasted this. Fixed. https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... File chromeos/network/network_util.cc (right): https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... chromeos/network/network_util.cc:185: onc_dictionary->SetString("profile_path", (*it)->profile_path()); On 2014/05/22 16:24:20, pneubeck wrote: > SetString -> SetStringWithoutPathExpansion > > maybe I should really start an effort to rename that damn function :-) So, when using a literal, I've not been bothering with the verbose form, since there's obviously nothing to expand. https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... chromeos/network/network_util.cc:188: onc_dictionary->SetString("onc_source", onc_source); On 2014/05/22 16:24:20, pneubeck wrote: > In network_ui.js you use "OncSource", maybe out of sync? Done. https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... File chromeos/network/network_util.h (right): https://codereview.chromium.org/260083007/diff/140001/chromeos/network/networ... chromeos/network/network_util.h:104: // additional debugging properties. On 2014/05/22 16:24:20, pneubeck wrote: > Maybe drop a note that |debugging_properties=true| will be used on official > builds and not e.g. only in tests. > > Just to ensure that nobody is every leaking critical properties through this > function. Done.
lgtm https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:19: * @return {Mixed} The value associated with the property On 2014/05/23 21:50:59, stevenjb wrote: > On 2014/05/21 21:22:28, xiyuan wrote: > > nit: Mixed is not a proper type name. Is it more like > > {(undefined|string|Object)}? > I copied this from somewhere else. It could be > {(undefined|string|int|bool|Object|Array)}. Is that the proper form? (Seems > unnecessarily verbose). > In this case, let's use {*}. https://codereview.chromium.org/260083007/diff/160001/chrome/browser/resource... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:21: * propety, e.g. 'WiFi.Security'. nit: add two more spaces before "property".
lgtm except the NOTREACHED without return. IMO javascript in general must be assumed to be insecure, but networking is sensitive; thus we should handled/crash invalid calls. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:45: storeCallback: function(callback) { On 2014/05/23 21:50:59, stevenjb wrote: > On 2014/05/22 16:24:20, pneubeck wrote: > > (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() > 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? > 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 ... https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:65: NOTREACHED(); On 2014/05/23 21:50:59, stevenjb wrote: > On 2014/05/22 16:24:20, pneubeck wrote: > > at least return; but as the "rule" is to not handle DCHECKs or NOTREACHEDs, > and > > crashing the browser is not necessary, these should be LOG(ERROR)s. > This represents a (JS) coding error, so should crash if it happens. It's simpler > than assigning each result to a bool and DCHECK(success) on each. IMO not acceptable. NOTREACHED will just succeed on release builds, in which case this would continue with uninitialized callback_id&filter. Better to be on the safe side and return or crash in this case. what I saw in other handlers is 'NOTREACHED(); return;' (although that's not following the dev.chromiumg style) or 'CHECK'
https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:19: * @return {Mixed} The value associated with the property On 2014/05/23 22:12:19, xiyuan wrote: > On 2014/05/23 21:50:59, stevenjb wrote: > > On 2014/05/21 21:22:28, xiyuan wrote: > > > nit: Mixed is not a proper type name. Is it more like > > > {(undefined|string|Object)}? > > I copied this from somewhere else. It could be > > {(undefined|string|int|bool|Object|Array)}. Is that the proper form? (Seems > > unnecessarily verbose). > > > > In this case, let's use {*}. Done. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:45: storeCallback: function(callback) { On 2014/05/24 13:38:53, pneubeck wrote: > On 2014/05/23 21:50:59, stevenjb wrote: > > On 2014/05/22 16:24:20, pneubeck wrote: > > > (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() > > 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? > > > > 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 > ... 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. https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/260083007/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:65: NOTREACHED(); On 2014/05/24 13:38:53, pneubeck wrote: > On 2014/05/23 21:50:59, stevenjb wrote: > > On 2014/05/22 16:24:20, pneubeck wrote: > > > at least return; but as the "rule" is to not handle DCHECKs or NOTREACHEDs, > > and > > > crashing the browser is not necessary, these should be LOG(ERROR)s. > > This represents a (JS) coding error, so should crash if it happens. It's > simpler > > than assigning each result to a bool and DCHECK(success) on each. > > IMO not acceptable. > NOTREACHED will just succeed on release builds, in which case this would > continue with uninitialized callback_id&filter. > Better to be on the safe side and return or crash in this case. > > what I saw in other handlers is 'NOTREACHED(); return;' (although that's not > following the dev.chromiumg style) or 'CHECK' 'filter' should be initialized to NULL here so that we crash in Release. https://codereview.chromium.org/260083007/diff/160001/chrome/browser/resource... File chrome/browser/resources/chromeos/network/network_config.js (right): https://codereview.chromium.org/260083007/diff/160001/chrome/browser/resource... chrome/browser/resources/chromeos/network/network_config.js:21: * propety, e.g. 'WiFi.Security'. On 2014/05/23 22:12:19, xiyuan wrote: > nit: add two more spaces before "property". Done.
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/260083007/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
Message was sent while issue was closed.
Committed patchset #10 manually as r273007 (presubmit successful). |
