[webkit-reviews] review granted: [Bug 237454] [WebAuthn] Provide [ASCAgent performAutoFillAuthorizationRequestsForContext] SPI with frameID : [Attachment 453802] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted j_pascoe at apple.com
<j_pascoe at apple.com>'s request for review:
Bug 237454: [WebAuthn] Provide [ASCAgent
performAutoFillAuthorizationRequestsForContext] SPI with frameID
https://bugs.webkit.org/show_bug.cgi?id=237454

Attachment 453802: Patch

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




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

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

r=me, but I think the SPI might benefit from some tweaks.

> Source/WebKit/Platform/spi/Cocoa/AuthenticationServicesCoreSPI.h:255
> + at property (nonatomic, nullable, copy) NSNumber *webFrameID;

Nit: Given that GlobalFrameIdentifier is a struct that holds a pageID and a
frameID, I wonder if this SPI would have been better designed using a similar
struct.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:239
> +static RetainPtr<ASCCredentialRequestContext>
configurationAssertionRequestContext(const PublicKeyCredentialRequestOptions&
options, Vector<uint8_t> hash, std::optional<WebCore::MediationRequirement>
mediation, std::optional<WebCore::GlobalFrameIdentifier> frameID)

Nit: I suggest 'globalFrameID' (rather than frameID) to distinguish between the
"frame-only" ID used in some code, versus this pair of frame and page.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:265
> +    if (frameID && [requestContext
respondsToSelector:@selector(setWebFrameID:)] && [requestContext
respondsToSelector:@selector(setWebPageID:)]) {

It would be nice to remember to remove these 'respondsToSelector' checks in the
future, as they do incur a cost. I also wonder if it's ever possible for a
version of the OS that responds to 'setWebFrameID' wouldn't also respond to
'setWebPageID'.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProx
y.mm:309
> +	   result = configurationAssertionRequestContext(options,
requestData.hash, requestData.mediation, requestData.frameID);

I really dislike the 'frameID' accessor for this GlobalFrameID (which is a pair
of Page/Frame). But that's not due to this change.