Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791813002:
Add documentation for networking.onc API (Closed)
DescriptionAdd documentation for networking.onc API
Adds networking.onc IDL, which is copy of networkingPrivate
IDL with updated comments, and with properties/methods that are not
going to be publicly exposed nodoced.
The IDL is *not* yet used for API code generation, as networking.onc
is only an alias for networkingPrivate, but that is expected to change
(networkingPrivate will become an alias for networking.onc) in near
future. Still, docs server expects networking.idl IDL to be present
in order to generate API documentation.
This is only the first run over the IDL - I plan to update it over
the following week or two.
BUG=711336
Review-Url: https://codereview.chromium.org/2791813002
Cr-Commit-Position: refs/heads/master@{#464576}
Committed: https://chromium.googlesource.com/chromium/src/+/d85987aa5fdd5785787516c6b30c3e98844cab3e
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : networking_onc.idl same as networking_private.idl #Patch Set 8 : . #Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : . #
Total comments: 5
Patch Set 12 : . #Patch Set 13 : . #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== Add documentation for networking.onc API This add docs for networking.onc API. This adds networking.onc IDL, which is copy of networkingPrivate IDL with updated comments, and with properties/methods that are not going to be publicly exposed nodoced. The IDL is *not* yet used for API code generation, as networking.onc is only an alias for networkingPrivate, but that is expected to change (networkingPrivate will become an alias for networking.onc) in near future. Still, docs server expects networking.idl IDL to be present in order to generate API documentation. This is only the first run over the IDL - I plan to update it over the following week or two. BUG= ========== to ========== Add documentation for networking.onc API Adds networking.onc IDL, which is copy of networkingPrivate IDL with updated comments, and with properties/methods that are not going to be publicly exposed nodoced. The IDL is *not* yet used for API code generation, as networking.onc is only an alias for networkingPrivate, but that is expected to change (networkingPrivate will become an alias for networking.onc) in near future. Still, docs server expects networking.idl IDL to be present in order to generate API documentation. This is only the first run over the IDL - I plan to update it over the following week or two. BUG=672186 ==========
The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tbarzic@chromium.org changed reviewers: + rdevlin.cronin@chromium.org, stevenjb@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It is unfortunate that we don't have a diff from networking_private.idl, maybe increase the threshold? Anyway, assuming it's just an updated link to the onc doc and renaming, lgtm.
On 2017/04/11 21:40:18, stevenjb wrote: > It is unfortunate that we don't have a diff from networking_private.idl, maybe > increase the threshold? > > Anyway, assuming it's just an updated link to the onc doc and renaming, lgtm. OK, Patch Set 7 is not the same as networking_private.idl, so delta from 7 should be the same as delta to networking_private.idl (I tried uploading with --similarity=10, but it didn't seem to help :/).
Heh, that was a lot of hoops, thanks for getting the delta diff though, that helps! And thanks for adding all the documentation! I just scanned through it, but lgtm++!
Two high-level thoughts: - Why do we have so many nodocs? Do we want to expose those in the API, or not? If not, we shouldn't have them in the spec; if so, we should probably document them. nodoc is typically reserved for either deprecated items or internal details, but these don't all look like internal details, and we shouldn't launch an API with deprecated properties. - This is a *huuuuge* API. Are y'all okay owning this from now until the end of time, and supporting it for public consumers? I'd personally opt for seeing if we could get away with something a bit leaner first, but it's largely your call. https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... File extensions/common/api/networking_onc.idl (right): https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... extensions/common/api/networking_onc.idl:9: // available in regular user session, but only to a white-listed set of I'd just remove the part about whitelisted apps. It's documented in the code, and we don't want to grow that list. https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... extensions/common/api/networking_onc.idl:597: // <code>'sim-puk'</code> and <code>''</code>. Why isn't this an enum? And why '' instead of undefined?
On 2017/04/12 22:13:04, Devlin wrote: > Two high-level thoughts: > - Why do we have so many nodocs? Do we want to expose those in the API, or not? > If not, we shouldn't have them in the spec; if so, we should probably document > them. nodoc is typically reserved for either deprecated items or internal > details, but these don't all look like internal details, and we shouldn't launch > an API with deprecated properties. > - This is a *huuuuge* API. Are y'all okay owning this from now until the end of > time, and supporting it for public consumers? I'd personally opt for seeing if > we could get away with something a bit leaner first, but it's largely your call. > > https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... > File extensions/common/api/networking_onc.idl (right): > > https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... > extensions/common/api/networking_onc.idl:9: // available in regular user > session, but only to a white-listed set of > I'd just remove the part about whitelisted apps. It's documented in the code, > and we don't want to grow that list. > > https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... > extensions/common/api/networking_onc.idl:597: // <code>'sim-puk'</code> and > <code>''</code>. > Why isn't this an enum? And why '' instead of undefined? I'm not entirely sure I follow the top level comment. This is a *huge* pre-existing API. We are already owning it until the end of time... We already trimmed the unnecessary stuff for networking config. The rest is there for a reason. We are now making it selectively available to kiosk app users and, incrementally, improving documentation.
https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... File extensions/common/api/networking_onc.idl (right): https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... extensions/common/api/networking_onc.idl:9: // available in regular user session, but only to a white-listed set of On 2017/04/12 22:13:03, Devlin wrote: > I'd just remove the part about whitelisted apps. It's documented in the code, > and we don't want to grow that list. sgtm https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... extensions/common/api/networking_onc.idl:597: // <code>'sim-puk'</code> and <code>''</code>. On 2017/04/12 22:13:03, Devlin wrote: > Why isn't this an enum? And why '' instead of undefined? When I converted the API to use enums, using 'sim-puk' as an enum value broke something, maybe the externs generator? So far we have avoided any ONC dictionary -> networking{Private|.onc} property translators; I would like to continue to avoid introducing such a layer.
On 2017/04/12 22:37:31, stevenjb wrote: > I'm not entirely sure I follow the top level comment. This is a *huge* > pre-existing API. We are already owning it until the end of time... > > We already trimmed the unnecessary stuff for networking config. The rest is > there for a reason. > > We are now making it selectively available to kiosk app users and, > incrementally, improving documentation. The difference is that it's currently a private API, which isn't *really* designed for arbitrary use. Our requirement to support it is relatively small, and we have a little more freedom to make breaking changes, update/remove/add methods, etc. When it's a public API, we try very hard to make sure we *don't* make breaking changes, etc. I'm just double checking that we want to be on the hook for this much of it.
On 2017/04/12 22:53:09, Devlin wrote: > On 2017/04/12 22:37:31, stevenjb wrote: > > I'm not entirely sure I follow the top level comment. This is a *huge* > > pre-existing API. We are already owning it until the end of time... > > > > We already trimmed the unnecessary stuff for networking config. The rest is > > there for a reason. > > > > We are now making it selectively available to kiosk app users and, > > incrementally, improving documentation. > > The difference is that it's currently a private API, which isn't *really* > designed for arbitrary use. Our requirement to support it is relatively small, > and we have a little more freedom to make breaking changes, update/remove/add > methods, etc. When it's a public API, we try very hard to make sure we *don't* > make breaking changes, etc. I'm just double checking that we want to be on the > hook for this much of it. Didn't we already have this discussion when we decided to open it up for kiosk mode? We already have two major internal users, Settings and OOBE/Login, so we work pretty hard not make breaking changes. There is not a much simpler useful subset that we could provide.
Yes, we are OK with owning this. Note that the nodoc parts are generally parts of the API that are private to web UI. I guess they are not strictly necessary at this moment (as the IDL is used only for doc generation), but I plan to switch networking_private to alias networking_onc (at which point networking_onc.idl would be used for code generation). I'm OK with removing these from this particular CL and adding them when networkingPrivate - networking.onc alias relationship is reversed, if you'd prefer it that way. https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... File extensions/common/api/networking_onc.idl (right): https://codereview.chromium.org/2791813002/diff/200001/extensions/common/api/... extensions/common/api/networking_onc.idl:9: // available in regular user session, but only to a white-listed set of On 2017/04/12 22:13:03, Devlin wrote: > I'd just remove the part about whitelisted apps. It's documented in the code, > and we don't want to grow that list. Done.
On 2017/04/12 23:46:59, tbarzic wrote: > Yes, we are OK with owning this. > > Note that the nodoc parts are generally parts of the API that are private to web > UI. > I guess they are not strictly necessary at this moment (as the IDL is used only > for doc generation), but I plan to switch networking_private to alias > networking_onc (at which point networking_onc.idl would be used for code > generation). I'm OK with removing these from this particular CL and adding them > when networkingPrivate - networking.onc alias relationship is reversed, if you'd > prefer it that way. That'd be great, if you don't mind!
OK, removed [nodoc] parts
Description was changed from ========== Add documentation for networking.onc API Adds networking.onc IDL, which is copy of networkingPrivate IDL with updated comments, and with properties/methods that are not going to be publicly exposed nodoced. The IDL is *not* yet used for API code generation, as networking.onc is only an alias for networkingPrivate, but that is expected to change (networkingPrivate will become an alias for networking.onc) in near future. Still, docs server expects networking.idl IDL to be present in order to generate API documentation. This is only the first run over the IDL - I plan to update it over the following week or two. BUG=672186 ========== to ========== Add documentation for networking.onc API Adds networking.onc IDL, which is copy of networkingPrivate IDL with updated comments, and with properties/methods that are not going to be publicly exposed nodoced. The IDL is *not* yet used for API code generation, as networking.onc is only an alias for networkingPrivate, but that is expected to change (networkingPrivate will become an alias for networking.onc) in near future. Still, docs server expects networking.idl IDL to be present in order to generate API documentation. This is only the first run over the IDL - I plan to update it over the following week or two. BUG=711336 ==========
Thanks! LGTM.
The CQ bit was checked by tbarzic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2791813002/#ps240001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1492121298393670,
"parent_rev": "2f30baa13c1136a5188b6ebdae22b52699d687df", "commit_rev":
"d85987aa5fdd5785787516c6b30c3e98844cab3e"}
Message was sent while issue was closed.
Description was changed from ========== Add documentation for networking.onc API Adds networking.onc IDL, which is copy of networkingPrivate IDL with updated comments, and with properties/methods that are not going to be publicly exposed nodoced. The IDL is *not* yet used for API code generation, as networking.onc is only an alias for networkingPrivate, but that is expected to change (networkingPrivate will become an alias for networking.onc) in near future. Still, docs server expects networking.idl IDL to be present in order to generate API documentation. This is only the first run over the IDL - I plan to update it over the following week or two. BUG=711336 ========== to ========== Add documentation for networking.onc API Adds networking.onc IDL, which is copy of networkingPrivate IDL with updated comments, and with properties/methods that are not going to be publicly exposed nodoced. The IDL is *not* yet used for API code generation, as networking.onc is only an alias for networkingPrivate, but that is expected to change (networkingPrivate will become an alias for networking.onc) in near future. Still, docs server expects networking.idl IDL to be present in order to generate API documentation. This is only the first run over the IDL - I plan to update it over the following week or two. BUG=711336 Review-Url: https://codereview.chromium.org/2791813002 Cr-Commit-Position: refs/heads/master@{#464576} Committed: https://chromium.googlesource.com/chromium/src/+/d85987aa5fdd5785787516c6b30c... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/d85987aa5fdd5785787516c6b30c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
