[webkit-reviews] review denied: [Bug 120160] [GTK] Add WebKit2 API for TLS errors : [Attachment 213272] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Brian Holt
<brian.holt at samsung.com>'s request for review:
Bug 120160: [GTK] Add WebKit2 API for TLS errors
https://bugs.webkit.org/show_bug.cgi?id=120160

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213272&action=review


I'm sorry I was not clear enough in previous reviews about the use of
WebCertificateInfo and PlatformCertificateInfo. May only concern was about
unnecessary allocating a WebCertificate object. Patch looks good to me, except
for the minor details I commented and the lack of WebKitTLSRequest API test. It
would be great if other GTK+ reviewers could review the public API and a
WebKit2 owner the cross-platform changes.

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:55
> +    GTlsCertificateFlags tlsErrors;
> +    GRefPtr<GTlsCertificate> certificate;

We could use a stack allocated PlatformCertificateInfo here and set the
certificate/flags in the init, that way we don't need the new
PlatformCertificateInfo constructor, but only the set methods added in bug
#118520. May main concern was allocating the WebCertificate object here,
because it's only used internally

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:74
> +    webkitWebContextAllowSpecificHTTPSCertificateForHost(priv->context,
priv->tlsErrors, priv->certificate.get(), priv->host.data());

Then you could pass the internal PlatformCertificateInfo here as a const
reference

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:103
> +    request->priv->tlsErrors = tlsErrors;
> +    request->priv->certificate = certificate;

Here you would call certificateInfo.setCertificate and 
certificateInfo.setTLSErrors

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:125
> + * Get the #GTlsCertificate certificate associated with this

the #GTlsCertificate certificate sounds a bit redundant

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequestPrivate.h:23
> +#include "WebCertificateInfo.h"

You don't need this here anymore.

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequestPrivate.h:26
> +#include "WebKitWebView.h"

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:24
> +#include "PlatformCertificateInfo.h"
> +#include "WebCertificateInfo.h"

WebCertificateInfo.h already includes PlatformCertificateInfo.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1521
> +	   GOwnPtr<SoupURI> soup_uri(soup_uri_new(failingURI));

soup_uri -> soupURI

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:206
> +}

I have just realized we are not testing the WebKitPermissionRequest API here,
you should check that get_certificate returns a valid certificate (and even
that it's deleted when test finishes), that tls_errors returns the expected
errors and get_host returns the expected host.

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:273
> +    // In this case the order of the tests does matter because
tls-errors-policy tests the default policy.
> +    SSLTest::add("WebKitWebView", "tls-errors-policy", testTLSErrorsPolicy);


If any other test changes the default value, it should ensure that when the
test finishes the value is set to the default again, so that it doesn't affect
to other tests.