|
|
Created:
3 years, 10 months ago by kpaulhamus Modified:
3 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, aczeskis_google.comchromium-reviews, agrieve+watch_chromium.org, Bernhard Bauer, blink-reviews, darin (slow to review), haraken, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd webauth makeCredential mojom.
Patch #1 of multiple
This patch adds the webauth authenticator mojom interface. It has the function makeCredential and necessary structs. The function is used by the WebAuthentication class.
BUG=664630
Review-Url: https://codereview.chromium.org/2702653002
Cr-Commit-Position: refs/heads/master@{#471498}
Committed: https://chromium.googlesource.com/chromium/src/+/b1f03c340eaaf193e6d4cae7047600c0eddd5ffb
Patch Set 1 #Patch Set 2 : Small syntax fix #Patch Set 3 : Updated UseCounter.h #
Total comments: 2
Patch Set 4 : Fixed Java compile errors #Patch Set 5 : Restructured to only include mojom and usage for makeCredential call #
Total comments: 28
Patch Set 6 : Addressing reviewer comments #
Total comments: 2
Patch Set 7 : Missing comma in DEPS #Patch Set 8 : Remove counters to put in a later CL - annoying to rebase, downstream CLs, etc #Patch Set 9 : rebase #Patch Set 10 : Blink renaming #Patch Set 11 : Updated based on SharedArrayBuffer changes #
Total comments: 19
Patch Set 12 : Addressing comments #Patch Set 13 : Addressing comments #
Total comments: 9
Patch Set 14 : Capitalizing mojom interface method #
Total comments: 6
Patch Set 15 : Addressing more comments #
Total comments: 13
Patch Set 16 : Addressing more comments #Patch Set 17 : working on updating layouttests #Patch Set 18 : Rebaselining layout tests #Patch Set 19 : Adding additional comments to mojom structs. #
Total comments: 8
Patch Set 20 : Rebaselining layout tests and fixes nits #
Total comments: 4
Patch Set 21 : Resolving final comments #Patch Set 22 : Change to ChromeInterfaceRegistrar.java shouldn't be here - left over from splitting of Android imp… #Patch Set 23 : Add Authenticator to interface provider to correct try job 'service_manager' error. #Patch Set 24 : IDL methods should be lower-case - was failing layouttests #Patch Set 25 : Removing obsolete idl-expected file to pass layouttests #Messages
Total messages: 86 (46 generated)
The CQ bit was checked by kpaulhamus@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by kpaulhamus@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
yzshen@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2702653002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorFactory.java (right): https://codereview.chromium.org/2702653002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorFactory.java:28: return null(); remove ()?
The CQ bit was checked by kpaulhamus@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
kpaulhamus@chromium.org changed reviewers: + bauerb@chromium.org, dcheng@chromium.org, esprehn@chromium.org
Hi, can you review the following? Elliott - simple changes in Source/modules/ Bernhard - addition to chrome/android/ Daniel - authenticator.mojom & UseCounter.h Thanks! https://codereview.chromium.org/2702653002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorFactory.java (right): https://codereview.chromium.org/2702653002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorFactory.java:28: return null(); On 2017/02/17 22:42:23, yzshen1 wrote: > remove ()? Done.
Can we break this CL up and land mojom changes incrementally as we implement them and the code that actually uses them? It will be easier to evaluate the security aspects that way.
On 2017/02/21 23:47:11, dcheng wrote: > Can we break this CL up and land mojom changes incrementally as we implement > them and the code that actually uses them? It will be easier to evaluate the > security aspects that way. Sure, I can do that. Stay tuned.
On 2017/02/22 00:25:10, kpaulhamus wrote: > On 2017/02/21 23:47:11, dcheng wrote: > > Can we break this CL up and land mojom changes incrementally as we implement > > them and the code that actually uses them? It will be easier to evaluate the > > security aspects that way. > > Sure, I can do that. Stay tuned. Daniel, does this look better? Unfortunately, using just the makeCredential call involves a lot of other structures, so it's still a little hefty. (Also, I bypassed presubmit because of this complaint: "WebAuthentication.h: Illegal include: "components/webauth/authenticator.mojom-blink.h". Any idea how to address that?) I'm also curious to know if there are better ways to write the custom converters in WebAuthentication.cpp - open to suggestions there!
Description was changed from ========== Added webauth .mojom and initial impl placeholders. BUG=664630 ========== to ========== Add webauth makeCredential mojom. This patch adds the webauth authenticator mojom interface. It has the function makeCredential and necessary structs. The function is used by the WebAuthentication class. BUG=664630 ==========
bauerb@chromium.org changed reviewers: - bauerb@chromium.org
kpaulhamus@chromium.org changed reviewers: + ikilpatrick@chromium.org
https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... components/webauth/authenticator.mojom:9: array<uint8> clientData; Nit: hacker_case instead of camelCase for variable names in mojom https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... components/webauth/authenticator.mojom:14: string rpDisplayName; Nit: 2 space indent. https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... components/webauth/authenticator.mojom:23: //TODO add AlgorithmIdentifier algorithm; Nit: TODO comment format is // TODO(username or https://crbug.com/number): Do that. https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... components/webauth/authenticator.mojom:50: makeCredential(RelyingPartyAccount accountInformation, Nit: even though this is in Blink, methods should be named with UpperCamelCase. The Blink rename is happening soon, and will fix the inconsistencies in Blink. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:51: if (credType == "ScopedCred") Unfortunately, mapping strings to enums is the best we can do atm: see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagec... for other code that has to do this. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:69: const blink::RelyingPartyAccount& accountInformation, However, this can be implemented by using something called a typemap. See https://www.chromium.org/developers/design-documents/mojo/type-mapping. A simple typemap example to look at is the Blink typemap for KURL: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/... Ditto for the conversion helpers below. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:117: WTF::bind(&WebAuthentication::onAuthenticatorConnectionError, Usually, the renderer won't ever see a connection error--if it sees one, that means the browser side broke the pipe: because the browser process crashed (in which case any further work in the renderer is meaningless), or the document is going away (in which case we'll call contextDestroyed). Is it necessary to have this here? (Also, including the service side implementation in //components/webauth will help with this review) https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:144: if (!executionContext->isSecureContext(errorMessage)) { Do you know if there any plan to implement the [SecureContext] attribute? https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:162: wrapPersistent(this), Should this be a weak persistent? If this object would otherwise be destroyed, it doesn't seem like the callback should keep this object alive. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right): https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.h:73: m_authenticatorRequests; // to use to keep track of requests. See Nit: consider moving the comment above the field declaration so it doesn't wrap so much
I just had a quick pass.. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:50: ScopedCredentialType convertScopedCredentialType(const WTF::String& credType) { is there a standard naming for these methods in the codebase? What does bluetooth do? https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:140: if (!m_authenticator) { can you un-nest these if stmts?, e.g. if (!m_authenticator) return ScriptPromise::rejectWithDOMException(...); String errorMessage; if (!executionContext->isSecureContext(...)) return return ScriptPromise::rejectWithDOMException(...); ScriptPromiseResolver* resolver = ... ...; return promise; https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:194: if (!markRequestComplete(resolver)) Should this reject promise? Or DCHECK that this never happens? https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:200: resolver->reject( just rejectWithDOMException?
https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... components/webauth/authenticator.mojom:9: array<uint8> clientData; On 2017/02/28 07:25:56, dcheng wrote: > Nit: hacker_case instead of camelCase for variable names in mojom Done. https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... components/webauth/authenticator.mojom:14: string rpDisplayName; On 2017/02/28 07:25:56, dcheng wrote: > Nit: 2 space indent. Done. https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... components/webauth/authenticator.mojom:23: //TODO add AlgorithmIdentifier algorithm; On 2017/02/28 07:25:56, dcheng wrote: > Nit: TODO comment format is > // TODO(username or https://crbug.com/number): Do that. Done. https://codereview.chromium.org/2702653002/diff/80001/components/webauth/auth... components/webauth/authenticator.mojom:50: makeCredential(RelyingPartyAccount accountInformation, On 2017/02/28 07:25:56, dcheng wrote: > Nit: even though this is in Blink, methods should be named with UpperCamelCase. > The Blink rename is happening soon, and will fix the inconsistencies in Blink. Done. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:50: ScopedCredentialType convertScopedCredentialType(const WTF::String& credType) { On 2017/02/28 19:18:18, ikilpatrick wrote: > is there a standard naming for these methods in the codebase? What does > bluetooth do? It doesn't really seem like it. Bluetooth doesn't have a completely analogous case; ImageCapture calls it "ParseX" for string-to-enum conversions, webusb uses "ConvertX", nfc says "toXType". Do you have a preference? https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:51: if (credType == "ScopedCred") On 2017/02/28 07:25:56, dcheng wrote: > Unfortunately, mapping strings to enums is the best we can do atm: see > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagec... > for other code that has to do this. Acknowledged. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:69: const blink::RelyingPartyAccount& accountInformation, On 2017/02/28 07:25:56, dcheng wrote: > However, this can be implemented by using something called a typemap. See > https://www.chromium.org/developers/design-documents/mojo/type-mapping. A simple > typemap example to look at is the Blink typemap for KURL: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/... > > Ditto for the conversion helpers below. Got it, I'll take a closer look. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:117: WTF::bind(&WebAuthentication::onAuthenticatorConnectionError, On 2017/02/28 07:25:56, dcheng wrote: > Usually, the renderer won't ever see a connection error--if it sees one, that > means the browser side broke the pipe: because the browser process crashed (in > which case any further work in the renderer is meaningless), or the document is > going away (in which case we'll call contextDestroyed). Is it necessary to have > this here? > > (Also, including the service side implementation in //components/webauth will > help with this review) I don't know enough about the pipe life cycle to make a determination, but it did look like every example I found is actively using set_connection_error_handle. (http://shortn/_HCSt7qtOy1) https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:140: if (!m_authenticator) { On 2017/02/28 19:18:18, ikilpatrick wrote: > can you un-nest these if stmts?, e.g. > > if (!m_authenticator) > return ScriptPromise::rejectWithDOMException(...); > > String errorMessage; > if (!executionContext->isSecureContext(...)) > return return ScriptPromise::rejectWithDOMException(...); > > ScriptPromiseResolver* resolver = ... > ...; > > return promise; Done. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:144: if (!executionContext->isSecureContext(errorMessage)) { On 2017/02/28 07:25:56, dcheng wrote: > Do you know if there any plan to implement the [SecureContext] attribute? It's a work in progress: https://bugs.chromium.org/p/chromium/issues/detail?id=634270 https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:162: wrapPersistent(this), On 2017/02/28 07:25:56, dcheng wrote: > Should this be a weak persistent? If this object would otherwise be destroyed, > it doesn't seem like the callback should keep this object alive. I talked with reillyg about this, just because I wasn't sure myself. It seems to be a rule of thumb that you use wrapPersistent() for both 'this' and the ScriptPromiseResolver when building a Mojo callback, and use wrapWeakPersistent() for connection error callbacks. The subtlety apparently is that promises don't retain a reference to the object that created them, so if the script doesn't keep a reference to a WebAuthentication object, then the promise will never be resolved. He did say - "The question of whether the promise should be resolved if the object you called the method on that returned it is GCed is one that makes intuitive sense to me but I have never seen anything specifying the behavior." https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:194: if (!markRequestComplete(resolver)) On 2017/02/28 19:18:18, ikilpatrick wrote: > Should this reject promise? Or DCHECK that this never happens? markRequestCompleted has the additional action of removing the request from tracking if the call was successful - similar to how m_requests in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/nfc/NF... or m_deviceRequests in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webusb... https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:200: resolver->reject( On 2017/02/28 19:18:18, ikilpatrick wrote: > just rejectWithDOMException? Do you have an example for this where the callback already has a ScriptPromiseResolver? The examples for rejectWithDOMException seemed to use it only in cases without an existing resolver. Internally, this seems to just call resolver.reject() anyways? https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right): https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webauth/WebAuthentication.h:73: m_authenticatorRequests; // to use to keep track of requests. See On 2017/02/28 07:25:56, dcheng wrote: > Nit: consider moving the comment above the field declaration so it doesn't wrap > so much Eh, I don't think the comment needs to stay anyways, was more a note for me at first.
https://codereview.chromium.org/2702653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/DEPS (right): https://codereview.chromium.org/2702653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/DEPS:2: "+components/webauth" The presubmit is complaining because there's a missing comma at the end of the line
Description was changed from ========== Add webauth makeCredential mojom. This patch adds the webauth authenticator mojom interface. It has the function makeCredential and necessary structs. The function is used by the WebAuthentication class. BUG=664630 ========== to ========== Add webauth makeCredential mojom. Patch #1 of multiple This patch adds the webauth authenticator mojom interface. It has the function makeCredential and necessary structs. The function is used by the WebAuthentication class. BUG=664630 ==========
There are some dependent CLs for the typemapping and others that show how the mojom will be used in the Java impl. Working on the C++ impls, but expect that will be a couple CLs down the line. https://codereview.chromium.org/2702653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/DEPS (right): https://codereview.chromium.org/2702653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/DEPS:2: "+components/webauth" On 2017/03/01 03:55:08, dcheng wrote: > The presubmit is complaining because there's a missing comma at the end of the > line Done.
esprehn@chromium.org changed reviewers: + dglazkov@chromium.org - esprehn@chromium.org
https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:8: struct ScopedCredentialInfo { Nit: please add some more comments to these structs, pointers to specs, and what (if any formatting) is present in things like string or byte array fields. Are all the byte arrays / string fields things that take arbitrary input? https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:14: string rp_display_name; It's not clear what the difference between rp_display_name and display_name is here. In general, abbreviations are also discouraged. https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:18: string image_url; Nit: url.mojom.Url https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:28: string rp_id; Ditto: it's not clear what rp_ stands for here https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:23: namespace mojo { Why namespace mojo? https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:35: Vector<uint8_t> convertBufferSource(const blink::BufferSource& buffer) { Since there's already a dependent patchset, it's OK to leave these conversions like this -- but perhaps let's change ones where it's possible to use TypeConverter<T, U> to use that in a followup. https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:49: ScopedCredentialType convertScopedCredentialType(const WTF::String& credType) { Nit: omit WTF:: here and elsewhere https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:151: // TODO(kpaulhamus) validate parameters according to spec What sort of validation needs to be done? Note that we'll need to do this on the impl side as well
https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:8: struct ScopedCredentialInfo { On 2017/04/20 13:04:44, dcheng (OOO through May 2) wrote: > Nit: please add some more comments to these structs, pointers to specs, and what > (if any formatting) is present in things like string or byte array fields. Are > all the byte arrays / string fields things that take arbitrary input? Comments added. Yeah, the byte arrays are all blobs, and the string fields take arbitrary input. There's no formatting that the browser needs to understand here. https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:14: string rp_display_name; On 2017/04/20 13:04:43, dcheng (OOO through May 2) wrote: > It's not clear what the difference between rp_display_name and display_name is > here. In general, abbreviations are also discouraged. I've added comments for RelyingPartyAccount in one of the downstream CLs (#3). I'll un-abbreviate rp. https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:18: string image_url; On 2017/04/20 13:04:43, dcheng (OOO through May 2) wrote: > Nit: url.mojom.Url This is coming in a follow-up CL https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:28: string rp_id; On 2017/04/20 13:04:43, dcheng (OOO through May 2) wrote: > Ditto: it's not clear what rp_ stands for here Ok, I'll unabbreviate same as above. https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:23: namespace mojo { On 2017/04/20 13:04:44, dcheng (OOO through May 2) wrote: > Why namespace mojo? It seemed that PaymentRequest.cpp and NFC.cpp used the namespace for their mojo converters. Is it not necessary? https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:35: Vector<uint8_t> convertBufferSource(const blink::BufferSource& buffer) { On 2017/04/20 13:04:44, dcheng (OOO through May 2) wrote: > Since there's already a dependent patchset, it's OK to leave these conversions > like this -- but perhaps let's change ones where it's possible to use > TypeConverter<T, U> to use that in a followup. Sure, I can do that. TypeConverter, not structtraits? Does that apply to all of these that I can't use structtraits for right now? https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:49: ScopedCredentialType convertScopedCredentialType(const WTF::String& credType) { On 2017/04/20 13:04:44, dcheng (OOO through May 2) wrote: > Nit: omit WTF:: here and elsewhere Done. https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:151: // TODO(kpaulhamus) validate parameters according to spec On 2017/04/20 13:04:44, dcheng (OOO through May 2) wrote: > What sort of validation needs to be done? Note that we'll need to do this on the > impl side as well I removed this TODO in a downstream CL because the validation is happening either during the mojo conversion or in the impl - things like the adjustedTimeout and origin.
https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/200001/components/webauth/aut... components/webauth/authenticator.mojom:8: struct ScopedCredentialInfo { On 2017/04/22 01:04:44, kpaulhamus wrote: > On 2017/04/20 13:04:44, dcheng (OOO through May 2) wrote: > > Nit: please add some more comments to these structs, pointers to specs, and > what > > (if any formatting) is present in things like string or byte array fields. Are > > all the byte arrays / string fields things that take arbitrary input? > > Comments added. > Yeah, the byte arrays are all blobs, and the string fields take arbitrary input. > There's no formatting that the browser needs to understand here. Is there any limit on the size of fields here? Any chance they're fixed size? (I'm assuming not, but figured I may as well ask) https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:23: namespace mojo { On 2017/04/22 01:04:44, kpaulhamus wrote: > On 2017/04/20 13:04:44, dcheng (OOO through May 2) wrote: > > Why namespace mojo? > > It seemed that PaymentRequest.cpp and NFC.cpp used the namespace for their mojo > converters. Is it not necessary? Well, these weren't actually standard Mojo converters. If we change them to use TypeConverter, then they will probably need to live in the mojo namespace. But there's no actual need for that in this CL. https://codereview.chromium.org/2702653002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:35: Vector<uint8_t> convertBufferSource(const blink::BufferSource& buffer) { On 2017/04/22 01:04:44, kpaulhamus wrote: > On 2017/04/20 13:04:44, dcheng (OOO through May 2) wrote: > > Since there's already a dependent patchset, it's OK to leave these conversions > > like this -- but perhaps let's change ones where it's possible to use > > TypeConverter<T, U> to use that in a followup. > > Sure, I can do that. TypeConverter, not structtraits? Does that apply to all of > these that I can't use structtraits for right now? Yeah, TypeConverter instead of StructTraits, since it's not really possible to use it with GarbageCollected types right now. TypeConverter is not ideal, but it's: 1) in the renderer 2) will help make it easier to find and audit code that does these conversions. https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:54: array<uint8> id; Similarly, any structure or form to |id| here? Are there any limits on the size of inputs here? https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:68: makeCredential(RelyingPartyAccount account_information, Nit: MakeCredential https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:70: array<uint8> attestation_challenge, Similar question here: is this byte array input fixed size? Is there a reasonable upper bound? https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:72: => (array<ScopedCredentialInfo> scoped_credentials); I might be reading the spec incorrectly, but it seems like the makeCredential() web API only returns one ScopedCredentialInfo. Does this need to return an array?
https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:54: array<uint8> id; On 2017/04/24 12:25:26, dcheng (OOO through May 2) wrote: > Similarly, any structure or form to |id| here? Are there any limits on the size > of inputs here? Ah, yeah, 255 bytes. Would I note that in a comment and then enforce during the conversion? https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:68: makeCredential(RelyingPartyAccount account_information, On 2017/04/24 12:25:26, dcheng (OOO through May 2) wrote: > Nit: MakeCredential Done. https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:70: array<uint8> attestation_challenge, On 2017/04/24 12:25:26, dcheng (OOO through May 2) wrote: > Similar question here: is this byte array input fixed size? Is there a > reasonable upper bound? Nope, this is just an encrypted blob with stuff for the server to use to store state. https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:72: => (array<ScopedCredentialInfo> scoped_credentials); On 2017/04/24 12:25:26, dcheng (OOO through May 2) wrote: > I might be reading the spec incorrectly, but it seems like the makeCredential() > web API only returns one ScopedCredentialInfo. Does this need to return an > array? No, you're reading it correctly; the spec changed. The mojom correctly reflects it in this downstream patch: https://codereview.chromium.org/2788823002/
kpaulhamus@chromium.org changed reviewers: + esprehn@chromium.org - dglazkov@chromium.org
kpaulhamus@chromium.org changed reviewers: + haraken@chromium.org - esprehn@chromium.org
https://codereview.chromium.org/2702653002/diff/260001/components/webauth/BUI... File components/webauth/BUILD.gn (right): https://codereview.chromium.org/2702653002/diff/260001/components/webauth/BUI... components/webauth/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2702653002/diff/260001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/260001/components/webauth/aut... components/webauth/authenticator.mojom:24: string relying_party_display_name; Did this get changed in the spec? \o/ ? https://codereview.chromium.org/2702653002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:143: if (!executionContext->IsSecureContext(errorMessage)) { should this go before the !m-authenticator check?
https://codereview.chromium.org/2702653002/diff/260001/components/webauth/BUI... File components/webauth/BUILD.gn (right): https://codereview.chromium.org/2702653002/diff/260001/components/webauth/BUI... components/webauth/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/24 22:11:44, ikilpatrick wrote: > 2017 Done. https://codereview.chromium.org/2702653002/diff/260001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/260001/components/webauth/aut... components/webauth/authenticator.mojom:24: string relying_party_display_name; On 2017/04/24 22:11:44, ikilpatrick wrote: > Did this get changed in the spec? \o/ ? Welll.. it did... but this whole structure is actually getting changed again due to the merge with the Cred Man API. All of that will come in follow-up CLs. https://codereview.chromium.org/2702653002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:143: if (!executionContext->IsSecureContext(errorMessage)) { On 2017/04/24 22:11:44, ikilpatrick wrote: > should this go before the !m-authenticator check? Done.
https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/240001/components/webauth/aut... components/webauth/authenticator.mojom:54: array<uint8> id; On 2017/04/24 17:48:34, kpaulhamus wrote: > On 2017/04/24 12:25:26, dcheng (OOO through May 2) wrote: > > Similarly, any structure or form to |id| here? Are there any limits on the > size > > of inputs here? > > Ah, yeah, 255 bytes. Would I note that in a comment and then enforce during the > conversion? I guess that's the best we can do for now, sadly. https://codereview.chromium.org/2702653002/diff/280001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/280001/components/webauth/aut... components/webauth/authenticator.mojom:39: int32 timeout_seconds; Mind adding a todo to use something like mojo.common.mojom.TimeDelta here as well in the future? (No need for image_url, since you mentioned you already have a CL for that) https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:35: Vector<uint8_t> convertBufferSource(const blink::BufferSource& buffer) { Nit: #include <stdint.h> for uint8_t, and uppercase method/function names to follow the new Blink naming conventions. https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:70: auto mojoAccount = RelyingPartyAccount::New(); Similarly, account_information, mojo_account, etc for other variable names. https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:138: if (!executionContext->IsSecureContext(errorMessage)) { Can you check if this is still needed? I think that the IDL compiler should properly support [SecureContext] for interfaces, based on https://bugs.chromium.org/p/chromium/issues/detail?id=634270. https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right): https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.h:56: webauth::mojom::blink::Authenticator* authenticator() const { Per the Blink style guide, these functions should all be NamedLikeThis(), even the getter. https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.h:71: webauth::mojom::blink::AuthenticatorPtr m_authenticator; authenticator_, authenticator_requests_
Also had to add layouttest updates due to blink naming changes. https://codereview.chromium.org/2702653002/diff/280001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/280001/components/webauth/aut... components/webauth/authenticator.mojom:39: int32 timeout_seconds; On 2017/04/25 12:52:14, dcheng (OOO through May 2) wrote: > Mind adding a todo to use something like mojo.common.mojom.TimeDelta here as > well in the future? > > (No need for image_url, since you mentioned you already have a CL for that) Yeah, the TODO is present in CL#2 (you made the same comment in that CL) https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:35: Vector<uint8_t> convertBufferSource(const blink::BufferSource& buffer) { On 2017/04/25 12:52:14, dcheng (OOO through May 2) wrote: > Nit: #include <stdint.h> for uint8_t, and uppercase method/function names to > follow the new Blink naming conventions. Done. https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:70: auto mojoAccount = RelyingPartyAccount::New(); On 2017/04/25 12:52:14, dcheng (OOO through May 2) wrote: > Similarly, account_information, mojo_account, etc for other variable names. Done. https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:138: if (!executionContext->IsSecureContext(errorMessage)) { On 2017/04/25 12:52:14, dcheng (OOO through May 2) wrote: > Can you check if this is still needed? I think that the IDL compiler should > properly support [SecureContext] for interfaces, based on > https://bugs.chromium.org/p/chromium/issues/detail?id=634270. Yeah, looks like it's not needed anymore. Thanks! https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right): https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.h:56: webauth::mojom::blink::Authenticator* authenticator() const { On 2017/04/25 12:52:14, dcheng (OOO through May 2) wrote: > Per the Blink style guide, these functions should all be NamedLikeThis(), even > the getter. Done. https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.h:71: webauth::mojom::blink::AuthenticatorPtr m_authenticator; On 2017/04/25 12:52:14, dcheng (OOO through May 2) wrote: > authenticator_, authenticator_requests_ Done.
(Sorry for the review delay, mostly caught up now) https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:70: auto mojoAccount = RelyingPartyAccount::New(); On 2017/05/03 17:00:45, kpaulhamus wrote: > On 2017/04/25 12:52:14, dcheng (OOO through May 2) wrote: > > Similarly, account_information, mojo_account, etc for other variable names. > > Done. Also: mojoAccount => mojo_account https://codereview.chromium.org/2702653002/diff/360001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/360001/components/webauth/aut... components/webauth/authenticator.mojom:58: // A 255-byte blob representing a credential key handle Is it always exactly 255 byte, or up to? https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webauth/idl.html (right): https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webauth/idl.html:82: Promise <ScopedCredentialInfo> MakeCredential ( The webidl still should be makeCredential though. https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right): https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.h:72: HeapHashSet<Member<ScriptPromiseResolver>> authenticatorRequests_; Nit: authenticator_requests_ (no camel case) https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.idl (right): https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.idl:11: [CallWith=ScriptState] Promise<sequence<ScopedCredentialInfo>> MakeCredential(RelyingPartyAccount accountInformation, sequence<ScopedCredentialParameters> cryptoParameters, BufferSource attestationChallenge, optional ScopedCredentialOptions options); WebIDL methods should be named makeCredential() and getAssertion() to match web-style naming. It's a bit confusing, but the rule is: - Things that are directly implementing a function called by IDL bindings should be namedLikedThis(): this way, the C++ tests resemble the way JS looks more closely in terms of style and naming. - Things that are just supporting functionality defined in WebIDL or helper functions, etc. should be NamedLikeThis().
https://codereview.chromium.org/2702653002/diff/360001/components/webauth/aut... File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/360001/components/webauth/aut... components/webauth/authenticator.mojom:58: // A 255-byte blob representing a credential key handle On 2017/05/04 07:22:35, dcheng wrote: > Is it always exactly 255 byte, or up to? Good catch, it's up to. https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webauth/idl.html (right): https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webauth/idl.html:82: Promise <ScopedCredentialInfo> MakeCredential ( On 2017/05/04 07:22:35, dcheng wrote: > The webidl still should be makeCredential though. d'oh. Right. I'll undo the layouttest rebaselining then. https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right): https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.h:72: HeapHashSet<Member<ScriptPromiseResolver>> authenticatorRequests_; On 2017/05/04 07:22:35, dcheng wrote: > Nit: authenticator_requests_ (no camel case) Done. https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.idl (right): https://codereview.chromium.org/2702653002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.idl:11: [CallWith=ScriptState] Promise<sequence<ScopedCredentialInfo>> MakeCredential(RelyingPartyAccount accountInformation, sequence<ScopedCredentialParameters> cryptoParameters, BufferSource attestationChallenge, optional ScopedCredentialOptions options); On 2017/05/04 07:22:35, dcheng wrote: > WebIDL methods should be named makeCredential() and getAssertion() to match > web-style naming. It's a bit confusing, but the rule is: > - Things that are directly implementing a function called by IDL bindings should > be namedLikedThis(): this way, the C++ tests resemble the way JS looks more > closely in terms of style and naming. > - Things that are just supporting functionality defined in WebIDL or helper > functions, etc. should be NamedLikeThis(). Ah, okay, I'm finally getting it. Yeah, the rename happened in the middle of this and I got mixed up.
mojo lgtm but... It would help code reviewers greatly to address as many comments as possible in the given CL. It was a bit unwieldy to review, since I had to keep track of which parts have pending TODOs and which don't, and which ones will be addressed by followup CLs. Honestly, I would be more comfortable if we just addressed all the TODOs that were raised in this review in this CL, despite the fact that it will require some rebasing in dependent CLs. That way we have the context for the changes and the reviewer can be confident that their concerns are addressed (and won't be accidentally missed in future followups).
On 2017/05/08 21:32:47, dcheng wrote: > mojo lgtm but... > > It would help code reviewers greatly to address as many comments as possible in > the given CL. It was a bit unwieldy to review, since I had to keep track of > which parts have pending TODOs and which don't, and which ones will be addressed > by followup CLs. > > Honestly, I would be more comfortable if we just addressed all the TODOs that > were raised in this review in this CL, despite the fact that it will require > some rebasing in dependent CLs. That way we have the context for the changes and > the reviewer can be confident that their concerns are addressed (and won't be > accidentally missed in future followups). Yeah, it did get a little confusing. I'm sorry about that. Some of the comments were repeat comments from downstream CLs that were addressed there, but you're totally right - no reason not to port them to this CL. I went back through the comments and TODOs. I added comments for the fields in RelyingPartyUserInfo. What's left is: 1) write typemaps or typeconverters to replace the conversion methods - relyingPartyAccount already done in a parent CL, the rest will be in follow up CLs 2) use url.mojom.Url and TimeDelta in authenticator.mojom - working with dcheng to figure out the mojo voodoo to get these to work. If we don't soon, will be in a follow up CL.
kpaulhamus@chromium.org changed reviewers: + jochen@chromium.org - yzshen@chromium.org
lgtm % comments. https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:180: authenticator_.reset(); nothing is needed here except for maybe a TODO but you'll need to determine if the promises should get rejected upon document destruction. You'll need to follow up on some whatwg/html issues with what the latest is there. https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:218: authenticator_requests_.erase(resolver); this isn't needed? (done in MarkRequestComplete?)
rubberstamp lgtm
The CQ bit was checked by kpaulhamus@chromium.org to run a CQ dry run
https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:180: authenticator_.reset(); On 2017/05/09 20:48:58, ikilpatrick wrote: > nothing is needed here except for maybe a TODO > > but you'll need to determine if the promises should get rejected upon document > destruction. You'll need to follow up on some whatwg/html issues with what the > latest is there. Talked with jyasskin about this and it sounds like there isn't a consistent policy atm; and since this would only come up if we're passing a promise from one document to another and it doesn't look like this is something RPs need nor do we want to support RPs ability to do so, for now we're saying promises should not get rejected. https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:218: authenticator_requests_.erase(resolver); On 2017/05/09 20:48:58, ikilpatrick wrote: > this isn't needed? (done in MarkRequestComplete?) Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kpaulhamus@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, jochen@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2702653002/#ps420001 (title: "Change to ChromeInterfaceRegistrar.java shouldn't be here - left over from splitting of Android imp…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/09 20:42:04, kpaulhamus wrote: > On 2017/05/08 21:32:47, dcheng wrote: > > mojo lgtm but... > > > > It would help code reviewers greatly to address as many comments as possible > in > > the given CL. It was a bit unwieldy to review, since I had to keep track of > > which parts have pending TODOs and which don't, and which ones will be > addressed > > by followup CLs. > > > > Honestly, I would be more comfortable if we just addressed all the TODOs that > > were raised in this review in this CL, despite the fact that it will require > > some rebasing in dependent CLs. That way we have the context for the changes > and > > the reviewer can be confident that their concerns are addressed (and won't be > > accidentally missed in future followups). > > Yeah, it did get a little confusing. I'm sorry about that. Some of the comments > were repeat comments from downstream CLs that were addressed there, but you're > totally right - no reason not to port them to this CL. I went back through the > comments and TODOs. I added comments for the fields in RelyingPartyUserInfo. > What's left is: > > 1) write typemaps or typeconverters to replace the conversion methods - > relyingPartyAccount already done in a parent CL, the rest will be in follow up > CLs > 2) use url.mojom.Url and TimeDelta in authenticator.mojom - working with dcheng > to figure out the mojo voodoo to get these to work. If we don't soon, will be in > a follow up CL. Sorry for not poking at this sooner. The compile error you mentioned earlier is because if you want to use the typemapped type, the mojom rule also has to specify a dependency on //url/mojo:url_mojom_gurl (if using url.mojom.Url), etc.
The CQ bit was unchecked by commit-bot@chromium.org
Exhausted attempt retry quota accross all builders (set as global_retry_quota=2 in cq.cfg of this project)
The CQ bit was checked by kpaulhamus@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kpaulhamus@chromium.org to run a CQ dry run
The CQ bit was unchecked by kpaulhamus@chromium.org
The CQ bit was checked by kpaulhamus@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, jochen@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2702653002/#ps440001 (title: "Add Authenticator to interface provider to correct try job 'service_manager' error.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kpaulhamus@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, jochen@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2702653002/#ps480001 (title: "Removing obsolete idl-expected file to pass layouttests")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kpaulhamus@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kpaulhamus@google.com
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": 480001, "attempt_start_ts": 1494613487380820, "parent_rev": "81640a046fb6659d1ab1c2fdcb53863244aefbc0", "commit_rev": "b1f03c340eaaf193e6d4cae7047600c0eddd5ffb"}
Message was sent while issue was closed.
Description was changed from ========== Add webauth makeCredential mojom. Patch #1 of multiple This patch adds the webauth authenticator mojom interface. It has the function makeCredential and necessary structs. The function is used by the WebAuthentication class. BUG=664630 ========== to ========== Add webauth makeCredential mojom. Patch #1 of multiple This patch adds the webauth authenticator mojom interface. It has the function makeCredential and necessary structs. The function is used by the WebAuthentication class. BUG=664630 Review-Url: https://codereview.chromium.org/2702653002 Cr-Commit-Position: refs/heads/master@{#471498} Committed: https://chromium.googlesource.com/chromium/src/+/b1f03c340eaaf193e6d4cae70476... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001) as https://chromium.googlesource.com/chromium/src/+/b1f03c340eaaf193e6d4cae70476...
Message was sent while issue was closed.
why was a new component added for this mojom file? Code in chrome/browser or content/browser can include mojom's declared in blink, which this is. Can you please move this to blink?
Message was sent while issue was closed.
On 2017/06/13 04:39:05, jam (ooo 6-14 - 6-20) wrote: > why was a new component added for this mojom file? Code in chrome/browser or > content/browser can include mojom's declared in blink, which this is. > > Can you please move this to blink? Hi John, I was under the impression that the mojom and implementation should live in components, and one reason that comes to mind is because I will be writing both C++ and Java implementations of the mojom (although I can't tell you exactly where I heard that, so maybe I'm mistaken). I've been referencing payments as an example. In any case, could you take a look at https://codereview.chromium.org/2788823002/ as well and make sure things are in the right place? Should the mojom implementation code in that CL move somewhere else as well?
Message was sent while issue was closed.
ping On Mon, Jun 12, 2017 at 9:39 PM, <jam@chromium.org> wrote: > why was a new component added for this mojom file? Code in chrome/browser > or > content/browser can include mojom's declared in blink, which this is. > > Can you please move this to blink? > > https://codereview.chromium.org/2702653002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2017/06/13 16:31:51, kpaulhamus wrote: > On 2017/06/13 04:39:05, jam (ooo 6-14 - 6-20) wrote: > > why was a new component added for this mojom file? Code in chrome/browser or > > content/browser can include mojom's declared in blink, which this is. > > > > Can you please move this to blink? > > Hi John, I was under the impression that the mojom and implementation should > live in components, and one reason that comes to mind is because I will be > writing both C++ and Java implementations of the mojom (although I can't tell > you exactly where I heard that, so maybe I'm mistaken). I've been referencing > payments as an example. In any case, could you take a look at > https://codereview.chromium.org/2788823002/ as well and make sure things are in > the right place? Should the mojom implementation code in that CL move somewhere > else as well? (ah just noticed this, for some reason I didn't get an email) Note payments moved to blink already, there was no need for that to be in a component. Java/C++ doesn't affect where the mojom is. Can you move this to webkit please? |