[webkit-reviews] review denied: [Bug 218893] [WebAuthn] Implement SPI for AuthenticationServices.Framework : [Attachment 414496] 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 414496: Patch

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




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

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

> Source/WebKit/ChangeLog:15
> +	   is the change of ownerships. Priori to this change,
AuthenticatorManager owns the APIWebAuthenticationPanel.

Prior

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:45
> +    : m_manager(WTF::makeUnique<AuthenticatorManager>())

I think WTF:: is unnecessary here.

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:68
> +    // FIXME<rdar://problem/71509848>: Remove the folowing deprecated
methods.

Space and colon before <
// FIXME: <rdar:...
Also, is there something that should be done before these should be removed? 
Why don't we just remove them in this patch?

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.h:79
> +    std::unique_ptr<WebKit::AuthenticatorManager> m_manager; // FIXME():
Change to UniqueRef.

Why don't we just do this now?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:85
> +    _WKPublicKeyCredentialTypePublicKey,

I don't think this one-value enum is useful unless there are plans to add to
it.

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

new should also be NS_UNAVAILABLE

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:154
> + at property (nonatomic, copy) NSNumber *alg;

This should have a better name.
"algorithm"?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:162
> + at property (nonatomic) BOOL requireResidentKey; // Default to NO.

If this comment is necessary, please use header doc style like in our public
API headers.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:173
> + at property (nonatomic) _WKPublicKeyCredentialType type;

I feel like a lot of these should be readonly.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:182
> + at property (nullable, nonatomic, copy) NSString *appid;

What is this?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:259
> +// FIXME: <rdar://problem/71509485>

Please add some comment here or remove this.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:328
> +    result.append((const uint8_t*)[data bytes], [data length]);

Dot syntax?

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:496
> +    return [[_WKAuthenticatorAttestationResponse alloc]
initWithRawId:[NSData dataWithBytes:data.rawId->data()
length:data.rawId->byteLength()] extensions:nil attestationObject:[NSData
dataWithBytes:data.attestationObject->data()
length:data.attestationObject->byteLength()]];

This looks like a memory leak.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:543
> +    return [[_WKAuthenticatorAssertionResponse alloc] initWithRawId:[NSData
dataWithBytes:data.rawId->data() length:data.rawId->byteLength()]
extensions:nil authenticatorData:[NSData
dataWithBytes:data.authenticatorData->data()
length:data.authenticatorData->byteLength()] signature:[NSData
dataWithBytes:data.signature->data() length:data.signature->byteLength()]
userHandle:userHandle];

ditto

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanelForTesting.h:39
> +// For testing only.

The name of this header makes this comment unnecessary.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:1724
> +    _WKAuthenticationExtensionsClientInputs *extensions =
[[_WKAuthenticationExtensionsClientInputs alloc] init];

Let's not leak memory, even in tests.  This doesn't use ARC yet.