[webkit-reviews] review denied: [Bug 185848] Adopt SecKeyProxy SPI in certificate based challenge response code : [Attachment 341058] 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 185848: Adopt SecKeyProxy SPI in certificate based challenge response code
https://bugs.webkit.org/show_bug.cgi?id=185848

Attachment 341058: Patch

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




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

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

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:46
> +    // The following xpc event handler overwrites the boostrap event handler
and is only used
> +    // to capture client certificate credential.
> +    xpc_connection_set_event_handler(connection->xpcConnection(),
^(xpc_object_t event) {

Will this intercept any other events?

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:50
> +	   if (type != XPC_TYPE_ERROR && weakThis) {

This could be an early return instead of indenting everything.

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:64
> +	       {

I don't think these additional scopes add much.

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68
> +		   identity = [SecKeyProxy
createIdentityFromEndpoint:endPoint.get() error:&error];

This needs to be adopted.  It's a memory leak as it stands now.

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:69
> +		   if (error) {

if (!identity || error)

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:81
> +		   auto total = xpc_array_get_count(certificateDataArray);
> +		   if (total) {

You can declare total inside the if statement.
if (auto total = ...)

> Source/WebKit/UIProcess/WebPageProxy.cpp:6211
> +	   auto secKeyProxyStore = SecKeyProxyStore::create();
> +	   authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore);

I still don't like this design.  Let's make SecKeyProxyStore::create take a
const WebCore::Credential& parameter and create it where we call
m_secKeyProxyStore->initialize instead of creating it here without a purpose
yet.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:182
> +    void addSecKeyProxyStore(RefPtr<SecKeyProxyStore>&&);

This should be a Ref.