[webkit-reviews] review granted: [Bug 190783] [WebAuthn] Combine AuthenticatorResponse and PublicKeyCredentialData : [Attachment 385239] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 190783: [WebAuthn] Combine AuthenticatorResponse and
PublicKeyCredentialData
https://bugs.webkit.org/show_bug.cgi?id=190783

Attachment 385239: Patch

https://bugs.webkit.org/attachment.cgi?id=385239&action=review




--- Comment #5 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 385239
  --> https://bugs.webkit.org/attachment.cgi?id=385239
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385239&action=review

Looks good! r=me

> Source/WebCore/ChangeLog:12
> +	   all different type of responses from authenticators anymore. For
example, authenticatorGetNextAssertion

all THE different

> Source/WebCore/ChangeLog:13
> +	   depends on numberOfCredentials member from authenticatorGetAssertion
response to function, but

depends on THE numberOfCredentials ...

> Source/WebCore/ChangeLog:14
> +	   numberOfCredentials is not used anywhere else. Therefore, a
polymorphism type is needed to

Therefore, a POLYMORPHIC type is needed to ...

> Source/WebCore/ChangeLog:19
> +	   IPC. To solve this, AuthenticatorResponseData
(PublicKeyCredentialData) is kept to be an intermediate type

is kept AS an intermediate type ...

> Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientOutputs.h:30
> +#include <wtf/text/WTFString.h>

Is WTFString actually needed here? No string appears in the header.

Seems more likely we would need <wtf/Optional.h>

> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:45
> +    RefPtr<ArrayBuffer> userhandle;

I don't think it's wise to rely on case difference to distinguish between
'userhandle' and 'userHandle'.

I suggest 'userHandleData' and 'userHandle'.

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:55
> +// FIXME: Find a way to reduce the unnecessary copies.

You should file a bugzilla or we will never remember to do this!

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:55
> +    void setClientDataJSON(Ref<ArrayBuffer>&&);

Why isn't this inlined like 'setExtensions'? Seems like we should be consistent
(all in the implementation file, or all in the header).

> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2019 maybe?