[webkit-reviews] review denied: [Bug 225519] [Cocoa] _WKAuthenticatorAssertionResponse should specify the attachment type used : [Attachment 430050] Rebased patch with fixed tests

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Garrett Davidson
<garrett_davidson at apple.com>'s request for review:
Bug 225519: [Cocoa] _WKAuthenticatorAssertionResponse should specify the
attachment type used
https://bugs.webkit.org/show_bug.cgi?id=225519

Attachment 430050: Rebased patch with fixed tests

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




--- Comment #13 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 430050
  --> https://bugs.webkit.org/attachment.cgi?id=430050
Rebased patch with fixed tests

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

> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:67
> +    AuthenticatorAttachment m_attachment;

Since we're storing this, this would probably be a good time to reduce the
size.  Replace "enum class AuthenticatorAttachment" with "enum class
AuthenticatorAttachment : bool", and you can also remove the EnumTraits in
AuthenticatorAttachment.h

>
Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:
75
> +	       handler({ }, (AuthenticatorAttachment)0, exception);

This is actually AuthenticatorAttachment::Platform.  Is that what you want? 
Maybe you want Optional<AuthenticatorAttachment>

> Source/WebKit/WebAuthnProcess/WebAuthnConnectionToWebProcess.cpp:87
> +	       handler({ }, (AuthenticatorAttachment)0, exception);

ditto

> Tools/ChangeLog:1
> +2021-05-28  Garrett Davidson  <davidson.garrettm at gmail.com>

Is this the email address you want to use?

> Tools/ChangeLog:11
> +	   Update the CTAP tests to specify the new attachment parameter. All
of these tests
> +	   assume a cross platform authenticator.

Is it possible to add a test that uses AuthenticatorAttachment::Platform?