Bug 119487

Created attachment 209098 [details] Patch

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

Comment on attachment 209098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209098&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:99 > + g_signal_connect(webView, "load-failed", G_CALLBACK(pageLoadFailed), authDialog); You aren't disconnecting form the signal at all in the case that dialog is destroyed. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h:52 > +GtkWidget* webkitAuthenticationDialogNew(WebKitAuthenticationRequest*, CredentialStorageMode, WebKitWebView* webView); You don't need the argument name here.

Created attachment 209191 [details] Patch

Comment on attachment 209191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209191&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:139 > +static void webkitAuthenticationDialogDispose(GObject *object) Your asterisk is in the wrong place here.

Created attachment 209196 [details] Patch Updated with Martin's comments

Comment on attachment 209196 [details] Patch Clearing flags on attachment: 209196 Committed r154329: <http://trac.webkit.org/changeset/154329>

All reviewed patches have been landed. Closing bug.

I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test.

(In reply to comment #9) > I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test. Hi, so do you think I should roll out this patch or it is better to create a new bug to implement it the way you said? Current solution does work anyway for the time being so it can stay before I implement the new one.

(In reply to comment #10) > Hi, so do you think I should roll out this patch or it is better to create a new bug to implement it the way you said? Current solution does work anyway for the time being so it can stay before I implement the new one. To met at least, it seems reasonable to add the cancellation API in another patch.

(In reply to comment #9) > I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test. So if I understand correctly, connect to the load-failed signal in the constructor of the WebKitAuthenticationRequest with the callback being webkit_authentication_request_cancel? Additionally then emit a new signal "authentication-failed" that clients can connect to?

(In reply to comment #10) > (In reply to comment #9) > > I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test. > > Hi, so do you think I should roll out this patch or it is better to create a new bug to implement it the way you said? Current solution does work anyway for the time being so it can stay before I implement the new one. Don't need to roll it out, since as you say it works and it doesn't new API like my proposal, and maybe even others disagree with me.

(In reply to comment #12) > (In reply to comment #9) > > I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test. > > So if I understand correctly, connect to the load-failed signal in the constructor of the WebKitAuthenticationRequest with the callback being webkit_authentication_request_cancel? Additionally then emit a new signal "authentication-failed" that clients can connect to? No, the view would keep a pointer (or ref) to the current request. When load-failed happens, the view checks if there's an active auth request and in that case it calls webkit_authentication_request_cancel. When webkit_authentication_request_cancel is called the request might emit a cancelled signal to notify it was cancelled, so that the users can destroy their UI.

(In reply to comment #14) > (In reply to comment #12) > > (In reply to comment #9) > > > I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test. > > > > So if I understand correctly, connect to the load-failed signal in the constructor of the WebKitAuthenticationRequest with the callback being webkit_authentication_request_cancel? Additionally then emit a new signal "authentication-failed" that clients can connect to? > > No, the view would keep a pointer (or ref) to the current request. When load-failed happens, the view checks if there's an active auth request and in that case it calls webkit_authentication_request_cancel. When webkit_authentication_request_cancel is called the request might emit a cancelled signal to notify it was cancelled, so that the users can destroy their UI. Sounds good to me, I will create new bug/patch.