[webkit-reviews] review denied: [Bug 218893] [WebAuthn] Implement SPI for AuthenticationServices.Framework : [Attachment 414640] 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 218893: [WebAuthn] Implement SPI for AuthenticationServices.Framework
https://bugs.webkit.org/show_bug.cgi?id=218893

Attachment 414640: Patch

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




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

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:31
> +// FIXME: <rdar://problem/71509848> Separate the following different
interfaces into separate files.

I'm still not crazy about having a bunch of little objects that don't have
their own header land in the repository.  Especially little objects that use
ObjC-generated ivars unlike the rest of the API objects in WebKit.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:192
> +- (instancetype)init NS_UNAVAILABLE;

new should also be NS_UNAVAILABLE here.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:270
> +// FIXME: <rdar://problem/71509848> Remove the following deprecated method.
> + at property (nonatomic, readonly, copy) NSString *relyingPartyID;

I think these should be annotated as deprecated with this change.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:337
> +static void
encodeEntity(WebCore::PublicKeyCredentialCreationOptions::Entity& output,
_WKPublicKeyCredentialEntity* input)

This looks a lot like returning by reference, but it's not.  I think it would
be better to just have these two lines of code duplicated for the two different
types of entities here.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:447
> +    return result;

Would it be possible to return { attachment, key, verification }; instead of
constructing an object, filling it in, and then returning it? Making this
change would cause a compiler failure if we add something in the future and
forget to update, and it also doesn't require default constructor to be called.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:512
> +	   }, [&](const WebCore::ExceptionData& exception) {
> +	       handler(nil, [NSError errorWithDomain:WKErrorDomain
code:WKErrorUnknown userInfo:nil]);

Can we use the exception data instead of just reporting an unknown error?