Bug 120350 – [GTK] Cancel the current active WebKitAuthenticationRequest on load failed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Anton Obzhirov]

See https://bugs.webkit.org/show_bug.cgi?id=119487 for more details. The patch will follow shortly.

[Anton Obzhirov]

Created attachment 209754 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Sergio Villar Senin]

Comment on attachment 209754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209754&action=review

Some comments

> Source/WebKit2/ChangeLog:10
> +        New test has been added in authentication tests.

Instead of a list of short descriptions of the changes, the ChangeLog normally contains a short description of the issue and the fix.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1432
> +            "cancel-authenticate",

This might be a bit of bikeshedding but authentication-canceled sounds more gnomish to me, although I'd let the API maintainers decide.

[Brian Holt]

Comment on attachment 209754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209754&action=review

Informal review: overall it looks good to me.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1432
>> +            "cancel-authenticate",
> 
> This might be a bit of bikeshedding but authentication-canceled sounds more gnomish to me, although I'd let the API maintainers decide.

I'm not a gnome developer but I think "authentication-cancelled" sounds more intuitive as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:248
> +    gboolean   (* cancel_authenticate)       (WebKitWebView               *web_view);

You should remove the last open slot (reserved6) to maintain the ABI.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1504
> +    AuthenticationTest::add("WebKitWebView", "authentication-cancel-authentication", testWebViewCancelAuthentication);

I would go for more descriptive names because these 2 tests are a bit ambiguous now. Perhaps authentication-load-cancelled?

[Anton Obzhirov]

Thanks for both reviews! - I will incorporate all comments in my next patch. Now waiting for Carlos :)

[Carlos Garcia Campos]

Comment on attachment 209754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209754&action=review

Agree with all other review comments, thanks Sergio and Brian.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:35
> +    unsigned long cancelAuthenticateEventID;

authenticationCancelledID;

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:124
> +    CANCEL_AUTHENTICATE,

This is a notification, it should be AUTHENTICATION_CANCELLED, but I wonder if it would be better to simply add a cancelled signal to the WebKitAuthenticationRequest object instead of adding another signal to the WebView.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1426
> +     * Returns: %TRUE to stop other handlers from being invoked for the event.
> +     *   %FALSE to propagate the event further.

Why is this true_handled? What happens when the user returns TRUE or FALSE?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1821
>      GRefPtr<WebKitAuthenticationRequest> request = adoptGRef(webkitAuthenticationRequestCreate(authenticationChallenge, privateBrowsingEnabled));
> +    webView->priv->authenticationRequest = request;

You can merge this now, and keep the initial reference in the view.

webView->priv->authenticationRequest = adoptGRef(webkitAuthenticationRequestCreate(authenticationChallenge, privateBrowsingEnabled));

[Brian Holt]

(In reply to comment #6)
> [...]
> This is a notification, it should be AUTHENTICATION_CANCELLED, but I wonder if it would be better to simply add a cancelled signal to the WebKitAuthenticationRequest object instead of adding another signal to the WebView.

Since this signal would always and only be emitted when authenication is cancelled, it makes sense to me to put in the request and emit the signal in webkit_authentication_request_cancel(), so I support this idea.

[Anton Obzhirov]

Created attachment 210630 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 210630 [details]
Patch

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

[Anton Obzhirov]

Comment on attachment 210630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210630&action=review

>> 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.

Makes sense for me as well

>> 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.

ok

>> 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.

ok

>> 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.

I'll check webkitWebViewBaseCancelAuthenticationDialog - I think you're right I just need to cover all places where request should be reset.

>> 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.

yep

>> 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

I'll go for bool - makes sense to use standard types for me

[Anton Obzhirov]

Created attachment 210758 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 210758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210758&action=review

This looks good to me, setting r- only because of the static function. This adds new API, so we need approval from another GTK+ port maintainer.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3081
> +void webkitWebViewResetAuthenticationRequest(WebKitWebView* webView)

This function is only used by this file, so it should be static. I'm not sure reset is the best name, though, I would use Cancel or Clear.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3083
> +    if (webView->priv->authenticationRequest) {

Use an early return here instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h:63
> +void webkitWebViewResetAuthenticationRequest(WebKitWebView*);

This is not needed because the function is only used in WebKitWebView.cpp

[Anton Obzhirov]

Created attachment 211028 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 211028 [details]
Patch

The API looks good to me!

[Carlos Garcia Campos]

Comment on attachment 211028 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 211028 [details]
Patch

Clearing flags on attachment: 211028

Committed r155347: <http://trac.webkit.org/changeset/155347>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.