[webkit-reviews] review denied: [Bug 206112] [WebAuthn] Implement SPI to tell UI clients to select assertion responses : [Attachment 387555] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 206112: [WebAuthn] Implement SPI to tell UI clients to select assertion
responses
https://bugs.webkit.org/show_bug.cgi?id=206112

Attachment 387555: Patch

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




--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 387555
  --> https://bugs.webkit.org/attachment.cgi?id=387555
Patch

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

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.h:40
> +    WTF::String name() const { return m_response->name(); }

const WTF::String&
This requires a change in WebCore, too.

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.h:44
> +    const Ref<WebCore::AuthenticatorAssertionResponse>& response() { return
m_response; }

Can you make this a const WebCore::AuthenticatorAssertionResponse& ?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationAssertionResponse.mm:30
> +    RetainPtr<NSData> _userHandle;

This shouldn't be here.  If you make
WebAuthenticationAssertionResponse::userHandle return an API::Data, you can
just return wrapper of that.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm
:155
> +    // Call delegates in the next run loop to prevent clients' reentrance
that would potentially modify the state
> +    // of the current run loop in unexpected ways.
> +    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this,
nsResponses = WTFMove(nsResponses), completionHandler =
WTFMove(completionHandler)] () mutable {

This shouldn't be necessary, and if it is then it shouldn't be in the API
layer.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm
:157
> +	      
completionHandler(static_cast<API::WebAuthenticationAssertionResponse&>([[nsRes
ponses firstObject] _apiObject]).response());

Will the set of responses ever be empty?  I see no such guarantee.