[webkit-reviews] review denied: [Bug 57195] Include certificate when sending a WebCore::ResourceError to UI process on Windows : [Attachment 87092] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Jeff Miller
<jeffm at apple.com>'s request for review:
Bug 57195: Include certificate when sending a WebCore::ResourceError to UI
process on Windows
https://bugs.webkit.org/show_bug.cgi?id=57195

Attachment 87092: Patch
https://bugs.webkit.org/attachment.cgi?id=87092&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87092&action=review

Seems worth another go-round. But this looks great!

> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:142
> +    CFErrorRef cfError = resourceError.cfError();
> +    if (!cfError) {
> +	   encoder->encode(WebKit::PlatformCertificateInfo());
> +	   return;
> +    }
> +    
> +    RetainPtr<CFDictionaryRef> userInfo(AdoptCF,
CFErrorCopyUserInfo(cfError));
> +    if (!userInfo) {
> +	   encoder->encode(WebKit::PlatformCertificateInfo());
> +	   return;
> +    }
> +    
> +    PCCERT_CONTEXT certificate =
static_cast<PCCERT_CONTEXT>(wkGetSSLPeerCertificateDataPtr(userInfo.get()));
> +    if (!certificate) {
> +	   encoder->encode(WebKit::PlatformCertificateInfo());
> +	   return;
> +    }

Why can't we just use ResourceError::m_certificate here?

> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:152
> +	   CertFreeCertificateContext(reinterpret_cast<PCCERT_CONTEXT>(ptr));

static_cast should work fine.

> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:158
> +    PCCERT_CONTEXT certCopy = 0;

You can wait to declare this until you initialize it on line 165.

> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:164
> +    if (!certDealloc) {
> +	   CFAllocatorContext allocContext = {
> +	       0, 0, 0, 0, 0, 0, 0, deallocCertContext, 0
> +	   };
> +	   certDealloc = CFAllocatorCreate(kCFAllocatorDefault, &allocContext);

> +    }

Having a separate createCertContextDeallocator function would clean this up a
bit. Then you could just assign its result into the certDealloc variable and
not need an if.

> Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:187
> +	   resourceError = WebCore::ResourceError(domain, errorCode,
failingURL, localizedDescription, copyCert(certificateChain[0]));

I personally prefer .first() instead of [0].