[webkit-reviews] review granted: [Bug 28171] [cURL] Support https protocol in cURL builds : [Attachment 34603] Revised to improve efficiency and thread safety.

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 28171: [cURL] Support https protocol in cURL builds
https://bugs.webkit.org/show_bug.cgi?id=28171

Attachment 34603: Revised to improve efficiency and thread safety.
https://bugs.webkit.org/attachment.cgi?id=34603&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +static CString certificateBundlePath()

"certificateBundlePath" is a little confusing. This doesn't return a path to a
"certificate bundle"; it returns a path to a certificate within the WebKit
bundle. I think "certificatePath" would be clearer.

> +{
> +#if PLATFORM(CF)
> +    CFBundleRef webKitBundle =
CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebKit"));
> +    RetainPtr<CFURLRef> certURLRef(AdoptCF,
CFBundleCopyResourceURL(webKitBundle, CFSTR("cacert"), CFSTR("pem"),
CFSTR("certificates")));
> +    if (certURLRef) {
> +	   char path[MAX_PATH];
> +	   CFURLGetFileSystemRepresentation(certURLRef.get(), false,
(UInt8*)path, MAX_PATH);

reinterpret_cast is preferred to a C-style cast.

> +    , m_certPath (certificateBundlePath())

You have an extra space after "m_certPath".

> +    if (!m_certPath.isNull())
> +	  curl_easy_setopt(d->m_handle, CURLOPT_CAINFO, m_certPath.data());

Checking isEmpty() seems a bit better than checking isNull().

> +    const CString m_certPath;

m_certificatePath seems a little clearer. I don't think the abbreviation adds
anything here.

r=me