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

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


API looks good to me, except what I comment below about the load-failed and
load-failed-with-tls-errors signals. Gustavo?

> Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:32
> + * @See_also: #WebKitWebView

Add #WebKitWebContext here too, since it uses the certificate info.

> Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:35
> + * WebKit will fire a #WebKitWebView::load-failed-with-tls-errors with a

a #WebKitWebView::load-failed-with-tls-errors signal

> Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:39
> + * To handle this signal asynchronously you should copy the
#WebKitCertificateInfo
> + * with #WebKitCertificateInfo::webkit_certificate_info_copy.

#WebKitCertificateInfo::webkit_certificate_info_copy ->
webkit_certificate_info_copy()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:862
> +	* with #webkit_certificate_info_copy.

#webkit_certificate_info_copy -> webkit_certificate_info_copy() and return
%TRUE.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1550
>	   webkitWebViewLoadFailed(webView, WEBKIT_LOAD_STARTED, failingURI,
error);

This will emit the failed signal. I think this should be the fallback when the
new signal is not handled. Imagine an application handling both failed and
filed-with-tls-errors, if we emit failed first, the app doesn't know that
with-tls will be mitted later, and will show an error page with the error. And
apps handling with-tls-errors, are not interested in handling the error again
in load-failed. This is the way we avoid the new policy.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1555
> +	   PlatformCertificateInfo certificateInfo;
> +	   certificateInfo.setCertificate(certificate);
> +	   certificateInfo.setTLSErrors(tlsErrors);

For this particular case I think it makes sense to add a
PlatformCertificateInfo constructor taking certificate + tls errors.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1556
> +	   WebKitCertificateInfo info(certificateInfo);

Or maybe it would be better to add a WebKitCertificateInfo taking the
certificate + flags instead. That way we avoid creating the temporary
PlatformCertificateInfo here. The constructor would call setCertificate and
setTLSErrors on its PlatformCertificateInfo

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:249
> +    gboolean   (* load_failed_with_tls_errors) (WebKitWebView	      
*web_view,
> +						   WebKitCertificateInfo      
*info);

The host parameter is missing here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:174
> +	   return;

Useless return here.

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

Extra line here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:191
> +   
g_assert(G_IS_TLS_CERTIFICATE(webkit_certificate_info_get_tls_certificate(test-
>m_certificateInfo)));

Better add getters for certificate and host