[webkit-reviews] review denied: [Bug 120160] [GTK] Add WebKit2 API for TLS errors : [Attachment 213171] 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 213171: Patch
https://bugs.webkit.org/attachment.cgi?id=213171&action=review

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


> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:61
> +	   PlatformCertificateInfo certificateInfo(resourceError);
> +	   webkitWebViewLoadFailedWithTLSErrors(WEBKIT_WEB_VIEW(clientInfo),
resourceError.failingURL().utf8().data(), webError.get(), certificateInfo);

I still don't see the point of using this PlatformCertificateInfo object here
instead of passing certificate + flags like we currently do. If we need to add
new API to PlatformCertificateInfo to create it with certificate + flags we can
always add it. In any case, hopefully this is going to change soon, see bug
#118520.

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:44
> + * the permission-request signal is emitted with this
WebKitTLSPermissionRequest.

#WebKitWebView::permission-request

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:99
> +    request->priv->webView = webView;

It seems we only want the view to get its web context, why not passing the
context instead since it's what we actually want?

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:118
> +GTlsCertificate*
webkit_tls_permission_request_get_certificate(WebKitTLSPermissionRequest
*request)

This should be documented.

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:125
> +GTlsCertificateFlags
webkit_tls_permission_request_get_tls_errors(WebKitTLSPermissionRequest
*request)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:132
> +const gchar*
webkit_tls_permission_request_get_host(WebKitTLSPermissionRequest *request)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.h:61
> +webkit_tls_permission_request_get_type (void);

Parameters should be lined up with all other methods.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:908
> +   
context->priv->context->allowSpecificHTTPSCertificateForHost(certificate,
String(host));

I think we should allocate the WebCertificateInfo here, right before using it.
host is utf8, you should use String::fromUTF8() here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1522
> +	   WebCore::URL uri(ParsedURLString, String(failingURI));

failingURI is UTF8, you should use String::fromUTF8(). Or you could create a
SoupURI for the given failingURI and pass soup_uri->host directly to avoid
charset conversions.

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:869
>  webkit_uri_scheme_request_get_scheme

You should also add the new type (webkit_tls_permission_request_get_type) to
the file webkit2gtk.types