[webkit-reviews] review denied: [Bug 119436] [curl] Improve detecting and handling of SSL related errors : [Attachment 216791] proposed patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied sipka at inf.u-szeged.hu's request
for review:
Bug 119436: [curl] Improve detecting and handling of SSL related errors
https://bugs.webkit.org/show_bug.cgi?id=119436

Attachment 216791: proposed patch
https://bugs.webkit.org/attachment.cgi?id=216791&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=216791&action=review


Looks very good!  A few style problems need to be cleaned up, but otherwise
this looks great!  Thanks for getting this functionality in place.

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:618
> +		   ResourceError resourceError = ResourceError(String(),
msg->data.result, String(url), String(curl_easy_strerror(msg->data.result)));

This construct is a little funny.  Why not just declare resourceError with
construction in place?

ResourceError resourceError(String(), msg->data.result, String(url),
String(curl_easy_strerror(msg->data.result));

> Source/WebCore/platform/network/curl/SSLHandle.cpp:122
> +bool pemData(X509_STORE_CTX *ctx, String& certificate)

We write this as X509_STORE_CTX*

> Source/WebCore/platform/network/curl/SSLHandle.cpp:142
> +static int certVerifyCallback(int ok, X509_STORE_CTX *ctx)

X509_STORE_CTX*

> Source/WebCore/platform/network/curl/SSLHandle.cpp:150
> +    SSL* ssl = (SSL*)X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());

This should be a C++-style cast (prob. a reinterpret_cast<SSL*>)

> Source/WebCore/platform/network/curl/SSLHandle.cpp:152
> +    ResourceHandle* job  = (ResourceHandle*)SSL_CTX_get_app_data(sslctx);

C++ style cast.

> Source/WebCore/platform/network/curl/SSLHandle.cpp:170
> +	   // so don't need to curl verifies the authenticity of the peer's
certificate

Because of this large comment, you should wrap the comment and curl_easy_setopt
in brackets to make the structure clear:

if (ok) {
    // blah blah
    curl_easy_setopt(...)
}

> Source/WebCore/platform/network/curl/SSLHandle.cpp:175
> +static CURLcode sslctxfun(CURL * curl, void * sslctx, void * parm)

CURL* curl, void* sslctx, void* parm

> Source/WebCore/platform/network/curl/SSLHandle.cpp:177
> +    SSL_CTX_set_app_data((SSL_CTX*)sslctx, parm);

C++-style casts, please.