Blink-layer update to match WebAuthN spec

Blink-layer update to match WebAuthN spec

1 of many CLs to merge WebAuthN with the Cred Man API. This change
renames idl components and updates the corresponding idl test.
(This changes as much as possible short of depending on anything in
credential manager).

BUG=664630
Review-Url: https://codereview.chromium.org/2966523002
Cr-Commit-Position: refs/heads/master@{#486648}
Committed: https://chromium.googlesource.com/chromium/src/+/04b66da848df095ca21d54194b7c59b0d01526ab

[kpaulhamus, 2017-06-29 16:19:35]

Description was changed from

==========
Blink-layer update to match WebAuthN spec draft 05

1 of many CLs to merge WebAuthN with the Cred Man API. This change
renames idl components and updates the corresponding idl test.
(This changes as much as possible short of depending on anything in
credential manager).

BUG=664630
==========

to

==========
Blink-layer update to match WebAuthN spec draft 05

1 of many CLs to merge WebAuthN with the Cred Man API. This change
renames idl components and updates the corresponding idl test.
(This changes as much as possible short of depending on anything in
credential manager).

BUG=664630
==========

[kpaulhamus, 2017-06-29 16:19:35]

kpaulhamus@chromium.org changed reviewers:
+ engedy@chromium.org, mkwst@chromium.org, vasilii@chromium.org

[Mike West, 2017-06-30 08:06:36]

Mostly IDL comments.

Now that I've actually looked through the code, forget about updating
//external/wpt. Let's save that for when we move things onto the credential
management API, because we'd just be failing all the tests right now. :)

Speaking of tests... You're totally going to have functional tests at some
point, right?

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/webauth/idl.html (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webauth/idl.html:5: <script
src=../resources/idlharness.js></script>
Can we move this file to
`//third_party/WebKit/LayoutTests/external/wpt/webauthn`? That will
automagically upstream the tests to the repository we shared with other browser
vendors. I'm surprised those tests aren't failing, actually...

Ah. They're marked as timing out in
//third_party/WebKit/LayoutTests/TestExpectations... Let's undo that. :)

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:31:
signature_(signature) {}
Should this constructor be private?

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:33:
virtual ~AuthenticatorAssertionResponse() {}
I don't think you need this, since you define an explicit destructor in the
parent.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.idl:12:
readonly attribute ArrayBuffer signature;
`[SameObject]` for both of these.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:28:
attestation_object_(attestation_object) {}
Should this constructor be private?

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:30:
virtual ~AuthenticatorAttestationResponse() {}
I don't think you need this, since you define an explicit destructor in the
parent.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.idl:11:
readonly attribute ArrayBuffer attestationObject;
[SameObject]

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:19: virtual
~AuthenticatorResponse() {}
Please move this under the static method.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:26: :
client_data_json_(client_data_json) {}
Consider making this protected, as I don't think we'll ever want to construct
one of these directly.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.idl:11: readonly
attribute ArrayBuffer clientDataJSON;
`[SameObject]`

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorSelectionCriteria.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorSelectionCriteria.idl:11:
typedef sequence<AAGUID> AuthenticatorSelectionList;
Are we planning on implementing this extension? Can we just drop it and `AAGUID`
for now?

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorSelectionCriteria.idl:15:
enum Attachment {
Should this be `AuthenticatorAttachment`
(https://w3c.github.io/webauthn/#enumdef-authenticatorattachment)?

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorSelectionCriteria.idl:23:
Attachment attachment;
Ditto.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/CollectedClientData.idl (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/CollectedClientData.idl:5: //
https://w3c.github.io/webauthn/#collectedclientdata
https://w3c.github.io/webauthn/#dictdef-collectedclientdata

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/CollectedClientData.idl:10: required
DOMString hashAlg;
s/hashAlg/hashAlgorithm/

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredential.h (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.h:16: class
PublicKeyCredential final : public GarbageCollected<PublicKeyCredential>,
Does this compile? You're inheriting from `Credential` in the IDL file, but not
here, which seems like it would explode.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.h:27: :
raw_id_(raw_id), response_(response) {}
Private.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl:12: readonly
attribute AuthenticatorResponse response;
`[SameObject]`.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredentialDescriptor.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialDescriptor.idl:5:
// https://w3c.github.io/webauthn/#enumdef-transport
https://w3c.github.io/webauthn/#enumdef-authenticatortransport

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredentialParameters.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialParameters.idl:9:
};
I don't understand why y'all need this. It's fine to have in as long as it's in
the spec, but I thought we'd removed it.

[kpaulhamus, 2017-06-30 10:59:38]

kpaulhamus@chromium.org changed reviewers:
+ dcheng@chromium.org

[kpaulhamus, 2017-06-30 10:59:40]

This patch includes an update to unit tests. The next CL will complete the merge
with CM and change the call to navigator.credentials.create, and will finally be
able to integrate with the more encompassing CM testing.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/webauth/idl.html (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webauth/idl.html:5: <script
src=../resources/idlharness.js></script>
On 2017/06/30 08:06:35, Mike West wrote:
> Can we move this file to
> `//third_party/WebKit/LayoutTests/external/wpt/webauthn`? That will
> automagically upstream the tests to the repository we shared with other
browser
> vendors. I'm surprised those tests aren't failing, actually...
> 
> Ah. They're marked as timing out in
> //third_party/WebKit/LayoutTests/TestExpectations... Let's undo that. :)

Noting per your other comment that we're going to leave these as-is for a little
longer.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:31:
signature_(signature) {}
On 2017/06/30 08:06:35, Mike West wrote:
> Should this constructor be private?

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:33:
virtual ~AuthenticatorAssertionResponse() {}
On 2017/06/30 08:06:35, Mike West wrote:
> I don't think you need this, since you define an explicit destructor in the
> parent.

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.idl:12:
readonly attribute ArrayBuffer signature;
On 2017/06/30 08:06:35, Mike West wrote:
> `[SameObject]` for both of these.

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:28:
attestation_object_(attestation_object) {}
On 2017/06/30 08:06:35, Mike West wrote:
> Should this constructor be private?

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:30:
virtual ~AuthenticatorAttestationResponse() {}
On 2017/06/30 08:06:35, Mike West wrote:
> I don't think you need this, since you define an explicit destructor in the
> parent.

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.idl:11:
readonly attribute ArrayBuffer attestationObject;
On 2017/06/30 08:06:35, Mike West wrote:
> [SameObject]

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:19: virtual
~AuthenticatorResponse() {}
On 2017/06/30 08:06:36, Mike West wrote:
> Please move this under the static method.

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:26: :
client_data_json_(client_data_json) {}
On 2017/06/30 08:06:36, Mike West wrote:
> Consider making this protected, as I don't think we'll ever want to construct
> one of these directly.

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.idl:11: readonly
attribute ArrayBuffer clientDataJSON;
On 2017/06/30 08:06:36, Mike West wrote:
> `[SameObject]`

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorSelectionCriteria.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorSelectionCriteria.idl:11:
typedef sequence<AAGUID> AuthenticatorSelectionList;
On 2017/06/30 08:06:36, Mike West wrote:
> Are we planning on implementing this extension? Can we just drop it and
`AAGUID`
> for now?

Sure! Happy to drop.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorSelectionCriteria.idl:15:
enum Attachment {
On 2017/06/30 08:06:36, Mike West wrote:
> Should this be `AuthenticatorAttachment`
> (https://w3c.github.io/webauthn/#enumdef-authenticatorattachment)?

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/AuthenticatorSelectionCriteria.idl:23:
Attachment attachment;
On 2017/06/30 08:06:36, Mike West wrote:
> Ditto.

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/CollectedClientData.idl (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/CollectedClientData.idl:5: //
https://w3c.github.io/webauthn/#collectedclientdata
On 2017/06/30 08:06:36, Mike West wrote:
> https://w3c.github.io/webauthn/#dictdef-collectedclientdata

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/CollectedClientData.idl:10: required
DOMString hashAlg;
On 2017/06/30 08:06:36, Mike West wrote:
> s/hashAlg/hashAlgorithm/

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredential.h (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.h:16: class
PublicKeyCredential final : public GarbageCollected<PublicKeyCredential>,
On 2017/06/30 08:06:36, Mike West wrote:
> Does this compile? You're inheriting from `Credential` in the IDL file, but
not
> here, which seems like it would explode.

Yeah, it does compile, and run. Is that a bug?

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.h:27: :
raw_id_(raw_id), response_(response) {}
On 2017/06/30 08:06:36, Mike West wrote:
> Private.

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl (right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl:12: readonly
attribute AuthenticatorResponse response;
On 2017/06/30 08:06:36, Mike West wrote:
> `[SameObject]`.

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredentialDescriptor.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialDescriptor.idl:5:
// https://w3c.github.io/webauthn/#enumdef-transport
On 2017/06/30 08:06:36, Mike West wrote:
> https://w3c.github.io/webauthn/#enumdef-authenticatortransport

Done.

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredentialParameters.idl
(right):

https://codereview.chromium.org/2966523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialParameters.idl:9:
};
On 2017/06/30 08:06:36, Mike West wrote:
> I don't understand why y'all need this. It's fine to have in as long as it's
in
> the spec, but I thought we'd removed it.

It's not gone yet... maybe it's still being discussed?

[kpaulhamus, 2017-06-30 11:35:31]

Description was changed from

==========
Blink-layer update to match WebAuthN spec draft 05

1 of many CLs to merge WebAuthN with the Cred Man API. This change
renames idl components and updates the corresponding idl test.
(This changes as much as possible short of depending on anything in
credential manager).

BUG=664630
==========

to

==========
Blink-layer update to match WebAuthN spec

1 of many CLs to merge WebAuthN with the Cred Man API. This change
renames idl components and updates the corresponding idl test.
(This changes as much as possible short of depending on anything in
credential manager).

BUG=664630
==========

[Mike West, 2017-06-30 16:20:18]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 16:20:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng1, 2017-06-30 16:23:54]

dcheng@google.com changed reviewers:
+ dcheng@google.com

[dcheng1, 2017-06-30 16:23:55]

some drive-bys (no need to block on me)

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/webauth/authenticator.mojom
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:93: //
[ScopedCredentialInfo] will only be set if status == SUCCESS.
Nit: this comment got lost in the updates.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/webauth/authenticator.mojom
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:32:
array<uint8> rawId;
Nit: raw_id

[commit-bot: I haz the power, 2017-06-30 18:12:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-30 18:12:22]

Dry run: 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_...)

[Mike West, 2017-07-03 08:22:07]

LGTM once you address dcheng@'s comments and some test nits.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/mac-mac10.11/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/mac-mac10.11/webexposed/global-interface-listing-expected.txt:6393:
method constructor
I don't think we need this file at all? Can you avoid adding it? :)

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/mac-retina/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/mac-retina/webexposed/global-interface-listing-expected.txt:710:
setter z
I don't think we need this file at all? Can you avoid adding it? :)

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/mac/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/mac/webexposed/global-interface-listing-expected.txt:710:
setter z
Here too.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/win/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/win/webexposed/global-interface-listing-expected.txt:710:
setter z
Some of this looks like a rebase gone wrong.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/mojo-loading/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/mojo-loading/webexposed/global-interface-listing-expected.txt:365:
interface AuthenticationAssertion
I don't think we need this file at all? Can you avoid adding it? :)

[vasilii, 2017-07-05 14:36:13]

lgtm

https://codereview.chromium.org/2966523002/diff/20001/content/browser/webauth...
File content/browser/webauth/authenticator_impl.cc (right):

https://codereview.chromium.org/2966523002/diff/20001/content/browser/webauth...
content/browser/webauth/authenticator_impl.cc:20: const char
kMakeCredentialType[] = "navigator.id.makeCredential";
The new trend is 'constexpr' wherever possible.

https://codereview.chromium.org/2966523002/diff/20001/content/browser/webauth...
File content/browser/webauth/authenticator_impl_unittest.cc (right):

https://codereview.chromium.org/2966523002/diff/20001/content/browser/webauth...
content/browser/webauth/authenticator_impl_unittest.cc:126:
opts->adjusted_timeout = base::TimeDelta::FromMilliseconds(60000);
Why not FromSeconds?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:30:
virtual ~AuthenticatorAttestationResponse() {}
override

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:24: const
int64_t kAdjustedTimeoutUpperInMilliseconds = 12000;
constexpr

[engedy, 2017-07-05 18:51:03]

Thanks! LGTM % comments, mostly nits.

@Mike, Daniel, there are couple of questions for you below, PTAL.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:25: 
optional nit: Do we want to supply a user-declared destructor here? I suppose
this depends on the answer to the other question, i.e. whether we are okay with
having it inlined.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:28: }
nit: Add blank line.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:31:
DEFINE_INLINE_TRACE() {
nit: I think this should be DEFINE_INLINE_VIRTUAL_TRACE.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:32:
AuthenticatorResponse::Trace(visitor);
nit: The GC API Guidelines recommend putting the delegation call at the end of
the trace method. See:
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:25:
AuthenticatorAttestationResponse(DOMArrayBuffer* client_data_json,
nit: Make constructor private if possible.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:37:
AuthenticatorResponse::Trace(visitor);
nit: Put delegation call at the end here, as well in a couple other files.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:9: #include
"platform/bindings/ScriptWrappable.h"
Please add `#include "platform/heap/Handle.h"` in all files using
GarbageCollected<> and friends.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:13: class
AuthenticatorResponse
optional nit: I wonder if most of these classes should be marked MODULES_EXPORT?
@Mike, @Daniel?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:30:
AuthenticatorResponse(DOMArrayBuffer* client_data_json)
nit: explicit

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:30:
AuthenticatorResponse(DOMArrayBuffer* client_data_json)
optional nit: In Chromium-land these constructor/destructors would be deemed
complex enough not to be inlined. @Mike, @Daniel, I wonder if the same applies
in Blink-land? If we decide to make them out-of-line (in a .cpp file), that
would apply to a couple classes in this CL.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/CollectedClientData.idl (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/CollectedClientData.idl:11: DOMString
tokenBindingId;
nit: Please add TODO(crbug.com/...) for extension fields.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/MakeCredentialOptions.idl
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/MakeCredentialOptions.idl:13:
sequence<PublicKeyCredentialDescriptor> excludeList;
s/excludeList/excludeCredentials/, also do we need the default value here? (=
[];)

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/MakeCredentialOptions.idl:15: };
nit: Add TODO(crbug.com/...) for extension fields.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl:9:
SecureContext
Let's add Exposed=Window as well for clarity.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl:13: };
nit: Add TODO for |clientExtensionResults| field.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredentialParameters.idl
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialParameters.idl:15:
required AlgorithmIdentifier algorithm;
s/algorithm/alg/

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialRequestOptions.idl
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialRequestOptions.idl:11:
sequence <PublicKeyCredentialDescriptor> allowList;
s/allowList/allowCredentials/

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialRequestOptions.idl:11:
sequence <PublicKeyCredentialDescriptor> allowList;
Do we need the default value here, i.e. `allowCredentials = []`?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialRequestOptions.idl:12:
};
nit: Add TODO(crbug.com/...) for |extensions| field?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:216: void
WebAuthentication::ContextDestroyed(ExecutionContext*) {
Hmm, how come we don't need this anymore?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:1: // Copyright
2016 The Chromium Authors. All rights reserved.
nit: Time machine? :)

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:12: #include
"core/dom/Document.h"
nit: Will be unneeded once the ContextLifecycleObserver is gone.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:17: #include
"modules/webauth/PublicKeyCredential.h"
nit: AuthenticatorAssertionResponse and PublicKeyCredential look unused?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:24: const
int64_t kAdjustedTimeoutUpperInMilliseconds = 12000;
On 2017/07/05 14:36:13, vasilii wrote:
> constexpr

Also, could this directly be `constexpr WTF::TimeDelta`? That would allow using
much shorter names, using FromMinutes, and not including stdint.h.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:24: const
int64_t kAdjustedTimeoutUpperInMilliseconds = 12000;
Missing trailing 0? i.e. 12 sec instead of 2 minutes?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:43:
vector.Append(static_cast<uint8_t*>(
nit: DCHECK(buffer.isArrayBufferView());

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:52: const
String& cred_type) {
nit: Let's avoid abbreviations, and make this either credential_type or type.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:116: if
(parameter.hasType()) {
nit: I'm not very familiar with this, but I wonder if this can really be false,
given everything is `required` in the IDLs?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:123: int64_t
predicted_timeout;
nit: What does predicted mean in this context? I don't find a mention to it in
the spec.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:130:
mojo_options->adjusted_timeout = WTF::TimeDelta::FromMilliseconds(std::max(
nit: Can we use WTF::clampTo here?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:176: }
nit: Add blank line after

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:205:
ScriptPromise promise = resolver->Promise();
nit: Do we need to make this call here because of side-effects of calling
Promise()? If so, could you please add a comment to this effect?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:259:
ScriptPromise WebAuthentication::RejectIfNotSupported(
Looks like this is not used anymore?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:8: #include
"bindings/core/v8/ArrayBufferOrArrayBufferView.h"
nit: No longer used in the header, move to .cpp.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:9: #include
"bindings/core/v8/ScriptPromise.h"
nit: I think the complete type is not needed in the header.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:11: #include
"core/dom/DOMArrayBuffer.h"
nit: No longer used in the header, move to .cpp.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:12: #include
"modules/webauth/MakeCredentialOptions.h"
nit: Fwd-declaration suffices in the header, move to .cpp.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:23: typedef
ArrayBufferOrArrayBufferView BufferSource;
nit: No longer used in the header, move to .cpp.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:28: public
ContextLifecycleObserver {
nit: No longer used, including its header.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:40: ScriptPromise
makeCredential(ScriptState*, MakeCredentialOptions&);
nit: Can the second argument be const too here?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:49: void
OnMakeCredential(ScriptPromiseResolver*,
nit: Do these need to be public?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:49: void
OnMakeCredential(ScriptPromiseResolver*,
nit: Need fwd-declaration for class ScriptPromiseResolver.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.idl (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.idl:5: //
https://w3c.github.io/webauthn/#webauthentication
nit: Note that this link is no longer valid. Maybe remove, add TODO, or add new
link to CM API?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.idl:11:
[CallWith=ScriptState] Promise<sequence<AuthenticatorAttestationResponse>>
makeCredential(MakeCredentialOptions publicKey);
nit: Maybe I'm missing something, but the new spec does not seem to support
returning multiple results. WDYT about removing the sequence<...> references
here already in this CL?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/webauth/authenticator.mojom
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:29: //
The base64url encoding of rawId.
nit: ... of |raw_id|.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:41: //
TODO(kpaulhamus): add AuthenticatorSelectionCriteria
nit: I think this goes to MakeCredentialOptions?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:46:
struct AuthenticatorResponse {
If I understand correctly, this needs to be polymorphic as per the WebAuthN
spec. How is the receiver end going to decide on the subtype here?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:55: //
Information about the relying party and the user account held by that
nit: Not sure about the semantics, but how about "the account held by the user
at that relying party"?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:84: //
Optional parameters that are used during makeCredential.
comment nit: These do not seem to be optional anymore. How about:

// Parameters passed into calls to MakeCredential.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:86:
PublicKeyCredentialEntity relying_party; // Relying party information.
style nit: Could you please move the comments above the members, and leave one
blank line in-between members throughout this file?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:86:
PublicKeyCredentialEntity relying_party; // Relying party information.
nit: Mention this corresponds to |rp| in the IDL.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:91:
array<PublicKeyCredentialDescriptor> exclude_list;
optional nit: s/exlude_list/excludeCredentials/, for consistency with IDL.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:95:
PUBLICKEY,
nit: I'm never sure about converting dashes to ALL_CAPS, but would PUBLIC_KEY
look better?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:109: enum
Transport {
nit: AuthenticatorTransport

[engedy, 2017-07-10 14:32:36]

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:13: class
AuthenticatorResponse
On 2017/07/05 18:51:04, engedy wrote:
> optional nit: I wonder if most of these classes should be marked
MODULES_EXPORT?
> @Mike, @Daniel?

Short answer: yes.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:30:
AuthenticatorResponse(DOMArrayBuffer* client_data_json)
On 2017/07/05 18:51:04, engedy wrote:
> optional nit: In Chromium-land these constructor/destructors would be deemed
> complex enough not to be inlined. @Mike, @Daniel, I wonder if the same applies
> in Blink-land? If we decide to make them out-of-line (in a .cpp file), that
> would apply to a couple classes in this CL.

I talked to jochen@, and he tells me these should not be inline. Normally we
have a clang plugin [1] to check this, but I'm told it's not run for WebKit
code.

[1]:
https://www.chromium.org/developers/coding-style/chromium-style-checker-error...

[kpaulhamus, 2017-07-12 21:21:50]

Thanks for the detailed review!

https://codereview.chromium.org/2966523002/diff/20001/content/browser/webauth...
File content/browser/webauth/authenticator_impl.cc (right):

https://codereview.chromium.org/2966523002/diff/20001/content/browser/webauth...
content/browser/webauth/authenticator_impl.cc:20: const char
kMakeCredentialType[] = "navigator.id.makeCredential";
On 2017/07/05 14:36:12, vasilii wrote:
> The new trend is 'constexpr' wherever possible.

Done.

https://codereview.chromium.org/2966523002/diff/20001/content/browser/webauth...
File content/browser/webauth/authenticator_impl_unittest.cc (right):

https://codereview.chromium.org/2966523002/diff/20001/content/browser/webauth...
content/browser/webauth/authenticator_impl_unittest.cc:126:
opts->adjusted_timeout = base::TimeDelta::FromMilliseconds(60000);
On 2017/07/05 14:36:13, vasilii wrote:
> Why not FromSeconds?

Timeout changed from seconds to milliseconds in the spec. I'll use FromMinutes,
though, since we're just setting a test value.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/mac-mac10.11/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/mac-mac10.11/webexposed/global-interface-listing-expected.txt:6393:
method constructor
On 2017/07/03 08:22:06, Mike West wrote:
> I don't think we need this file at all? Can you avoid adding it? :)

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/mac-retina/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/mac-retina/webexposed/global-interface-listing-expected.txt:710:
setter z
On 2017/07/03 08:22:06, Mike West wrote:
> I don't think we need this file at all? Can you avoid adding it? :)

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/mac/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/mac/webexposed/global-interface-listing-expected.txt:710:
setter z
On 2017/07/03 08:22:06, Mike West wrote:
> Here too.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/win/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/win/webexposed/global-interface-listing-expected.txt:710:
setter z
On 2017/07/03 08:22:07, Mike West wrote:
> Some of this looks like a rebase gone wrong.

Rebase gone wrong indeed!

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/mojo-loading/webexposed/global-interface-listing-expected.txt
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/mojo-loading/webexposed/global-interface-listing-expected.txt:365:
interface AuthenticationAssertion
On 2017/07/03 08:22:07, Mike West wrote:
> I don't think we need this file at all? Can you avoid adding it? :)

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:25: 
On 2017/07/05 18:51:03, engedy wrote:
> optional nit: Do we want to supply a user-declared destructor here? I suppose
> this depends on the answer to the other question, i.e. whether we are okay
with
> having it inlined.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:28: }
On 2017/07/05 18:51:04, engedy wrote:
> nit: Add blank line.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:31:
DEFINE_INLINE_TRACE() {
On 2017/07/05 18:51:04, engedy wrote:
> nit: I think this should be DEFINE_INLINE_VIRTUAL_TRACE.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:32:
AuthenticatorResponse::Trace(visitor);
On 2017/07/05 18:51:04, engedy wrote:
> nit: The GC API Guidelines recommend putting the delegation call at the end of
> the trace method. See:
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:25:
AuthenticatorAttestationResponse(DOMArrayBuffer* client_data_json,
On 2017/07/05 18:51:04, engedy wrote:
> nit: Make constructor private if possible.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:30:
virtual ~AuthenticatorAttestationResponse() {}
On 2017/07/05 14:36:13, vasilii wrote:
> override

Actually don't think it's needed, took it out.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAttestationResponse.h:37:
AuthenticatorResponse::Trace(visitor);
On 2017/07/05 18:51:04, engedy wrote:
> nit: Put delegation call at the end here, as well in a couple other files.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:9: #include
"platform/bindings/ScriptWrappable.h"
On 2017/07/05 18:51:05, engedy wrote:
> Please add `#include "platform/heap/Handle.h"` in all files using
> GarbageCollected<> and friends.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:13: class
AuthenticatorResponse
On 2017/07/10 14:32:36, engedy wrote:
> On 2017/07/05 18:51:04, engedy wrote:
> > optional nit: I wonder if most of these classes should be marked
> MODULES_EXPORT?
> > @Mike, @Daniel?
> 
> Short answer: yes.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:30:
AuthenticatorResponse(DOMArrayBuffer* client_data_json)
On 2017/07/05 18:51:04, engedy wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:30:
AuthenticatorResponse(DOMArrayBuffer* client_data_json)
On 2017/07/05 18:51:04, engedy wrote:
> nit: explicit

K, I made impl files for AuthenticatorResponse,
AuthenticatorAttestationResponse, AuthenticatorAssertionResponse, and
PublicKeyKeyCredential. Take a look and see if that's what you had in mind.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/CollectedClientData.idl (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/CollectedClientData.idl:11: DOMString
tokenBindingId;
On 2017/07/05 18:51:05, engedy wrote:
> nit: Please add TODO(crbug.com/...) for extension fields.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/MakeCredentialOptions.idl
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/MakeCredentialOptions.idl:13:
sequence<PublicKeyCredentialDescriptor> excludeList;
On 2017/07/05 18:51:05, engedy wrote:
> s/excludeList/excludeCredentials/, also do we need the default value here? (=
> [];)

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/MakeCredentialOptions.idl:15: };
On 2017/07/05 18:51:05, engedy wrote:
> nit: Add TODO(crbug.com/...) for extension fields.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl:9:
SecureContext
On 2017/07/05 18:51:05, engedy wrote:
> Let's add Exposed=Window as well for clarity.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredential.idl:13: };
On 2017/07/05 18:51:05, engedy wrote:
> nit: Add TODO for |clientExtensionResults| field.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/PublicKeyCredentialParameters.idl
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialParameters.idl:15:
required AlgorithmIdentifier algorithm;
On 2017/07/05 18:51:05, engedy wrote:
> s/algorithm/alg/

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialRequestOptions.idl
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialRequestOptions.idl:11:
sequence <PublicKeyCredentialDescriptor> allowList;
On 2017/07/05 18:51:06, engedy wrote:
> s/allowList/allowCredentials/

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialRequestOptions.idl:11:
sequence <PublicKeyCredentialDescriptor> allowList;
On 2017/07/05 18:51:06, engedy wrote:
> Do we need the default value here, i.e. `allowCredentials = []`?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/PublicKeyCredentialRequestOptions.idl:12:
};
On 2017/07/05 18:51:06, engedy wrote:
> nit: Add TODO(crbug.com/...) for |extensions| field?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:216: void
WebAuthentication::ContextDestroyed(ExecutionContext*) {
On 2017/07/05 18:51:07, engedy wrote:
> Hmm, how come we don't need this anymore?

I was going to put it back, but it's a ContextLifecycleObserver override, which
we don't use anymore.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/07/05 18:51:07, engedy wrote:
> nit: Time machine? :)

:-)

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:12: #include
"core/dom/Document.h"
On 2017/07/05 18:51:07, engedy wrote:
> nit: Will be unneeded once the ContextLifecycleObserver is gone.

It's still needed for GetFrame.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:17: #include
"modules/webauth/PublicKeyCredential.h"
On 2017/07/05 18:51:07, engedy wrote:
> nit: AuthenticatorAssertionResponse and PublicKeyCredential look unused?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:24: const
int64_t kAdjustedTimeoutUpperInMilliseconds = 12000;
On 2017/07/05 14:36:13, vasilii wrote:
> constexpr

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:24: const
int64_t kAdjustedTimeoutUpperInMilliseconds = 12000;
On 2017/07/05 18:51:08, engedy wrote:
> Missing trailing 0? i.e. 12 sec instead of 2 minutes?

good catch

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:24: const
int64_t kAdjustedTimeoutUpperInMilliseconds = 12000;
On 2017/07/05 18:51:07, engedy wrote:
> On 2017/07/05 14:36:13, vasilii wrote:
> > constexpr
> 
> Also, could this directly be `constexpr WTF::TimeDelta`? That would allow
using
> much shorter names, using FromMinutes, and not including stdint.h.

Yep, that's a good idea.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:43:
vector.Append(static_cast<uint8_t*>(
On 2017/07/05 18:51:06, engedy wrote:
> nit: DCHECK(buffer.isArrayBufferView());

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:52: const
String& cred_type) {
On 2017/07/05 18:51:06, engedy wrote:
> nit: Let's avoid abbreviations, and make this either credential_type or type.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:116: if
(parameter.hasType()) {
On 2017/07/05 18:51:07, engedy wrote:
> nit: I'm not very familiar with this, but I wonder if this can really be
false,
> given everything is `required` in the IDLs?

Good point.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:123: int64_t
predicted_timeout;
On 2017/07/05 18:51:06, engedy wrote:
> nit: What does predicted mean in this context? I don't find a mention to it in
> the spec.

It's essentially a temp variable. I'll change the name so it's not confusing.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:130:
mojo_options->adjusted_timeout = WTF::TimeDelta::FromMilliseconds(std::max(
On 2017/07/05 18:51:07, engedy wrote:
> nit: Can we use WTF::clampTo here?

I didn't know about clampTo, that's nifty. I tried using it, but it threw errors
over WTF::TimeDelta types. I think it's an either-or situation between clampTo
and using WTF::TimeDelta for the kAdjustedTimeout* types. The max/min comparison
as-is seems fine to me?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:176: }
On 2017/07/05 18:51:07, engedy wrote:
> nit: Add blank line after

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:205:
ScriptPromise promise = resolver->Promise();
On 2017/07/05 18:51:07, engedy wrote:
> nit: Do we need to make this call here because of side-effects of calling
> Promise()? If so, could you please add a comment to this effect?

Could you clarify what kind of comment you mean? This is how promises are
conventionally returned. I could just "return resolver->Promise()" at the end
like before, if that would be clearer.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:259:
ScriptPromise WebAuthentication::RejectIfNotSupported(
On 2017/07/05 18:51:06, engedy wrote:
> Looks like this is not used anymore?

Ah, it's supposed to be used. Bad merge/rebase.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:8: #include
"bindings/core/v8/ArrayBufferOrArrayBufferView.h"
On 2017/07/05 18:51:08, engedy wrote:
> nit: No longer used in the header, move to .cpp.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:9: #include
"bindings/core/v8/ScriptPromise.h"
On 2017/07/05 18:51:08, engedy wrote:
> nit: I think the complete type is not needed in the header.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:11: #include
"core/dom/DOMArrayBuffer.h"
On 2017/07/05 18:51:08, engedy wrote:
> nit: No longer used in the header, move to .cpp.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:12: #include
"modules/webauth/MakeCredentialOptions.h"
On 2017/07/05 18:51:08, engedy wrote:
> nit: Fwd-declaration suffices in the header, move to .cpp.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:23: typedef
ArrayBufferOrArrayBufferView BufferSource;
On 2017/07/05 18:51:08, engedy wrote:
> nit: No longer used in the header, move to .cpp.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:28: public
ContextLifecycleObserver {
On 2017/07/05 18:51:08, engedy wrote:
> nit: No longer used, including its header.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:40: ScriptPromise
makeCredential(ScriptState*, MakeCredentialOptions&);
On 2017/07/05 18:51:08, engedy wrote:
> nit: Can the second argument be const too here?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:49: void
OnMakeCredential(ScriptPromiseResolver*,
On 2017/07/05 18:51:08, engedy wrote:
> nit: Do these need to be public?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.h:49: void
OnMakeCredential(ScriptPromiseResolver*,
On 2017/07/05 18:51:08, engedy wrote:
> nit: Need fwd-declaration for class ScriptPromiseResolver.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.idl (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.idl:5: //
https://w3c.github.io/webauthn/#webauthentication
On 2017/07/05 18:51:09, engedy wrote:
> nit: Note that this link is no longer valid. Maybe remove, add TODO, or add
new
> link to CM API?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.idl:11:
[CallWith=ScriptState] Promise<sequence<AuthenticatorAttestationResponse>>
makeCredential(MakeCredentialOptions publicKey);
On 2017/07/05 18:51:08, engedy wrote:
> nit: Maybe I'm missing something, but the new spec does not seem to support
> returning multiple results. WDYT about removing the sequence<...> references
> here already in this CL?

Sure

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/webauth/authenticator.mojom
(left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:93: //
[ScopedCredentialInfo] will only be set if status == SUCCESS.
Thx

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/webauth/authenticator.mojom
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:29: //
The base64url encoding of rawId.
On 2017/07/05 18:51:09, engedy wrote:
> nit: ... of |raw_id|.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:32:
array<uint8> rawId;
On 2017/06/30 16:23:55, dcheng1 wrote:
> Nit: raw_id

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:41: //
TODO(kpaulhamus): add AuthenticatorSelectionCriteria
On 2017/07/05 18:51:09, engedy wrote:
> nit: I think this goes to MakeCredentialOptions?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:46:
struct AuthenticatorResponse {
On 2017/07/05 18:51:09, engedy wrote:
> If I understand correctly, this needs to be polymorphic as per the WebAuthN
> spec. How is the receiver end going to decide on the subtype here?

The call being made (create/get, or as-currently makeCredential/getAssertion)
informs the return type. i.e. makeCredential always wants an "Attestation"
response, and would just need to verify that there is an attestation_object
returned in this AuthenticatorResponse. I  mashed them together for convenience
in the mojo, since mojo technically doesn't have to match the formal spec.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:55: //
Information about the relying party and the user account held by that
On 2017/07/05 18:51:09, engedy wrote:
> nit: Not sure about the semantics, but how about "the account held by the user
> at that relying party"?

Sure

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:84: //
Optional parameters that are used during makeCredential.
On 2017/07/05 18:51:09, engedy wrote:
> comment nit: These do not seem to be optional anymore. How about:
> 
> // Parameters passed into calls to MakeCredential.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:86:
PublicKeyCredentialEntity relying_party; // Relying party information.
On 2017/07/05 18:51:09, engedy wrote:
> style nit: Could you please move the comments above the members, and leave one
> blank line in-between members throughout this file?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:86:
PublicKeyCredentialEntity relying_party; // Relying party information.
On 2017/07/05 18:51:09, engedy wrote:
> nit: Mention this corresponds to |rp| in the IDL.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:91:
array<PublicKeyCredentialDescriptor> exclude_list;
On 2017/07/05 18:51:09, engedy wrote:
> optional nit: s/exlude_list/excludeCredentials/, for consistency with IDL.

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:95:
PUBLICKEY,
On 2017/07/05 18:51:10, engedy wrote:
> nit: I'm never sure about converting dashes to ALL_CAPS, but would PUBLIC_KEY
> look better?

Done.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:109: enum
Transport {
On 2017/07/05 18:51:09, engedy wrote:
> nit: AuthenticatorTransport

Done.

[engedy, 2017-07-13 11:33:54]

Still LGTM % nits, thank you!

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorResponse.h:30:
AuthenticatorResponse(DOMArrayBuffer* client_data_json)
On 2017/07/12 21:21:46, kpaulhamus wrote:
> On 2017/07/05 18:51:04, engedy wrote:
> > nit: explicit
> 
> K, I made impl files for AuthenticatorResponse,
> AuthenticatorAttestationResponse, AuthenticatorAssertionResponse, and
> PublicKeyKeyCredential. Take a look and see if that's what you had in mind.
>

Yep, that's exactly it, thanks!

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:216: void
WebAuthentication::ContextDestroyed(ExecutionContext*) {
On 2017/07/12 21:21:48, kpaulhamus wrote:
> On 2017/07/05 18:51:07, engedy wrote:
> > Hmm, how come we don't need this anymore?
> 
> I was going to put it back, but it's a ContextLifecycleObserver override,
which
> we don't use anymore.

Could you please explain to me why we don't need it anymore? I don't see the
lifetime management changes. What am I missing?

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:12: #include
"core/dom/Document.h"
On 2017/07/12 21:21:48, kpaulhamus wrote:
> On 2017/07/05 18:51:07, engedy wrote:
> > nit: Will be unneeded once the ContextLifecycleObserver is gone.
> 
> It's still needed for GetFrame.

Acknowledged.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:123: int64_t
predicted_timeout;
On 2017/07/12 21:21:48, kpaulhamus wrote:
> On 2017/07/05 18:51:06, engedy wrote:
> > nit: What does predicted mean in this context? I don't find a mention to it
in
> > the spec.
> 
> It's essentially a temp variable. I'll change the name so it's not confusing.

Much clearer, thank you!

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:130:
mojo_options->adjusted_timeout = WTF::TimeDelta::FromMilliseconds(std::max(
On 2017/07/12 21:21:47, kpaulhamus wrote:
> On 2017/07/05 18:51:07, engedy wrote:
> > nit: Can we use WTF::clampTo here?
> 
> I didn't know about clampTo, that's nifty. I tried using it, but it threw
errors
> over WTF::TimeDelta types. I think it's an either-or situation between clampTo
> and using WTF::TimeDelta for the kAdjustedTimeout* types. The max/min
comparison
> as-is seems fine to me?

That's a shame. And yes, I agree, using WTF::TimeDelta trumps using clampTo, so
let's stick to min/max comparison. Ack.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:205:
ScriptPromise promise = resolver->Promise();
On 2017/07/12 21:21:48, kpaulhamus wrote:
> On 2017/07/05 18:51:07, engedy wrote:
> > nit: Do we need to make this call here because of side-effects of calling
> > Promise()? If so, could you please add a comment to this effect?
> 
> Could you clarify what kind of comment you mean? This is how promises are
> conventionally returned. I could just "return resolver->Promise()" at the end
> like before, if that would be clearer.

Sorry for the ambiguous phrasing. I was indeed just wondering why we don't just
call resolver->Promise() in the return statement, as you are doing now. Ack.

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/webauth/authenticator.mojom
(right):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/webauth/authenticator.mojom:46:
struct AuthenticatorResponse {
On 2017/07/12 21:21:49, kpaulhamus wrote:
> On 2017/07/05 18:51:09, engedy wrote:
> > If I understand correctly, this needs to be polymorphic as per the WebAuthN
> > spec. How is the receiver end going to decide on the subtype here?
> 
> The call being made (create/get, or as-currently makeCredential/getAssertion)
> informs the return type. i.e. makeCredential always wants an "Attestation"
> response, and would just need to verify that there is an attestation_object
> returned in this AuthenticatorResponse. I  mashed them together for
convenience
> in the mojo, since mojo technically doesn't have to match the formal spec.

Got it.

https://codereview.chromium.org/2966523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
nit: Let's make this 2017 at least for `actual` new files that are introducing
new classes.

[kpaulhamus, 2017-07-13 14:07:32]

The CQ bit was checked by kpaulhamus@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-13 14:07:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-13 15:14:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-13 15:14:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kpaulhamus, 2017-07-13 16:00:53]

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:216: void
WebAuthentication::ContextDestroyed(ExecutionContext*) {
On 2017/07/13 11:33:53, engedy wrote:
> On 2017/07/12 21:21:48, kpaulhamus wrote:
> > On 2017/07/05 18:51:07, engedy wrote:
> > > Hmm, how come we don't need this anymore?
> > 
> > I was going to put it back, but it's a ContextLifecycleObserver override,
> which
> > we don't use anymore.
> 
> Could you please explain to me why we don't need it anymore? I don't see the
> lifetime management changes. What am I missing?

Yeah, in WebAuthentication.h/.cpp in Patch 2, you had comments about not needing
ContextLifecycleObserver. Once I stripped that out, it made no sense to have
ContextDestroyed(). It certainly doesn't matter once I move this stuff to
CredMan because WebAuthentication will stop controlling the lifecycle of the
blink-layer implementation anyways.

https://codereview.chromium.org/2966523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h
(right):

https://codereview.chromium.org/2966523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/AuthenticatorAssertionResponse.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/07/13 11:33:54, engedy wrote:
> nit: Let's make this 2017 at least for `actual` new files that are introducing
> new classes.

Done.

[engedy, 2017-07-13 20:27:13]

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (left):

https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:216: void
WebAuthentication::ContextDestroyed(ExecutionContext*) {
On 2017/07/13 16:00:53, kpaulhamus wrote:
> On 2017/07/13 11:33:53, engedy wrote:
> > On 2017/07/12 21:21:48, kpaulhamus wrote:
> > > On 2017/07/05 18:51:07, engedy wrote:
> > > > Hmm, how come we don't need this anymore?
> > > 
> > > I was going to put it back, but it's a ContextLifecycleObserver override,
> > which
> > > we don't use anymore.
> > 
> > Could you please explain to me why we don't need it anymore? I don't see the
> > lifetime management changes. What am I missing?
> 
> Yeah, in WebAuthentication.h/.cpp in Patch 2, you had comments about not
needing
> ContextLifecycleObserver. Once I stripped that out, it made no sense to have
> ContextDestroyed(). It certainly doesn't matter once I move this stuff to
> CredMan because WebAuthentication will stop controlling the lifecycle of the
> blink-layer implementation anyways.

Sorry, misunderstanding. I suggested removing only because it seemed it was not
used anymore, but if we needed it, then we still need it. Could you please
re-add it?

[kpaulhamus, 2017-07-13 20:29:53]

On 2017/07/13 20:27:13, engedy wrote:
>
https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (left):
> 
>
https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:216: void
> WebAuthentication::ContextDestroyed(ExecutionContext*) {
> On 2017/07/13 16:00:53, kpaulhamus wrote:
> > On 2017/07/13 11:33:53, engedy wrote:
> > > On 2017/07/12 21:21:48, kpaulhamus wrote:
> > > > On 2017/07/05 18:51:07, engedy wrote:
> > > > > Hmm, how come we don't need this anymore?
> > > > 
> > > > I was going to put it back, but it's a ContextLifecycleObserver
override,
> > > which
> > > > we don't use anymore.
> > > 
> > > Could you please explain to me why we don't need it anymore? I don't see
the
> > > lifetime management changes. What am I missing?
> > 
> > Yeah, in WebAuthentication.h/.cpp in Patch 2, you had comments about not
> needing
> > ContextLifecycleObserver. Once I stripped that out, it made no sense to have
> > ContextDestroyed(). It certainly doesn't matter once I move this stuff to
> > CredMan because WebAuthentication will stop controlling the lifecycle of the
> > blink-layer implementation anyways.
> 
> Sorry, misunderstanding. I suggested removing only because it seemed it was
not
> used anymore, but if we needed it, then we still need it. Could you please
> re-add it?

Ah, got it. Sure, will do.

Hey, do you know what's going on with the layouttests? I can't tell if it's just
flakiness, if I simply need to rebaseline, or if I'm doing something wrong.
It looks like a lot of tests are timing out, but most are not WebAuthN related.

I went to rebaseline them and the tool added a lot of odd "expected" files, ex:
LayoutTests/platform/mac-retina/webexposed/global-interface-listing-expected.txt
LayoutTests/platform/linux/webexposed/global-interface-listing-expected.txt
LayoutTests/platform/linux/virtual/mojo-loading/http/tests/inspector/extensions/extensions-panel-expected.txt
LayoutTests/platform/linux/http/tests/security/subresourceIntegrity/shared-with-xhtml-expected.txt
LayoutTests/platform/mac/external/wpt/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/moving-documents-expected.txt

It didn't quite seem right. Any ideas?

[engedy, 2017-07-13 21:00:10]

On 2017/07/13 20:29:53, kpaulhamus wrote:
> On 2017/07/13 20:27:13, engedy wrote:
> >
>
https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (left):
> > 
> >
>
https://codereview.chromium.org/2966523002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:216: void
> > WebAuthentication::ContextDestroyed(ExecutionContext*) {
> > On 2017/07/13 16:00:53, kpaulhamus wrote:
> > > On 2017/07/13 11:33:53, engedy wrote:
> > > > On 2017/07/12 21:21:48, kpaulhamus wrote:
> > > > > On 2017/07/05 18:51:07, engedy wrote:
> > > > > > Hmm, how come we don't need this anymore?
> > > > > 
> > > > > I was going to put it back, but it's a ContextLifecycleObserver
> override,
> > > > which
> > > > > we don't use anymore.
> > > > 
> > > > Could you please explain to me why we don't need it anymore? I don't see
> the
> > > > lifetime management changes. What am I missing?
> > > 
> > > Yeah, in WebAuthentication.h/.cpp in Patch 2, you had comments about not
> > needing
> > > ContextLifecycleObserver. Once I stripped that out, it made no sense to
have
> > > ContextDestroyed(). It certainly doesn't matter once I move this stuff to
> > > CredMan because WebAuthentication will stop controlling the lifecycle of
the
> > > blink-layer implementation anyways.
> > 
> > Sorry, misunderstanding. I suggested removing only because it seemed it was
> not
> > used anymore, but if we needed it, then we still need it. Could you please
> > re-add it?
> 
> Ah, got it. Sure, will do.
> 
> Hey, do you know what's going on with the layouttests? I can't tell if it's
just
> flakiness, if I simply need to rebaseline, or if I'm doing something wrong.
> It looks like a lot of tests are timing out, but most are not WebAuthN
related.
> 
> I went to rebaseline them and the tool added a lot of odd "expected" files,
ex:
>
LayoutTests/platform/mac-retina/webexposed/global-interface-listing-expected.txt
> LayoutTests/platform/linux/webexposed/global-interface-listing-expected.txt
>
LayoutTests/platform/linux/virtual/mojo-loading/http/tests/inspector/extensions/extensions-panel-expected.txt
>
LayoutTests/platform/linux/http/tests/security/subresourceIntegrity/shared-with-xhtml-expected.txt
>
LayoutTests/platform/mac/external/wpt/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/moving-documents-expected.txt
> 
> It didn't quite seem right. Any ideas?

The `webkit-patch` tool and I aren't very good friends either, but some of those
tests seem to have flaked on the tryjobs, others are marked in various
TestExpectations files, so I'd say just rebaseline the global-interface-listing
tests and have another go at the tryjobs.

Some trybots are pretty red too (e.g.
https://build.chromium.org/p/tryserver.blink/builders/win10_blink_rel), other
try jobs are failing the same layout tests yours are.

[kpaulhamus, 2017-07-14 00:38:55]

The CQ bit was checked by kpaulhamus@chromium.org

[kpaulhamus, 2017-07-14 00:38:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, mkwst@chromium.org, engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/2966523002/#ps120001
(title: "Add ContectLifecycleObserver... part2")

[commit-bot: I haz the power, 2017-07-14 00:39:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-14 02:52:58]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499992735720180,
"parent_rev": "c23f8d441fb8c5924fbfa0fb32742a664d2c33cb", "commit_rev":
"04b66da848df095ca21d54194b7c59b0d01526ab"}

[commit-bot: I haz the power, 2017-07-14 02:53:10]

Description was changed from

==========
Blink-layer update to match WebAuthN spec

1 of many CLs to merge WebAuthN with the Cred Man API. This change
renames idl components and updates the corresponding idl test.
(This changes as much as possible short of depending on anything in
credential manager).

BUG=664630
==========

to

==========
Blink-layer update to match WebAuthN spec

1 of many CLs to merge WebAuthN with the Cred Man API. This change
renames idl components and updates the corresponding idl test.
(This changes as much as possible short of depending on anything in
credential manager).

BUG=664630

Review-Url: https://codereview.chromium.org/2966523002
Cr-Commit-Position: refs/heads/master@{#486648}
Committed:
https://chromium.googlesource.com/chromium/src/+/04b66da848df095ca21d54194b7c...
==========

[commit-bot: I haz the power, 2017-07-14 02:53:12]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/04b66da848df095ca21d54194b7c...