Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1038873004:
Convert networkingPrivate to use IDL format (Closed)
|
Created:
5 years, 8 months ago by stevenjb Modified:
5 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@issue_470282_onc_cellular_technology Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert networkingPrivate to use IDL format
This includes a minor hack to use properties.additional_properties
directly in SetProperties and CreateConfiguraiton until we can add
proper types.
BUG=470262
Committed: https://crrev.com/de7c5c49be6c434a6c60ac20a2563cffaeb6b73b
Cr-Commit-Position: refs/heads/master@{#322636}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Feedback #
Total comments: 46
Patch Set 3 : Address feedback #
Messages
Total messages: 11 (3 generated)
stevenjb@chromium.org changed reviewers: + asargent@chromium.org, michaelpg@chromium.org, pneubeck@chromium.org
I isolated just the .json -> .idl conversion from https://codereview.chromium.org/1030963003/ to simplify that review, PTAL.
lgtm w/ a few small nits https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... File extensions/common/api/networking_private.idl (right): https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:10: // use it as the ONC specification. Is there a pointer to the ONC documentation you can provide here, to help point folks in the right direction until you finish this TODO? (Is this the right one? http://www.chromium.org/chromium-os/chromiumos-design-docs/open-network-confi...) Also, I don't think that the ONC acronym is well-known, so it might make sense to spell it out the first time you use the term in the above comment and include the abbreviation in parentheses, e.g. "Open Networking Configuration (ONC)" https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:68: // limit. it might be worth mentioning if leaving limit undefined means no limit, or means the default of 1000 https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:81: callback GetEnabledNetwrokTypesCallback = void(NetworkType[] result); typo: "..Netwrok.." https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:141: // getState. A filter is provided to specify the type of networks returned Consider using "$(ref:networkingPrivate.getState)" instead of just "getState" here https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:144: // first. static void getNetworks(); Did you mean to leave "static void getNetworks();" in the comment here?
https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... File extensions/common/api/networking_private.idl (right): https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:10: // use it as the ONC specification. On 2015/03/26 19:57:09, Antony Sargent wrote: > Is there a pointer to the ONC documentation you can provide here, to help point > folks in the right direction until you finish this TODO? (Is this the right one? > http://www.chromium.org/chromium-os/chromiumos-design-docs/open-network-confi...) > > Also, I don't think that the ONC acronym is well-known, so it might make sense > to spell it out the first time you use the term in the above comment and include > the abbreviation in parentheses, e.g. "Open Networking Configuration (ONC)" > > Done. https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:68: // limit. On 2015/03/26 19:57:08, Antony Sargent wrote: > it might be worth mentioning if leaving limit undefined means no limit, or means > the default of 1000 Done. https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:81: callback GetEnabledNetwrokTypesCallback = void(NetworkType[] result); On 2015/03/26 19:57:08, Antony Sargent wrote: > typo: "..Netwrok.." Done. https://codereview.chromium.org/1038873004/diff/1/extensions/common/api/netwo... extensions/common/api/networking_private.idl:144: // first. static void getNetworks(); On 2015/03/26 19:57:08, Antony Sargent wrote: > Did you mean to leave "static void getNetworks();" in the comment here? > Done.
lgtm as mentioned already in the original CL, please check the generated documentation and if necessary tweak the IDL a bit more. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... File extensions/common/api/networking_private.idl (right): https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:35: // A string containing a Base64-encoded RSAPublicKey ASN.1 structure, nit: Base64 -> base64 https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:36: // representing the public key to be used by verifyAndEncryptCredentials and references in comments can be done using $(ref:verifyAndEncryptCredentials) feel free to do another pass to cleanup the generated documentation in a follow up CL. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:51: // configurations. neither the old "Only set if the device has already been setup once." nor the new comment are very clear, since it's not obvious what 'configuration' or 'setup' means in this case. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:89: // Gets all the properties of the network with id networkGuid. Includes all I'd put references to variables (that you can't refer to with $(ref:..), I think) between <code>...</code> again feel free to defer. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:92: // |callback|: Returns the properties of the network. technically it's not a return but more of a 'receives' or so. others seem to document it in the way 'Called with ... when ... is done.' https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:113: // |callback|: Returns the managed properties of the network. 'managed' is wrong https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:129: // |properties|: The ONC properties to configure the new network with. here and for setProperties, you call it ONC properties. In get*Properties it's just 'properties' and 'managed properties' I'd leave ONC out. Once the type descriptions are added, it's anyways defined. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:130: // |callback|: Returns the identifier of the created network. here or in a follow up, the use of 'identifier'/GUID/... should be made consistent, e.g. always be called 'network identifier' or 'network GUID'. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:136: // the network with GUID 'networkGuid'. This may also include any other s/'...'/<code>...</code> https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:139: // fail. other functions have parameter comments. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:151: GetNetworksCallback callback); was optional but probably didn't make sense https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:154: // filter.visble = true instead. typo: visble -> visible should be embedded in <code>...</code> https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:173: // update the list returned by getVisibleNetworks. This is only a $(ref:getVisibleNetworks) https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:175: // is updated, then the onNetworkListChanged event will be fired. $(ref:onNetworkListChanged) https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:179: // |networkGuid|: The identifier of the network to connect to. probably drop the argument comments in all trivial cases (or use them everywhere)? https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:181: optional VoidCallback callback); actually, for the start* functions, the comment at the |callback| was rather important to explain that it's called independently of the actual operation finishing. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:225: // Enables TDLS for wifi traffic with a specified peer if available. wifi -> WiFi https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:230: // the current TDLS status. 'Failed' indicates that the request failed these strings should probably be enums https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:233: // getWifiTDLSStatus). $(ref:getWifiTDLSStatus) https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:236: StringCallback callback); was optional but probably you changed it on purpose (potentially breaking consumers) https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:241: // TDLS status which can be 'Connected', 'Disabled', 'Disconnected', these strings should probably be enums https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:246: // Returns captive portal status for the network matching 'guid'. 'guid' is wrong https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:249: // |calback|: Results of the query for network captive portal status. calback -> callback the comment isn't right for a callback (it was the comment from the callback's argument)
Thanks for the great feedback. I did another pass at the preview doc and everything still looks fine. There are definitely more improvements to be made, but this seems like a definite step forward; nothing looks particularly out of whack. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... File extensions/common/api/networking_private.idl (right): https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:35: // A string containing a Base64-encoded RSAPublicKey ASN.1 structure, On 2015/03/27 18:08:02, pneubeck wrote: > nit: Base64 -> base64 Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:36: // representing the public key to be used by verifyAndEncryptCredentials and On 2015/03/27 18:08:03, pneubeck wrote: > references in comments can be done using $(ref:verifyAndEncryptCredentials) > feel free to do another pass to cleanup the generated documentation in a follow > up CL. Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:51: // configurations. On 2015/03/27 18:08:03, pneubeck wrote: > neither the old "Only set if the device has already been setup once." nor the > new comment are very clear, since it's not obvious what 'configuration' or > 'setup' means in this case. Acknowledged. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:89: // Gets all the properties of the network with id networkGuid. Includes all On 2015/03/27 18:08:03, pneubeck wrote: > I'd put references to variables (that you can't refer to with $(ref:..), I > think) between <code>...</code> > > again feel free to defer. Acknowledged. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:92: // |callback|: Returns the properties of the network. On 2015/03/27 18:08:02, pneubeck wrote: > technically it's not a return but more of a 'receives' or so. > others seem to document it in the way > 'Called with ... when ... is done.' Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:113: // |callback|: Returns the managed properties of the network. On 2015/03/27 18:08:03, pneubeck wrote: > 'managed' is wrong Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:129: // |properties|: The ONC properties to configure the new network with. On 2015/03/27 18:08:03, pneubeck wrote: > here and for setProperties, you call it ONC properties. In get*Properties it's > just 'properties' and 'managed properties' > > I'd leave ONC out. Once the type descriptions are added, it's anyways defined. Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:130: // |callback|: Returns the identifier of the created network. On 2015/03/27 18:08:02, pneubeck wrote: > here or in a follow up, the use of 'identifier'/GUID/... should be made > consistent, e.g. always be called 'network identifier' or 'network GUID'. Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:136: // the network with GUID 'networkGuid'. This may also include any other On 2015/03/27 18:08:03, pneubeck wrote: > s/'...'/<code>...</code> Acknowledged. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:139: // fail. On 2015/03/27 18:08:03, pneubeck wrote: > other functions have parameter comments. Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:151: GetNetworksCallback callback); On 2015/03/27 18:08:03, pneubeck wrote: > was optional but probably didn't make sense Correct. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:154: // filter.visble = true instead. On 2015/03/27 18:08:03, pneubeck wrote: > typo: visble -> visible > > should be embedded in <code>...</code> Done, ack https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:173: // update the list returned by getVisibleNetworks. This is only a On 2015/03/27 18:08:03, pneubeck wrote: > $(ref:getVisibleNetworks) Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:175: // is updated, then the onNetworkListChanged event will be fired. On 2015/03/27 18:08:03, pneubeck wrote: > $(ref:onNetworkListChanged) Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:179: // |networkGuid|: The identifier of the network to connect to. On 2015/03/27 18:08:03, pneubeck wrote: > probably drop the argument comments in all trivial cases (or use them > everywhere)? Using everywhere. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:181: optional VoidCallback callback); On 2015/03/27 18:08:03, pneubeck wrote: > actually, for the start* functions, the comment at the |callback| was rather > important to explain that it's called independently of the actual operation > finishing. Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:225: // Enables TDLS for wifi traffic with a specified peer if available. On 2015/03/27 18:08:03, pneubeck wrote: > wifi -> WiFi Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:230: // the current TDLS status. 'Failed' indicates that the request failed On 2015/03/27 18:08:03, pneubeck wrote: > these strings should probably be enums Acknowledged. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:233: // getWifiTDLSStatus). On 2015/03/27 18:08:03, pneubeck wrote: > $(ref:getWifiTDLSStatus) Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:236: StringCallback callback); On 2015/03/27 18:08:03, pneubeck wrote: > was optional but probably you changed it on purpose (potentially breaking > consumers) Actually, setter callbacks should be optional, good catch, thanks. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:241: // TDLS status which can be 'Connected', 'Disabled', 'Disconnected', On 2015/03/27 18:08:03, pneubeck wrote: > these strings should probably be enums Acknowledged. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:246: // Returns captive portal status for the network matching 'guid'. On 2015/03/27 18:08:03, pneubeck wrote: > 'guid' is wrong Done. https://codereview.chromium.org/1038873004/diff/20001/extensions/common/api/n... extensions/common/api/networking_private.idl:249: // |calback|: Results of the query for network captive portal status. On 2015/03/27 18:08:03, pneubeck wrote: > calback -> callback > > the comment isn't right for a callback (it was the comment from the callback's > argument) Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asargent@chromium.org, pneubeck@chromium.org Link to the patchset: https://codereview.chromium.org/1038873004/#ps40001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038873004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/de7c5c49be6c434a6c60ac20a2563cffaeb6b73b Cr-Commit-Position: refs/heads/master@{#322636} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
