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

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


This looks much better, there's just still a minor issue with the signal
emission. Note also that PlatformCertificateInfo will be renamed as
CertificateInfo in bug #118520.

> Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfoPrivate.h:31
> +	   : certificateInfo(tlsErrors, certificate)
> +    {
> +    }

I actually meant calling setCertificate and setTLSErrors here so that we don't
need the new PlatformCertificateInfo constructor, but in any case the new
constructor makes sense to me.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:859
> +	* and this host use #webkit_web_context_allow_tls_certificate_for_host.


#webkit_web_context_allow_tls_certificate_for_host ->
webkit_web_context_allow_tls_certificate_for_host()

I guess missed this one in previous review, but the rule is the same, simply
use foo() for functions. #Foo for classes and %FOO for enum values

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1556
> +	   gboolean returnValue;
> +	   if (g_signal_has_handler_pending(webView,
signals[LOAD_FAILED_WITH_TLS_ERRORS], 0, TRUE)) {
> +	       GOwnPtr<SoupURI> soupURI(soup_uri_new(failingURI));
> +	       WebKitCertificateInfo info(tlsErrors, certificate);
> +	       g_signal_emit(webView, signals[LOAD_FAILED_WITH_TLS_ERRORS], 0,
&info, soupURI->host, &returnValue);
> +	   } else
> +	       g_signal_emit(webView, signals[LOAD_FAILED], 0,
WEBKIT_LOAD_STARTED, failingURI, error, &returnValue);

Emit the signal and check the return value. If return value is FALSE
(unhandled) you emit the load-failed signal. I think the default value is FALSE
in case nobody connects to the signal, but you can always add a default handler
that simply returns FALSE to be extra sure.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:250
> +						   const gchar		      
host);

Nit: host should be lined up with info, not with the *