[webkit-reviews] review denied: [Bug 120350] [GTK] Cancel the current active WebKitAuthenticationRequest on load failed : [Attachment 210630] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Anton Obzhirov
<a.obzhirov at samsung.com>'s request for review:
Bug 120350: [GTK] Cancel the current active WebKitAuthenticationRequest on load
failed
https://bugs.webkit.org/show_bug.cgi?id=120350

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

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


This looks much better, but there are still some minor details to fix. Thanks!

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:99
> +	* WebKitAuthenticationRequest::authentication-cancelled:

I think the name is a bit redundant, we can simply use
WebKitAuthenticationRequest::cancelled, which means the authentication request
has been cancelled.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:105
> +	*/

Add Since: tag here, I think we should merge this in the stable branch, so it
should be 2.2, unless other maintainers disagree.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:307
> + * #WebKitAuthenticationRequest::authentication-cancelled signal with
#WebKitAuthenticationRequest will be emitted.

You don't need to say explicitly that the signal will be emitted with a
WebKitAuthenticationRequest, all signals are emitted with the object that emits
them.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1501
> +	   webView->priv->authenticationRequest.clear();

You should also reset this when the load finishes successfully, since this is
not used once the load has finished, and also when the load fails with TLS
errors. I would also reset this when a new load starts, that way we don't
probably need webkitWebViewBaseCancelAuthenticationDialog anymore, if there's
an active request, it will be cancelled (in the dispose method) and the dialog
will be destroyed.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1268
> +    static gboolean
runAuthenticationCancelledCallback(WebKitAuthenticationRequest*,
AuthenticationTest*)

Why is this called run? This doesn't run anything, this is just notified that
the auth request has been cancelled. Also, this should be void, not gboolean.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1292
> +gboolean AuthenticationTest::authenticationCancelledReceived = false;

Don't use gboolean and false, use FALSE instead, or even better, use bool