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

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




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

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:270
> + at interface _WKWebAuthenticationPanel (WKDeprecated)

Instead of this, use WK_API_DEPRECATED or WK_API_DEPRECATED_WITH_REPLACEMENT.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:51
> +    _appid = appid;

All the other objects in our ObjC API (with a few exceptions that we should
fix) are implemented by C++ objects.  This is implemented by copying all the
C++ properties into ivars.  We should do the former instead.
Initializers should call API::Object::constructInWrapper
dealloc should call C++ destructor.
You'll need new API object types in APIObject.h and new calls to alloc in
APIObject.mm
You'll need an API::ObjectStorage "ivar" with @package.
You'll need to define WrapperTraits for the API object types.
You'll probably need an *Internal.h header.  Also, it's a good pattern to have
one class per header instead of cramming them all into one like this is now.

WKNavigationAction is a good example of this pattern.

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

Memory leak.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:536
> +	   extensions = [[_WKAuthenticationExtensionsClientOutputs alloc]
initWithAppid:data.appid.value()];

Memory leak.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:542
> +    _WKAuthenticatorAssertionResponse *result =
[[_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];

Memory leak.