Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
847163002:
Add the IDL for chrome.platformKeys. (Closed)
|
Created:
5 years, 11 months ago by pneubeck (no reviews) Modified:
5 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, erikwright+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the IDL for chrome.platformKeys.
BUG=450167
Committed: https://crrev.com/5fdc72773907757b37f0d444fed9024fce981cbb
Cr-Commit-Position: refs/heads/master@{#313478}
Patch Set 1 : #
Total comments: 11
Patch Set 2 : Addressed comments. #Patch Set 3 : Fix dictionary comments. #Patch Set 4 : Incorporated latest feedback #Messages
Total messages: 40 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
pneubeck@chromium.org changed reviewers: + kalman@chromium.org, rsleevi@chromium.org, wad@chromium.org
pneubeck@chromium.org changed required reviewers: + kalman@chromium.org
@Benjamin, please take a look. Ryan, Will: FYI
could you add a BUG? I thought this was already implemented?
On 2015/01/14 20:02:55, kalman wrote: > could you add a BUG? Request holds. > I thought this was already implemented? Never mind, I was thinking of enterprise.platformKeys.
https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/platform_keys.idl (right): https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:7: [platforms = ("chromeos")] annotation not necessary https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:24: object privateKey; make it an object? then https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:73: optional ArrayBuffer[] clientCerts, Better if these 3 arguments are reduced to a single object, even if it involves nesting, so extension would call like: chrome.platformKeys.selectClientCertificates({ interactive: true, request: { certificateTypes: ..., ... }, clientCerts: ... }, function(matches) { ... }); https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:80: [nocompile] static object subtleCrypto(); What would it look like to use this method?
https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/platform_keys.idl (right): https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:80: [nocompile] static object subtleCrypto(); On 2015/01/14 21:42:35, kalman wrote: > What would it look like to use this method? actually, this should be a static member, but I think that isn't supported by our idl to json-schema converter? usage would look like: selectClientCertificates(..., function(certs) { subtleCrypto(). sign(signAlgorithm, certs[0].privateKey, data). then(onSignSuccess, onSignFailure); })
https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/platform_keys.idl (right): https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:7: [platforms = ("chromeos")] On 2015/01/14 21:42:35, kalman wrote: > annotation not necessary Done. https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:24: object privateKey; On 2015/01/14 21:42:35, kalman wrote: > make it an object? then Done. https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:73: optional ArrayBuffer[] clientCerts, On 2015/01/14 21:42:35, kalman wrote: > Better if these 3 arguments are reduced to a single object, even if it involves > nesting, so extension would call like: > > chrome.platformKeys.selectClientCertificates({ > interactive: true, > request: { > certificateTypes: ..., > ... > }, > clientCerts: ... > }, function(matches) { > ... > }); Done. https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:80: [nocompile] static object subtleCrypto(); On 2015/01/15 09:23:52, pneubeck wrote: > On 2015/01/14 21:42:35, kalman wrote: > > What would it look like to use this method? > > actually, this should be a static member, but I think that isn't supported by > our idl to json-schema converter? > > usage would look like: > > selectClientCertificates(..., function(certs) { > subtleCrypto(). > sign(signAlgorithm, certs[0].privateKey, data). > then(onSignSuccess, onSignFailure); > }) Is there an easy way to make it a member and not a function?
Patchset #2 (id:80001) has been deleted
On 2015/01/14 21:38:22, kalman wrote: > On 2015/01/14 20:02:55, kalman wrote: > > could you add a BUG? > > Request holds. Yes, the launch bug for the API is under way.
https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/platform_keys.idl (right): https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:80: [nocompile] static object subtleCrypto(); On 2015/01/15 14:32:06, pneubeck wrote: > On 2015/01/15 09:23:52, pneubeck wrote: > > On 2015/01/14 21:42:35, kalman wrote: > > > What would it look like to use this method? > > > > actually, this should be a static member, but I think that isn't supported by > > our idl to json-schema converter? > > > > usage would look like: > > > > selectClientCertificates(..., function(certs) { > > subtleCrypto(). > > sign(signAlgorithm, certs[0].privateKey, data). > > then(onSignSuccess, onSignFailure); > > }) > > Is there an easy way to make it a member and not a function? It looks to me like you're wanting a global subtleCrypto() function there? like window.subtleCrypto()? To answer what might be your question, for doing anything more complex than just generating chrome.namespace.myFunction you need to wire it up through a custom binding.
https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/platform_keys.idl (right): https://codereview.chromium.org/847163002/diff/60001/chrome/common/extensions... chrome/common/extensions/api/platform_keys.idl:80: [nocompile] static object subtleCrypto(); On 2015/01/15 21:48:56, kalman wrote: > On 2015/01/15 14:32:06, pneubeck wrote: > > On 2015/01/15 09:23:52, pneubeck wrote: > > > On 2015/01/14 21:42:35, kalman wrote: > > > > What would it look like to use this method? > > > > > > actually, this should be a static member, but I think that isn't supported > by > > > our idl to json-schema converter? > > > > > > usage would look like: > > > > > > selectClientCertificates(..., function(certs) { > > > subtleCrypto(). > > > sign(signAlgorithm, certs[0].privateKey, data). > > > then(onSignSuccess, onSignFailure); > > > }) > > > > Is there an easy way to make it a member and not a function? > > It looks to me like you're wanting a global subtleCrypto() function there? like > window.subtleCrypto()? Yes, I shouldn't have shortcut in my example though: chrome.platformKeys.selectClientCertificates(..., function(certs) { chrome.platformKeys.subtleCrypto(). sign(signAlgorithm, certs[0].privateKey, data). then(onSignSuccess, onSignFailure); }) subtleCrypto would not have to be a function, i.e. ... chrome.platformKeys.subtleCrypto.sign(...) would be a tiny bit easier to use, but IMO not worth overhead in the codebase. the registerHooks in custom bindings seem also to expect functions and not object members. The current definition of subtleCrypto seems to be alright then? > > To answer what might be your question, for doing anything more complex than just > generating chrome.namespace.myFunction you need to wire it up through a custom > binding.
bug added
On 2015/01/20 09:35:48, pneubeck wrote: > bug added This is how you make a property: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... pretty simple. You can see how it's declared in the JSON: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... just as a property.
On 2015/01/20 18:48:10, kalman wrote: > On 2015/01/20 09:35:48, pneubeck wrote: > > bug added > > This is how you make a property: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... > pretty simple. You can see how it's declared in the JSON: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > just as a property. whether the idl -> json-schema converter supports that?
On 2015/01/20 20:15:52, pneubeck wrote: > On 2015/01/20 18:48:10, kalman wrote: > > On 2015/01/20 09:35:48, pneubeck wrote: > > > bug added > > > > This is how you make a property: > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... > > pretty simple. You can see how it's declared in the JSON: > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > just as a property. > > whether the idl -> json-schema converter supports that? Hm, looks like it doesn't, however it may be as easy as adding a 'Properties' namespace here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_... if that's the interface you want, then it'd be worth doing.
On 2015/01/20 20:29:26, kalman wrote: > On 2015/01/20 20:15:52, pneubeck wrote: > > On 2015/01/20 18:48:10, kalman wrote: > > > On 2015/01/20 09:35:48, pneubeck wrote: > > > > bug added > > > > > > This is how you make a property: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... > > > pretty simple. You can see how it's declared in the JSON: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > > just as a property. > > > > whether the idl -> json-schema converter supports that? > > Hm, looks like it doesn't, however it may be as easy as adding a 'Properties' > namespace here: > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_... > > if that's the interface you want, then it'd be worth doing. okay, I'll look into that.
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
pneubeck@chromium.org changed reviewers: + ericroman@google.com
+Eric, FYI @Benjamin, ptal
LGTM. I suppose you went with the subtleCrypto() function instead of a property to save that yak shave?
On 2015/01/27 16:29:10, kalman wrote: > LGTM. I suppose you went with the subtleCrypto() function instead of a property > to save that yak shave? yes. That's what we agreed on on the other CL, if I understood correctly.
Though could you remind me why this is in chrome/common/extensions/api rather than extensions/common/api?
On 2015/01/27 16:33:02, kalman wrote: > Though could you remind me why this is in chrome/common/extensions/api rather > than extensions/common/api? I don't know what the rule for that would be. The API definition itself is not really platform specific (if that is the criterion).
On 2015/01/27 16:30:17, pneubeck wrote: > On 2015/01/27 16:29:10, kalman wrote: > > LGTM. I suppose you went with the subtleCrypto() function instead of a > property > > to save that yak shave? > > yes. That's what we agreed on on the other CL, if I understood correctly. Oh, right. What I thought you proposed was to interpret single-argument functions in an interface called "Properties" as properties (i.e. think of them as getters in the JS sense), rather than as functions, as a workaround for IDL lacking that description. It would have required a change to the IDL compiler. If you're happy at an API level for this to be subtleCrypto()... rather than subtleCrypto... though, what you have here is obviously less (i.e. zero) churn.
On 2015/01/27 16:37:03, pneubeck wrote: > On 2015/01/27 16:33:02, kalman wrote: > > Though could you remind me why this is in chrome/common/extensions/api rather > > than extensions/common/api? > > I don't know what the rule for that would be. > The API definition itself is not really platform specific (if that is the > criterion). also not Chrome specific
On 2015/01/27 16:37:03, pneubeck wrote: > On 2015/01/27 16:33:02, kalman wrote: > > Though could you remind me why this is in chrome/common/extensions/api rather > > than extensions/common/api? > > I don't know what the rule for that would be. > The API definition itself is not really platform specific (if that is the > criterion). Mostly just that we try to put new APIs in /extensions/common/api rather than /chrome - it's not always possible if the API heavily relies on Chrome's idioms (like the tabs API). Given basically the whole of the platform keys functionality lives in /chrome it seems as though you have little choice, unless you add Delegate interface all over the place - not something I would suggest. Ok.
On 2015/01/27 16:37:08, kalman wrote: > On 2015/01/27 16:30:17, pneubeck wrote: > > On 2015/01/27 16:29:10, kalman wrote: > > > LGTM. I suppose you went with the subtleCrypto() function instead of a > > property > > > to save that yak shave? > > > > yes. That's what we agreed on on the other CL, if I understood correctly. > > Oh, right. What I thought you proposed was to interpret single-argument > functions in an interface called "Properties" as properties (i.e. think of them > as getters in the JS sense), rather than as functions, as a workaround for IDL > lacking that description. It would have required a change to the IDL compiler. Ah, no. I didn't think of that. > > If you're happy at an API level for this to be subtleCrypto()... rather than > subtleCrypto... though, what you have here is obviously less (i.e. zero) churn. Considering the implications of the compiler change, I'd prefer (not strongly) keeping it as is.
On 2015/01/27 16:44:58, pneubeck wrote: > On 2015/01/27 16:37:08, kalman wrote: > > On 2015/01/27 16:30:17, pneubeck wrote: > > > On 2015/01/27 16:29:10, kalman wrote: > > > > LGTM. I suppose you went with the subtleCrypto() function instead of a > > > property > > > > to save that yak shave? > > > > > > yes. That's what we agreed on on the other CL, if I understood correctly. > > > > Oh, right. What I thought you proposed was to interpret single-argument > > functions in an interface called "Properties" as properties (i.e. think of > them > > as getters in the JS sense), rather than as functions, as a workaround for IDL > > lacking that description. It would have required a change to the IDL compiler. > > Ah, no. I didn't think of that. > > > > > If you're happy at an API level for this to be subtleCrypto()... rather than > > subtleCrypto... though, what you have here is obviously less (i.e. zero) > churn. > > Considering the implications of the compiler change, I'd prefer (not strongly) > keeping it as is. Totally up to you, and what API you want to expose to developers. If it's advantageous for developers to have subtleCrypto as a property - make the schema change (and it wouldn't be too bad at all). If not, I wouldn't bother.
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/847163002/220001
On 2015/01/27 16:51:11, kalman wrote: > On 2015/01/27 16:44:58, pneubeck wrote: > > On 2015/01/27 16:37:08, kalman wrote: > > > On 2015/01/27 16:30:17, pneubeck wrote: > > > > On 2015/01/27 16:29:10, kalman wrote: > > > > > LGTM. I suppose you went with the subtleCrypto() function instead of a > > > > property > > > > > to save that yak shave? > > > > > > > > yes. That's what we agreed on on the other CL, if I understood correctly. > > > > > > Oh, right. What I thought you proposed was to interpret single-argument > > > functions in an interface called "Properties" as properties (i.e. think of > > them > > > as getters in the JS sense), rather than as functions, as a workaround for > IDL > > > lacking that description. It would have required a change to the IDL > compiler. > > > > Ah, no. I didn't think of that. > > > > > > > > If you're happy at an API level for this to be subtleCrypto()... rather than > > > subtleCrypto... though, what you have here is obviously less (i.e. zero) > > churn. > > > > Considering the implications of the compiler change, I'd prefer (not strongly) > > keeping it as is. > > Totally up to you, and what API you want to expose to developers. If it's > advantageous for developers to have subtleCrypto as a property - make the schema > change (and it wouldn't be too bad at all). If not, I wouldn't bother. going with the current version, static function.
Message was sent while issue was closed.
Committed patchset #4 (id:220001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5fdc72773907757b37f0d444fed9024fce981cbb Cr-Commit-Position: refs/heads/master@{#313478} |
