[webkit-reviews] review denied: [Bug 120350] [GTK] Cancel the current active WebKitAuthenticationRequest on load failed : [Attachment 209754] 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 209754: Patch
https://bugs.webkit.org/attachment.cgi?id=209754&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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));