Bug 99352

Created attachment 207091 [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

Attachment 207091 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h" WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h:27: The parameter name "authenticationChallenge" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h:28: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h:31: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h:31: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h:32: The parameter name "authenticationChallenge" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h:32: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h:35: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequestPrivate.h:35: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:25: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:119: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:122: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:123: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:124: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:132: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:134: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:135: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:136: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:137: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:146: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:148: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:167: Declaration has space between type name and * in AuthenticationChallengeProxy *authenticationChallenge [whitespace/declaration] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:167: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:182: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:199: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:216: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:831: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1231: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1232: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1234: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1235: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1236: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1237: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:25: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h" WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialogRequest.h" Total errors found: 38 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #1) > Created an attachment (id=207091) [details] > Patch This is a WIP patch proposed to add the API and signal to support a user-created authentication dialog. Because the API is not yet confirmed I haven't added documentation for all the functions. Questions for discussion: - is the name of the request object, "WebkitAuthenticationDialogRequest" right? - is the API of WebkitAuthenticationDialogRequest right? - is the name of the signal "run-authentication-dialog" right? - is the architecture right? There will be an email to webkitgtk-dev where this can be discussed.

Created attachment 207111 [details] Patch

Attachment 207111 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h" WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h" Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:27: The parameter name "authenticationChallenge" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:30: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:31: The parameter name "authenticationChallenge" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:31: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:34: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:25: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:90: Declaration has space between type name and * in AuthenticationChallengeProxy *authenticationChallenge [whitespace/declaration] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:90: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:112: Missing space before ( in switch( [whitespace/parens] [5] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:113: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:864: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:865: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:866: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:25: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:152: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:831: Should have a space between // and comment [whitespace/comments] [4] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h" Total errors found: 20 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #6) > Attachment 207111 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h']" exit_code: 1 > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h" > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h" > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:27: The parameter name "authenticationChallenge" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:30: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:31: The parameter name "authenticationChallenge" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:31: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:34: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h" > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:25: Alphabetical sorting problem. [build/include_order] [4] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:90: Declaration has space between type name and * in AuthenticationChallengeProxy *authenticationChallenge [whitespace/declaration] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:90: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:112: Missing space before ( in switch( [whitespace/parens] [5] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:113: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:864: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:865: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:866: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:25: Alphabetical sorting problem. [build/include_order] [4] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:152: Should have a space between // and comment [whitespace/comments] [4] > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:831: Should have a space between // and comment [whitespace/comments] [4] > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h" > Total errors found: 20 in 13 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Can you please upload your patches with webkit-patch or run them through check-webkit-style first to ensure that the bugzilla style checker does not keep spamming the patch? :)

Or better, don't set the review flag for wip patches, even less the commit-queue one.

(In reply to comment #8) > Or better, don't set the review flag for wip patches, even less the commit-queue one. Sorry about that, I'll be sure not to in the future.

Created attachment 207193 [details] Revised WIP Patch Fixed style issues, revised authenticate logic, pass CredentialStorageMode to WebKitAuthenticationDialogNew.

(In reply to comment #9) > (In reply to comment #8) > > Or better, don't set the review flag for wip patches, even less the commit-queue one. > > Sorry about that, I'll be sure not to in the future. No problem!, sorry if I sounded a bit rude, I really appreciate you are working on this.

(In reply to comment #11) > (In reply to comment #9) > > (In reply to comment #8) > > > Or better, don't set the review flag for wip patches, even less the commit-queue one. > > > > Sorry about that, I'll be sure not to in the future. > > No problem!, sorry if I sounded a bit rude, I really appreciate you are working on this. Don't worry, I didn't take it personally. Both you and Martin made good points and since I'm hoping to contribute more regularly I want to make sure that I learn from these mistakes.

(In reply to comment #10) > Created an attachment (id=207193) [details] > Revised WIP Patch > > Fixed style issues, revised authenticate logic, pass CredentialStorageMode to WebKitAuthenticationDialogNew. Leaving aside the implementation details, are the names and the API ok? If they are then I'll get on with documenting the API properly and writing the unit test.

Comment on attachment 207193 [details] Revised WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207193&action=review Note that webkit could be compiled without libsecret, so we probably need a way to tell the user whether we can save credentials persistently or not. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:34 > + GRefPtr<_WebKitAuthenticationRequest> request; Use WebKitAuthenticationRequest. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:147 > + authDialog->priv->request = adoptGRef(request); I think we should keep a ref instead of adopting it. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:149 > + AuthenticationChallengeProxy* authenticationChallenge = webkit_authentication_request_get_authentication_challenge(request); > authDialog->priv->authenticationChallenge = authenticationChallenge; If the request now contains the challenge, we don't need to keep another reference in the private struct. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:23 > +#include "AuthenticationChallengeProxy.h" This is already included by WebKitAuthenticationRequestPrivate.h > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:29 > +#include <wtf/gobject/GOwnPtr.h> > +#include <wtf/gobject/GRefPtr.h> GOwnPtr and GRefPtr don't seem to be used in this file at all. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:41 > + * Whenever the user attempts to load a page protected by http http -> HTTP > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:43 > + * authentication, WebKit will need to show a dialog for the user to > + * enter their username and password. For that to happen in a general way, Why does wk has to show a dialog? > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:44 > + * instead of just opening the custom #WebKitAuthenticationDialog WebKitAuthenticationDialog is private, don't mention it in the documentation. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:53 > + * a #WebKitAuthenticationDialog for the user to interact with. Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:61 > +WEBKIT_DEFINE_TYPE(WebKitAuthenticationRequest, webkit_authentication_request, G_TYPE_OBJECT); Unneeded trailing ; here. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:88 > +void webkit_authentication_request_set_authentication_challenge(WebKitAuthenticationRequest *request, AuthenticationChallengeProxy *authenticationChallenge) This is a private method, it should follow the webkit coding style. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:90 > + g_return_if_fail(WEBKIT_IS_AUTHENTICATION_REQUEST(request)); Do not use g_return macros in private methods. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:92 > +} This method is unused. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:97 > +AuthenticationChallengeProxy* webkit_authentication_request_get_authentication_challenge(WebKitAuthenticationRequest *request) > +{ > + return request->priv->authenticationChallenge.get(); > +} This method is unused. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:100 > +void webkit_authentication_request_authenticate(WebKitAuthenticationRequest *request, const gchar *username, const gchar *password, WebKitCredentialPersistence webkitPersistence) WebKitAuthenticationRequest *request -> WebKitAuthenticationRequest* request const gchar *username -> const gchar* username const gchar *password -> const gchar* password I wonder if it would be better to expose a boxed type for the credential WebKitCredential or WebKitWebCrendetial wrapping a WebCore::Credential. It will simplify the API and will allow us to add support for client certificate authentication using the same API in the future. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:118 > + CredentialPersistence persistence; > + switch (webkitPersistence) { > + case WebKitCredentialPersistenceNone: > + persistence = CredentialPersistenceNone; > + break; > + case WebKitCredentialPersistenceForSession: > + persistence = CredentialPersistenceForSession; > + break; > + case WebKitCredentialPersistencePermanent: > + persistence = CredentialPersistencePermanent; > + break; > + default: > + // warn that the WebKitCredentialPersistence enum is invalid > + break; > + } Instead of this we can use the same values than WebCore and add a compile time assert to make sure the values match. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:120 > + Credential credential = request->priv->authenticationChallenge->proposedCredential()->core(); What is this for? > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:130 > + RefPtr<WebString> usernameString = WebString::createFromUTF8String(username); > + if (usernameString->isEmpty()) > + usernameString = WebString::create(credential.user()); > + > + RefPtr<WebString> passwordString = WebString::createFromUTF8String(password); > + if (passwordString->isEmpty()) > + passwordString = WebString::create(credential.password()); > + > + RefPtr<WebCredential> webCredential = WebCredential::create(usernameString.get(), passwordString.get(), persistence); All this could be simplified as: RefPtr<WebCredential> webCredential = WebCredential::create(WebCore::Credential(String::fromUTF8(username), String::fromUTF8(password), persistence)); I think we should consider an error to call this method with NULL user/pass, if the user wants to use the values stored in the keyring, they should pass back the proposed credential. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:139 > + * Ask WebKit to cancel the request. It's important to do this in case I would just say something like Cancel the authentication request, instead of Ask WebKit . . . > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:142 > + * pending forever, which might cause the browser to hang. Browser? I should not assume the API is only sed by browsers. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:143 > + */ Since: 2.2 > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:62 > + WebKitCredentialPersistenceNone, > + WebKitCredentialPersistenceForSession, > + WebKitCredentialPersistencePermanent, This is public API, it should follow GNOME coding style. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:76 > + We need a way to get the proposed credential, to fill the UI or whatever and use it in case the user wants to use the saved data. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:32 > +void > +webkit_authentication_request_set_authentication_challenge(WebKitAuthenticationRequest*, WebKit::AuthenticationChallengeProxy*); > + > +WebKit::AuthenticationChallengeProxy* webkit_authentication_request_get_authentication_challenge(WebKitAuthenticationRequest*); Remove this unused methods. > Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:135 > - webkitWebViewHandleAuthenticationChallenge(WEBKIT_WEB_VIEW(clientInfo), toImpl(authenticationChallenge)); > + WebKitAuthenticationRequest* request = webkitAuthenticationRequestCreate(toImpl(authenticationChallenge)); > + webkitWebViewHandleAuthenticationChallenge(WEBKIT_WEB_VIEW(clientInfo), request); You can leave this as it was and create the request in webkitWebViewHandleAuthenticationChallenge. You should use GRefPtr and adopt the ref, so that callback handlers (the default one or user provided one) can increase the ref counter if they need to handle the signal asynchronously. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:92 > + AUTHENTICATION, This should be AUTHENTICATE > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:433 > +static gboolean webkitWebViewRunAuthenticationDialog(WebKitWebView* webView, WebKitAuthenticationRequest* request) This is private method, use bool instead of gboolean. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:439 > + CredentialStorageMode credentialStorageMode; > + if (webkit_settings_get_enable_private_browsing(webkit_web_view_get_settings(webView))) > + credentialStorageMode = DisallowPersistentStorage; > + else > + credentialStorageMode = AllowPersistentStorage; This should be handled by the WebKitAuthenticationRequest too. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:615 > + webViewClass->authentication = webkitWebViewRunAuthenticationDialog; This should be authenticate and the default handler should be called webkitWebViewAuthenticate, so that it's clear it's the default handler of the authenticate signal. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:853 > + * This signal is emitted when the user is challenged with http http -> HTTP > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:855 > + * a dialog to where the user can supply credentials. To let the Why a dialog, the user should be free to use whatever to authenticate. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:861 > + * You should mention that it's possible to handle this asynchronously by keeping a ref of the request and returning TRUE. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:867 > + * Since: 2.2 > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:869 > + signals[AUTHENTICATION] = AUTHENTICATE > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:211 > + gboolean (* authentication) (WebKitWebView *web_view, > + WebKitAuthenticationRequest *request); authenticate. This is an ABI break, you should add the vfunc at the bottom and remove one of he paddings.

Created attachment 207310 [details] WIP Patch 2 A second WIP patch, wrapping WebCore::Credential in a new class WebKitCredential.

Unfortunately the unit test where I was planning to test this functionality is being skipped because its failing: https://bugs.webkit.org/show_bug.cgi?id=117689. One option is to create a new test TestWebKitAuthentication.cpp, although it seems like it belongs in TestWebKitWebView.

Thanks for working on this! As an uzbl developer, is see a few things missing. It would be nice to have the "realm" if possible for the request; the host and/or URI might not be unique enough. Second, a credentials property would be nice for the request class and username/password/persistence on the credentials class. Is there documentation for where WebKit stores credentials or is it just "see libsecret docs"? An option in WebKitSettings along the lines of "enable-credential-storage" would be appreciated as well if possible.

(In reply to comment #16) > Unfortunately the unit test where I was planning to test this functionality is > being skipped because its failing: https://bugs.webkit.org/show_bug.cgi? > id=117689. That is bad luck :). Fortunately, I don't think that test will be skipped forever so that should not hold you form writing the test in the right place. > One option is to create a new test TestWebKitAuthentication.cpp, although it > seems like it belongs in TestWebKitWebView. Yes. As the central point of this API addition is the signal you are adding to WebKitWebView, this new unit test should IMHO be added there. You'll probably want to create a subclass of WebViewTest inside of TestWebKitWebView.cpp as well to write that new unit test, btw.

Comment on attachment 207310 [details] WIP Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=207310&action=review Great patch, Brian! Please consider some comments from my side too, unless Carlos disagree with them of course (since he will probably be right anyway) :) > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:99 > +gboolean webkit_authentication_request_can_save_credential(WebKitAuthenticationRequest *request) Misplaced * for 'request', since we are in a private file (it's fine in the header file) > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:111 > +{ > + g_return_val_if_fail(WEBKIT_IS_AUTHENTICATION_REQUEST(request), FALSE); > + > + gboolean credentialStorageEnabled; > +#if ENABLE(CREDENTIAL_STORAGE) > + credentialStorageEnabled = TRUE; > +#else > + credentialStorageEnabled = FALSE; > +#endif > + > + return (!request->priv->privateBrowsingEnabled && credentialStorageEnabled); > +} You don't really need that credentialStorageEnabled variable there. Why not doing just something like the following? { g_return_val_if_fail(WEBKIT_IS_AUTHENTICATION_REQUEST(request), FALSE); #if ENABLE(CREDENTIAL_STORAGE) return !request->priv->privateBrowsingEnabled; #else return FALSE; #endif } > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:120 > + * Returns: #WebKitCredential A short sentence is probably better here :) > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:122 > +WebKitCredential* webkit_authentication_request_get_credential(WebKitAuthenticationRequest *request) Misplaced * > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:126 > + WebKitCredential* credential = webkitCredentialCreate(request->priv->authenticationChallenge->proposedCredential()->core()); > + return credential; This can be made a single statement. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:137 > +void webkit_authentication_request_authenticate(WebKitAuthenticationRequest *request, WebKitCredential *credential) Misplaced * > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:154 > +void webkit_authentication_request_cancel(WebKitAuthenticationRequest *request) Misplaced * > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:72 > + * Returns: The passed in #WebKitCredential Nit: Missing period at the end > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:103 > + * Returns: the username Nit: This should be a proper sentence, starting with Capitalized words and finishing with a period > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:105 > +const gchar * webkit_credential_get_username(WebKitCredential* credential) Unneeded extra space between gchar and * here > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:116 > + * Returns: the password Same nit than before. Sorry! > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:119 > +{ ...and same extra space here > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:129 > + * Returns: the #WebKitCredentialPersistence Likewise > Source/WebKit2/UIProcess/API/gtk/WebKitCredentialPrivate.h:30 > +#endif // WebKitJavascriptResultPrivate_h Wrong comment > Source/WebKit2/UIProcess/API/gtk/WebKitCredentialPrivate.h:38 > + > + > + > + > + > + > + > + Unneeded extra lines at the end of the file > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:93 > + AUTHENTICATE, Ok. This is probably too much of a nit, but I would probably separate this line from the following three lines with a blank one, since those three are related between them and this is a completely different thing. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:440 > + CredentialStorageMode credentialStorageMode; > + if (webkit_authentication_request_can_save_credential(request)) > + credentialStorageMode = AllowPersistentStorage; > + else > + credentialStorageMode = DisallowPersistentStorage; What about using the ternary operator to make this 5 lines just one? Actually, we could even combine that ternary operator with the following one, but I'm afraid readability would suffer too much. What do you think? > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:796 > +class AuthenticationTest: public UIClientTest { As I mentioned in my previous comment, you probably need to inherit from WebViewTest since all the things you will need for this test are already there. Thus, there's no need I think to use UIClientTest here. Sorry if my comment in the office the other way was misleading! :)

Comment on attachment 207310 [details] WIP Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=207310&action=review I agree with Mario too. > Source/WebKit2/ChangeLog:9 > + emitted along with a new WebKitWebView::authentication signal to WebKitWebView::authenticate signal > Source/WebKit2/ChangeLog:11 > + when the user is challenged with http authentication. The http -> HTTP > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:54 > + WebKitCredential* credential = webkitCredentialCreate(webkitAuthenticationWidgetCreateCredential(WEBKIT_AUTHENTICATION_WIDGET(priv->authWidget))); > + webkitAuthenticationDialogAuthenticate(authDialog, credential); Now that we are not calling webkitAuthenticationDialogAuthenticate with a NULL credential to cancel the request, we can probably create the credential in webkitAuthenticationDialogAuthenticate instead of passing it as a parameter. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:61 > - webkitAuthenticationDialogAuthenticate(authDialog, 0); > + WebKitAuthenticationDialogPrivate* priv = authDialog->priv; > + webkit_authentication_request_cancel(authDialog->priv->request.get()); > + gtk_widget_destroy(GTK_WIDGET(authDialog)); I would use a helper function here too like webkitAuthenticationDialogCancel > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:90 > + AuthenticationChallengeProxy* authenticationChallenge = webkit_authentication_request_get_authentication_challenge(authDialog->priv->request.get()); > + authDialog->priv->authWidget = webkitAuthenticationWidgetNew(authenticationChallenge->core(), credentialStorageMode); This could probably be a single statement split in two lines, since we don't really need the local variable. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:58 > +/* Private methods */ Don't use C style comments, in this particular case I don't think we need a comment to know these are the private methods. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:87 > +AuthenticationChallengeProxy* webkit_authentication_request_get_authentication_challenge(WebKitAuthenticationRequest* request) > +{ > + return request->priv->authenticationChallenge.get(); > +} This is a private method, it should follow the WebKit coding style. webkitAuthenticationRequestGetAuthenticationChallenge() > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:89 > +/* Public methods */ Same comment here that for private methods. Public methods are documented so it's pretty obvious which ones are public. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:91 > + * webkit_authentication_request_can_save_credential: Reading this again I've noticed that the names suggest that there's a specific credential for which you are checking if persistent storage is allowed, but this is generic, maybe we could use webkit_authentication_request_can_save_credentials instead. Maybe Martin or Gustavo have any other suggestion? > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:95 > + * #WebKitFileChooserRequest should allow the storage of credentials. WebKitFileChooser?. I don't think this is accurate, the user can allow to save credentials by providing their own way. This methods returns whether webkit supports can save credentials, which means that it's supported (build with libsecret) and private browsing is not enabled. You should explicitly mention here that when private browsing is enabled this method will return %FALSE. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:98 > + */ Since: 2.2 Please add this tag to all new methods, properties and signals added to the API, please read: http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:118 > + * Get the #WebKitCredential currently stored in the keyring. The client can use this > + * directly for authentication or construct their own. I would avoid mentioning the keyring here, I would say the proposed credential or something like that. >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:120 >> + * Returns: #WebKitCredential > > A short sentence is probably better here :) You should also add gobject introspection annotations to make it clear that this returns a new object that the user should free. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:125 > + WebKitCredential* credential = webkitCredentialCreate(request->priv->authenticationChallenge->proposedCredential()->core()); We can check if the credential is empty and return NULL instead, since the WebKitCredential object is read-only, returning a new allocated empty object is useless. You should document also that it can return NULL if the request doesn't have any initial/proposed credential. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:151 > + * won't be properly completed and the browser will keep the request Why the browser? > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:157 > + request->priv->authenticationChallenge->listener()->cancel(); This is not correct, we don't want to cancel the auth, but to continue the load without credentials, so you should call AuthenticationDecisionListener::useCredential with a NULL credential like the current code does. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:71 > +webkit_authentication_request_authenticate (WebKitAuthenticationRequest *request, > + WebKitCredential *credential); Parameter names should be lined up > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:28 > + Remove this empty line > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequestPrivate.h:29 > +WebKit::AuthenticationChallengeProxy* webkit_authentication_request_get_authentication_challenge(WebKitAuthenticationRequest*); This is private method it should follow the WebKit coding style. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:33 > + , referenceCount(1) I'm not sure we really need this to be refcounted. This will have a very short life, that's why I proposed to use a boxed type, and it will be mostly read-only object. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:37 > + WebCore::Credential core; Call this credential instead of core > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:54 > +WebCredential* webkit_credential_get_webcredential(WebKitCredential* credential) > +{ > + RefPtr<WebCredential> webCredential = WebCredential::create(credential->core); > + return webCredential.release().leakRef(); This is private method, it should follow WebKit coding style, why do we need to create a WebCredential, why not returning a const ref of the WebCore credential? We can create the WebCredential in the caller instead. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:107 > + return credential->core.user().utf8().data(); This is a temp value, we need to cache the user/password as CString in the struct to be able to return a const char *. We can cache it on demand. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:120 > + return credential->core.password().utf8().data(); Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.h:40 > +typedef enum { > + WebKitCredentialPersistenceNone, > + WebKitCredentialPersistenceForSession, > + WebKitCredentialPersistencePermanent, > +} WebKitCredentialPersistence; This should follow the GNOME coding style and should be documented as well > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.h:46 > +webkit_credential_new (const gchar *username, const gchar *password, WebKitCredentialPersistence persistence); Every parameter should be in a new line with the names lined up. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:850 > /** > + * WebKitWebView::authenticate: Indentation is wrong here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:861 > + * of the request and return TRUE. Extra space at the beginning of the line here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:870 > + * Remove this extra line. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:247 > + gboolean (* authenticate) (WebKitWebView *web_view, > + WebKitAuthenticationRequest *request); Don't replace the padding, place this together with the other vfunc and remove the padding, probably better to remove the last padding instead of the first one. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:802 > + g_signal_connect(m_webView, "authentication", G_CALLBACK(runAuthenticationCallback), this); You should disconnect the signal in the destructor. Signal name is authenticate.

(In reply to comment #17) > Thanks for working on this! As an uzbl developer, is see a few things missing. It would be nice to have the "realm" if possible for the request; the host and/or URI might not be unique enough. I'm not quite sure what you mean by "realm", could you expand a bit on how you would like to see this implemented? > Second, a credentials property would be nice for the request class and username/password/persistence on the credentials class. Sure, I could add properties for these, thats not a problem. Any comments/objections from Carlos, Martin or Gustavo? > Is there documentation for where WebKit stores credentials or is it just "see libsecret docs"? An option in WebKitSettings along the lines of "enable-credential-storage" would be appreciated as well if possible. webkit_authentication_request_can_save_credentials() in the request class seems to be what you are looking for - credentials are saved if the library support is available and we are not in private browsing mode.

(In reply to comment #21) > (In reply to comment #17) > > Thanks for working on this! As an uzbl developer, is see a few things missing. It would be nice to have the "realm" if possible for the request; the host and/or URI might not be unique enough. > > I'm not quite sure what you mean by "realm", could you expand a bit on how you would like to see this implemented? The realm is available in the WebCore::AuthenticationChallenge. note that the user should be able to build the exact same auth dialog that wk uses by default with the public API. > > Second, a credentials property would be nice for the request class and username/password/persistence on the credentials class. > > Sure, I could add properties for these, thats not a problem. Any comments/objections from Carlos, Martin or Gustavo? I don't see why we need a property in the request, the credential object is not going to change and it's created on demand (at least in the current patch). The credential class is a boxed type not a gobject, think about it like a struct with a GType. > > Is there documentation for where WebKit stores credentials or is it just "see libsecret docs"? An option in WebKitSettings along the lines of "enable-credential-storage" would be appreciated as well if possible. > > webkit_authentication_request_can_save_credentials() in the request class seems to be what you are looking for - credentials are saved if the library support is available and we are not in private browsing mode.

Comment on attachment 207310 [details] WIP Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=207310&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:91 >> + * webkit_authentication_request_can_save_credential: > > Reading this again I've noticed that the names suggest that there's a specific credential for which you are checking if persistent storage is allowed, but this is generic, maybe we could use webkit_authentication_request_can_save_credentials instead. Maybe Martin or Gustavo have any other suggestion? I agree, credentials makes it more understandable. I wonder if this needs to be a WebKitAuthenticationRequest method, perhaps it should go in the Context? > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:94 > + * Determine whether the authentication method associated with this Authentication method sounds misleading to me, it sounds like HTTP basic auth may allow saving whereas other methods won't. Also, why is it associated with a file chooser request at all?

(In reply to comment #23) > (From update of attachment 207310 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207310&action=review > > >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:91 > >> + * webkit_authentication_request_can_save_credential: > > > > Reading this again I've noticed that the names suggest that there's a specific credential for which you are checking if persistent storage is allowed, but this is generic, maybe we could use webkit_authentication_request_can_save_credentials instead. Maybe Martin or Gustavo have any other suggestion? > > I agree, credentials makes it more understandable. I wonder if this needs to be a WebKitAuthenticationRequest method, perhaps it should go in the Context? I agree it is global and not specific to the given request, but it can't be in the context either, since it depends also on whether private browsing is enabled, so it actually depends on the web view group. I think having it in WebKitAuthRequest is very convenient to use.

(In reply to comment #23) > (From update of attachment 207310 [details]) > Also, why is it associated with a file chooser request at all? :/ because I used the FileChooserRequest as my starting point and missed this when doing my find/replace. Sorry for confusion.

(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #17) > > > Thanks for working on this! As an uzbl developer, is see a few things missing. It would be nice to have the "realm" if possible for the request; the host and/or URI might not be unique enough. > > > > I'm not quite sure what you mean by "realm", could you expand a bit on how you would like to see this implemented? > > The realm is available in the WebCore::AuthenticationChallenge. note that the user should be able to build the exact same auth dialog that wk uses by default with the public API. I agree completely but I've been wondering how far to take this. The WebKitAuthenticationDialog and WebKitAuthenticationWidget currently use the AuthenticationChallengeProxy directly, should it be refactored to use the public API of WebKitAuthenticationRequest? Perhaps then I could remove the private function webkitAuthenticationRequestGetAuthenticationChallenge() which would be substituted by _get_realm(), _get_host(), _get_port()?

(In reply to comment #26) > (In reply to comment #22) > > (In reply to comment #21) > > > (In reply to comment #17) > > > > Thanks for working on this! As an uzbl developer, is see a few things missing. It would be nice to have the "realm" if possible for the request; the host and/or URI might not be unique enough. > > > > > > I'm not quite sure what you mean by "realm", could you expand a bit on how you would like to see this implemented? > > > > The realm is available in the WebCore::AuthenticationChallenge. note that the user should be able to build the exact same auth dialog that wk uses by default with the public API. > > I agree completely but I've been wondering how far to take this. The WebKitAuthenticationDialog and WebKitAuthenticationWidget currently use the AuthenticationChallengeProxy directly, should it be refactored to use the public API of WebKitAuthenticationRequest? Perhaps then I could remove the private function webkitAuthenticationRequestGetAuthenticationChallenge() which would be substituted by _get_realm(), _get_host(), _get_port()? WebKitAuthWidget is in WebCore so it can't use the public API. And the AuthDialog doesn't use the auth challenge except to create the WebKitAuthWidget, no? I think we can just add methods to get host, port, realm, etc. caching the required string on demand, so that when the default dialog is used we are not duplicating any string.

Created attachment 207532 [details] Patch + docs Proposal for the API + docs (tests to follow)

(In reply to comment #28) > Created an attachment (id=207532) [details] > Patch + docs > > Proposal for the API + docs (tests to follow) Looks great! I'll try testing it out with uzbl tonight.

(In reply to comment #29) > Looks great! I'll try testing it out with uzbl tonight. First, the headers are missing from the variable which has the list of headers to install (webkit2gtk_h_api?). So some differences I see compared to WebKit1 (which used libsoup directly) is that there's no indication of whether the authentication is for a proxy or not (looks to be accessible from WebCore::ProtectionSpaceServerType), and the paths which are affected by the request (which doesn't look like it's plumbed through WebCore at all). Also, the port from the request is a gint; maybe it makes more sense as a guint (as the port method on WebKitSecurityOrigin uses)?

(In reply to comment #29) > Looks great! I'll try testing it out with uzbl tonight. I'm seeing the following: (gdb) list <snip> 779 WebKitAuthenticationScheme scheme = webkit_authentication_request_get_scheme (request); <snip> (gdb) p scheme $1 = WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN (gdb) p request->priv->authenticationChallenge.m_ptr->m_coreAuthenticationChallenge.m_protectionSpace.m_authenticationScheme $2 = WebCore::ProtectionSpaceAuthenticationSchemeHTTPBasic so it seems that the auth scheme is mismatching somewhere. I also see m_previousFailureCount in there. Is it worth plumbing that through (at least as a "has this auth been tried before?" property)?

Here's the code I'm using: https://github.com/mathstuf/uzbl/commit/e1a6f6bf9d69157528eb1e0c9e6be195f602071e (The 2.1.4 is a local bump to differentiate between the patched and system builds).

(In reply to comment #30) > (In reply to comment #29) > > Looks great! I'll try testing it out with uzbl tonight. Much appreciated for trying it out! It's really encouraging to see that this is likely to get some use and for the early working feedback! > > First, the headers are missing from the variable which has the list of headers to install (webkit2gtk_h_api?). > Well spotted, I didn't know about that variable. > So some differences I see compared to WebKit1 (which used libsoup directly) is that there's no indication of whether the authentication is for a proxy or not (looks to be accessible from WebCore::ProtectionSpaceServerType), and the paths which are affected by the request (which doesn't look like it's plumbed through WebCore at all). > I can add an _is_proxy() function. > Also, the port from the request is a gint; maybe it makes more sense as a guint (as the port method on WebKitSecurityOrigin uses)? I agree

(In reply to comment #31) > (In reply to comment #29) > > Looks great! I'll try testing it out with uzbl tonight. > > I'm seeing the following: > > (gdb) list > <snip> > 779 WebKitAuthenticationScheme scheme = webkit_authentication_request_get_scheme (request); > <snip> > (gdb) p scheme > $1 = WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN > (gdb) p request->priv->authenticationChallenge.m_ptr->m_coreAuthenticationChallenge.m_protectionSpace.m_authenticationScheme > $2 = WebCore::ProtectionSpaceAuthenticationSchemeHTTPBasic > > so it seems that the auth scheme is mismatching somewhere. Hmmm, I'll take a look at that > I also see m_previousFailureCount in there. Is it worth plumbing that through (at least as a "has this auth been tried before?" property)? Sure I'll add a _get_previous_failures() function

Comment on attachment 207532 [details] Patch + docs View in context: https://bugs.webkit.org/attachment.cgi?id=207532&action=review API looks good to me in general, I'm looking forward to seeing the unit tests, thanks! > Source/WebKit2/GNUmakefile.list.am:683 > Source/WebKit2/UIProcess/API/gtk/WebKitDownload.h \ Add also the public headers to webkit2gtk_h_api. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:25 > #include "WebCredential.h" This can be removed too now that is included by the header. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:26 > +#include "WebKitAuthenticationRequest.h" This is already included by the header. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:29 > +#include "WebKitCredential.h" > +#include "WebKitCredentialPrivate.h" WebKitCredential is already included by WebKitCredentialPrivate.h > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h:25 > +#include "WebKitCredential.h" Why do you need this one in the header? > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:42 > + * This #WebKitAuthenticationRequest object encapsulates the Don't use # in this particular case, since it links to this block, you are already in. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:97 > + * You should explain here that this can be FALSE if webkit doesn't support credential storing or private browsing is enabled. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:98 > + * Returns: %TRUE if the client should allow storage of credentials or %FALSE otherwise. As I said in previous review this is not accurate, the client can perfectly allow to store credentials if it implements the credential storage, this is just about whether webkit can do it. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:118 > + * Get the #WebKitCredential of the proposed authentication challenge. The client can use this > + * directly for authentication or construct their own #WebKitCredential. We should be clear that the proposed credential is what it was stored in a previous session. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:120 > + * Returns: (transfer full):a #WebKitCredential encapsulating credential details. :a -> : a > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:127 > + g_return_val_if_fail(WEBKIT_IS_AUTHENTICATION_REQUEST(request), 0); > + const WebCore::Credential& credential = request->priv->authenticationChallenge->proposedCredential()->core(); Leave an empty line here like in all other methods, jut for consistency. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:140 > + * Returns: The host that this authentication challenge is applicable to. Repeating the body here is not very useful, you should either remove the body or use a different sentence here, something like "the host of @request" or something similar. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:148 > + request->priv->host = request->priv->authenticationChallenge->protectionSpace()->host().utf8().data(); Remove the trailing .data() since .utf8() is already a CString > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:162 > +gint webkit_authentication_request_get_port(WebKitAuthenticationRequest* request) I think this could be unsigned, guint16 or just guint > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:183 > + request->priv->realm = request->priv->authenticationChallenge->protectionSpace()->realm().utf8().data(); Same here about the .data() > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:202 > + ProtectionSpaceAuthenticationScheme oldScheme = request->priv->authenticationChallenge->protectionSpace()->authenticationScheme(); Why is this old scheme? > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:212 > + scheme = static_cast<WebKitAuthenticationScheme>(oldScheme); You can just return here. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:214 > + scheme = WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN; Ditto. Why this special case instead of using ProtectionSpaceAuthenticationSchemeUnknown. I think we could just use the same values than WebCore and add compile time checks. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:220 > + Extra empty line here. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:71 > + Missing * here > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:73 > + */ Since: 2.2 > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:94 > +webkit_authentication_request_get_proposed_credential (WebKitAuthenticationRequest *request); Leave only one space between function name and arguments for the longest function > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:33 > + * @See_also: #WebKitWebView WebKitWebView? See also #WebKitAuthenticationRequest, I'd say > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:42 > + _WebKitCredential(const WebCore::Credential& core) Use coreCredential instead of jut core. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:94 > + delete credential; This is wrong, the credential hasn't been allocated with new, but with g_slice_new. You should call the destructor manually and free the memory with g_slice_free. credential->~WebKitCredential(); g_slice_free(WebKitCredential, credential); > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:101 > + * @persistence: The persistence of the new credential a #WebKitCredentialPersistence so that there will be a link here to the enum doc. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:102 > + * Extra empty line here. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:112 > +{ > + return webkitCredentialCreate(WebCore::Credential( You should add g_return macros to make sure that username and password are valid pointers > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:131 > + credential->username = credential->credential.user().utf8().data(); Remove the .data() here too. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:148 > + credential->password = credential->credential.password().utf8().data(); And here. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:183 > + return static_cast<WebKitCredentialPersistence>(credential->credential.persistence()); You should also add compile time checks to make sure this cast is always valid. See the macro COMPILE_ASSERT_MATCHING_ENUM in WebKitPrivate.h and examples of how to use it in files like WebKitError.cpp or WebKitNavigationPolicyDecision.cpp > Source/WebKit2/UIProcess/API/gtk/WebKitCredentialPrivate.h:24 > +#include "WebKitCredential.h" > +#include "WebKitPrivate.h" Dont' you need to include <WebCore/Credential.h> here? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1398 > + * To handle this signal asynchronously you should keep a ref > + * of the request and return TRUE. To disable HTTP authentication entirely, connect to this signal and simply return TRUE; > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-docs.sgml:62 > + <index id="api-index-2-2" role="2.2"> > + <title>Index of new symbols in 2.2</title> > + <xi:include href="xml/api-index-2.2.xml"><xi:fallback /></xi:include> > + </index> Thanks! > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:192 > +webkit_authentication_request_get_type This is considere private from the docs point of view, as is expected to be used with the standard macro WEBKIT_TYPE_AUTHENTICATION_REQUEST > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:205 > +<SUBSECTION Private> > +WebKitAuthenticationRequestPrivate > +</SECTION> webkit_authentication_request_get_type goes here too. > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:208 > +<SECTION> > +<FILE>WebKitCredential</FILE> This could be a subsection of the WebKitAuthenticationRequest section. > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:218 > +webkit_credential_new Move the new at the beginning.

(In reply to comment #33) > (In reply to comment #30) > > (In reply to comment #29) > > > Looks great! I'll try testing it out with uzbl tonight. > > Much appreciated for trying it out! It's really encouraging to see that this is likely to get some use and for the early working feedback! > > > > > First, the headers are missing from the variable which has the list of headers to install (webkit2gtk_h_api?). > > > > Well spotted, I didn't know about that variable. "Remember to add it to webkit2gtk_headers variable in ​GNUmakefile.am file" in http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API :-)

Created attachment 207661 [details] Patch + docs + unit test Proposal for the API + docs + unit test

Comment on attachment 207661 [details] Patch + docs + unit test I didn't see that Carlos just reviewed the previous patch proposal so I'm going to cancel this and incorporate his feedback before submitting a new patch. Sorry for the noise. Will hopefully submit the final patch tomorrow.

Comment on attachment 207661 [details] Patch + docs + unit test View in context: https://bugs.webkit.org/attachment.cgi?id=207661&action=review Looks pretty good to me. Just a few comments... > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:44 > + * > + * Whenever the user attempts to load a page protected by HTTP > + * authentication, the user will need to provide their credentials. > + * This #WebKitAuthenticationRequest object provides client side > + * authentication support and is emitted with the > + * #WebKitWebView::authenticate signal. For the WebKitPolicyDecision we talk about how to handle the request asynchronously. I think similar language here could be useful. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:211 > + scheme = static_cast<WebKitAuthenticationScheme>(psScheme - 1); /* ProtectionSpaceAuthScheme enum starts at 1 */ Doesn't C support manually assigning the enum values? I recommend just making them match up exactly and casting the value. Then you can use ASSERT_MATCHING_ENUM to prevent breaking it in the future. That will make this method a lot simpler. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:214 > + default: > + scheme = WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN; Seems unnecessary since ProtectionSpaceAuthenticationSchemeUnknown is already there. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:94 > + delete credential; I think it's an error to allocate memory with the slice allocator and then delete it. Instead I believe you should manually call the destructor and then deallocate it with the slice allocation API. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:115 > + return webkitCredentialCreate(WebCore::Credential( > + String::fromUTF8(username), > + String::fromUTF8(password), > + static_cast<WebCore::CredentialPersistence>(persistence))); this can be one line. :) > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:441 > + CredentialStorageMode credentialStorageMode; > + if (webkit_authentication_request_can_save_credentials(request)) > + credentialStorageMode = AllowPersistentStorage; > + else > + credentialStorageMode = DisallowPersistentStorage; I think I'd prefer an inline conditional here.

(In reply to comment #34) > (In reply to comment #31) > > so it seems that the auth scheme is mismatching somewhere. > > Hmmm, I'll take a look at that Some more information: Breakpoint 2, webkit_authentication_request_get_scheme (request=0xbde520) at ../Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:198 198 { (gdb) n 199 g_return_val_if_fail(WEBKIT_IS_AUTHENTICATION_REQUEST(request), WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN); (gdb) fin Run till exit from #0 webkit_authentication_request_get_scheme (request=0xbde520) at ../Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:199 0x0000000000410349 in authenticate_cb (view=0x6a63c0, request=0xbde520, data=0x0) at src/gui.c:779 so it looks like WEBKIT_IS_AUTHENTICATION_REQUEST(request) is failing… > > I also see m_previousFailureCount in there. Is it worth plumbing that through (at least as a "has this auth been tried before?" property)? > > Sure I'll add a _get_previous_failures() function Thanks!

(In reply to comment #40) > (In reply to comment #34) > > (In reply to comment #31) > > > so it seems that the auth scheme is mismatching somewhere. > > > > Hmmm, I'll take a look at that > > Some more information: > > Breakpoint 2, webkit_authentication_request_get_scheme (request=0xbde520) at ../Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:198 > 198 { > (gdb) n > 199 g_return_val_if_fail(WEBKIT_IS_AUTHENTICATION_REQUEST(request), WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN); > (gdb) fin > Run till exit from #0 webkit_authentication_request_get_scheme (request=0xbde520) at ../Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:199 > 0x0000000000410349 in authenticate_cb (view=0x6a63c0, request=0xbde520, data=0x0) at src/gui.c:779 > > so it looks like WEBKIT_IS_AUTHENTICATION_REQUEST(request) is failing… > I checked this in my unit test and it seems the problem was that the case was falling through to default (no break). In the latest patch it should work fine, but perhaps you want to hold off testing again until I submit a final patch. > > > I also see m_previousFailureCount in there. Is it worth plumbing that through (at least as a "has this auth been tried before?" property)? > > > > Sure I'll add a _get_previous_failures() function > > Thanks!

Comment on attachment 207532 [details] Patch + docs View in context: https://bugs.webkit.org/attachment.cgi?id=207532&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:202 >> + ProtectionSpaceAuthenticationScheme oldScheme = request->priv->authenticationChallenge->protectionSpace()->authenticationScheme(); > > Why is this old scheme? I was struggling to come up with a useful variable name. In the later patch I changed to psScheme, but suggestions are welcome. >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:214 >> + scheme = WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN; > > Ditto. Why this special case instead of using ProtectionSpaceAuthenticationSchemeUnknown. I think we could just use the same values than WebCore and add compile time checks. Agreed > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:253 > + request->priv->authenticationChallenge->listener()->useCredential(0); I know we've discussed this before but I propose to change this to so that the request->priv->authenticationChallenge->listener()->cancel(); so that the cancel() function is actually plumbed. For those what want to authenticate with no credentials, just pass 0 into _authenticate().

Created attachment 207731 [details] Patch + docs + unit test Final patch + docs + unit test?

(In reply to comment #43) > Created an attachment (id=207731) [details] > Patch + docs + unit test > > Final patch + docs + unit test? Thanks very much for all the reviews, feedback and comments. I believe I've addressed everything so this could well be the final patch if you want to take a last look.

(In reply to comment #44) > Thanks very much for all the reviews, feedback and comments. I believe I've addressed everything so this could well be the final patch if you want to take a last look. Things look good here. I am wondering how to get an HTML form to fire an authentication signal, but it seems that nothing in WebKit2 actually uses it yet, so that seems to be separate. Thanks!

Comment on attachment 207731 [details] Patch + docs + unit test View in context: https://bugs.webkit.org/attachment.cgi?id=207731&action=review API looks good to me in general, other reviewer should approve it too. Code looks great too, I have just some minor comments. The main problem is that the unit test is very poor. If possible, we should check all API methods, the most important ones, authenticate and cancel are not even tested. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:57 > + webkit_authentication_request_authenticate(authDialog->priv->request.get(), 0); > } I think we could destroy the dialog here, for consistency with the ok button approach. We can even get rid of the helper methods, since they are used only once in the callbacks. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:126 > + * Returns: (transfer full): A #WebKitCredential encapsulating credential details. or %NULL if there isn't any previous credential saved. It's important to document this can return NULL. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:216 > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_DEFAULT, ProtectionSpaceAuthenticationSchemeDefault); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_HTTP_BASIC, ProtectionSpaceAuthenticationSchemeHTTPBasic); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_HTTP_DIGEST, ProtectionSpaceAuthenticationSchemeHTTPDigest); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_HTML_FORM, ProtectionSpaceAuthenticationSchemeHTMLForm); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_NTLM, ProtectionSpaceAuthenticationSchemeNTLM); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_NEGOTIATE, ProtectionSpaceAuthenticationSchemeNegotiate); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_CLIENT_CERTIFICATE_REQUESTED, ProtectionSpaceAuthenticationSchemeClientCertificateRequested); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_SERVER_TRUST_EVALUATION_REQUESTED, ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN, ProtectionSpaceAuthenticationSchemeUnknown); We normally add these checks at the beginning of the file, right before the methods, not inside a function, this makes the function more difficult to read. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:247 > +guint webkit_authentication_request_get_previous_failures(WebKitAuthenticationRequest* request) This returns the amount of previous failures, so I would call this either webkit_authentication_request_get_n_previous_failures or webkit_authentication_request_get_previous_failure_count. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:257 > + * @credential: (transfer none):a #WebKitCredential :a -> : a You should also add a (allow-none) annotation and say, a #WebKitCredential or %NULL > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:260 > + * supplied. To authenticate without credentials, pass NULL as the credential. "authenticate without credentials" sounds confusing to me. I would say, "To continue without credentials pass %NULL as @credential" > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:268 > + if (credential) { Leave only one spaces between ) and {. The style checker should probably check this. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:34 > +#define WEBKIT_TYPE_AUTHENTICATION_REQUEST (webkit_authentication_request_get_type()) > +#define WEBKIT_AUTHENTICATION_REQUEST(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_AUTHENTICATION_REQUEST, WebKitAuthenticationRequest)) Line up macros, please. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:74 > + * Since 2.2 Since: 2.2 > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:64 > + * Since 2.2 Since: 2.2 > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:70 > + WebKitCredential* copy = g_slice_new(WebKitCredential); > + new (copy) WebKitCredential(*credential); > + return copy; Maybe it would be simpler to simply return webkitCredentialCreate(credential->credential); > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:75 > + * @credential: (transfer full): A #WebKitCredential Don't use the transfer full annotation in this case. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:176 > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_CREDENTIAL_PERSISTENCE_NONE, WebCore::CredentialPersistenceNone); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_CREDENTIAL_PERSISTENCE_FOR_SESSION, WebCore::CredentialPersistenceForSession); > + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_CREDENTIAL_PERSISTENCE_PERMANENT, WebCore::CredentialPersistencePermanent); Move this out of this function too, please. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.h:44 > + * Since 2.2 Since: 2.2 > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.h:63 > +WEBKIT_API WebKitCredential * > +webkit_credential_new (const gchar *username, > + const gchar *password, Nit: the _new method is usually the first one, could you please move it? > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:220 > +WEBKIT_TYPE_CREDENTIAL Move this to the other Standard subsection > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:221 > +webkit_credential_get_type And this to the private one > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:825 > + const gchar *host = webkit_authentication_request_get_host(request); > + guint port = webkit_authentication_request_get_port(request); You get these but they are not checked at all. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:826 > + const gchar *realm = webkit_authentication_request_get_realm(request); Don't need to cache this, you can directly add g_assert_cmpstr(webkit_authentication_request_get_realm(request), ==, "my realm"); > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:827 > + WebKitAuthenticationScheme scheme = webkit_authentication_request_get_scheme(request); Ditto. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:828 > + gboolean isProxy = webkit_authentication_request_is_for_proxy(request); Ditto, g_assert(!webkit_authentication_request_is_for_proxy(request)); > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:829 > + guint previousFailures = webkit_authentication_request_get_previous_failures(request); This is not checked either.

Comment on attachment 207731 [details] Patch + docs + unit test View in context: https://bugs.webkit.org/attachment.cgi?id=207731&action=review Thanks for the review! >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:57 >> } > > I think we could destroy the dialog here, for consistency with the ok button approach. We can even get rid of the helper methods, since they are used only once in the callbacks. Sounds good to me. >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:126 >> + * Returns: (transfer full): A #WebKitCredential encapsulating credential details. > > or %NULL if there isn't any previous credential saved. It's important to document this can return NULL. Good point >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:247 >> +guint webkit_authentication_request_get_previous_failures(WebKitAuthenticationRequest* request) > > This returns the amount of previous failures, so I would call this either webkit_authentication_request_get_n_previous_failures or webkit_authentication_request_get_previous_failure_count. Ok, I'll go for webkit_authentication_request_get_previous_failure_count since it ties in with the function that it calls >> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:70 >> + return copy; > > Maybe it would be simpler to simply return webkitCredentialCreate(credential->credential); Good idea. >> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.h:63 >> + const gchar *password, > > Nit: the _new method is usually the first one, could you please move it? Sure

Created attachment 207922 [details] Rebased to master, addressed all comments and improved unit test

I'm happy with this API too!

Comment on attachment 207922 [details] Rebased to master, addressed all comments and improved unit test View in context: https://bugs.webkit.org/attachment.cgi?id=207922&action=review Looks great, there are still some minor issues and nits. You might consider to split the unit test in several test cases too, making sure they are independent to each other. For example "authentication-request" to check WebKitAuthentication values (port, host, scheme, can_save_credentials, etc) "authentication-cancel" to check cancellation, "authentication-failure" to check failures and failure counts, "authentication-no-credential" to check continue the load without credentials and "authetication-success" to check successful auth and subsequent loads of the same auth uri. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:23 > +#include "AuthenticationChallengeProxy.h" This is already inclucded by WebKitAuthenticationRequestPrivate.h > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:51 > + * In case the client application does not wish > + * to handle this signal WebKit will provide a default handler. To handle > + * authentication asynchronously, simply increase the reference count of the > + * WebKitAuthenticationRequest object. I think this should be in the signal documentation. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:211 > + * Returns: The authentication scheme of @request. a #WebKitAuthenticationScheme .... this way there's a link to the WebKitAuthenticationScheme to know what this is about. This is important, because after get_host and get_port, get_scheme sounds like a protocol name. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:259 > + * @credential: (transfer none) (allow-none): A #WebKitCredential A #WebKitCredential, or %NULL > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:274 > + if (credential) { > + RefPtr<WebCredential> webCredential = WebCredential::create(webkitCredentialGetCredential(credential)); > + request->priv->authenticationChallenge->listener()->useCredential(webCredential.get()); > + } else > + request->priv->authenticationChallenge->listener()->useCredential(0); I think this could be simplified as something like: RefPtr<WebCredential> webCredential = credential ? WebCredential::create(webkitCredentialGetCredential(credential)) : 0; request->priv->authenticationChallenge->listener()->useCredential(webCredential.get()); > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:92 > + g_assert(credential); Do not use g_assert here, this is public API, use g_return_val_if_fail() instead. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:107 > +{ > + credential->~WebKitCredential(); You could add a g_return macro here too, or return early if credential is NULL > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1394 > + * of the request and return TRUE. To disable HTTP authentication %TRUE > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:839 > + // Require authentication Nit: Finish comments with period. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:843 > + soup_message_body_append(message->response_body, SOUP_MEMORY_COPY, authSuccessHTMLString, strlen(authSuccessHTMLString)); authSuccessHTMLString is a static var, we don't need to copy it, use SOUP_MEMORY_STATIC instead. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:850 > + // Authentication not successful, display a "401 Authorization Required" page Nit: Finish comments with period. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:852 > + soup_message_body_append(message->response_body, SOUP_MEMORY_COPY, authFailureHTMLString, strlen(authFailureHTMLString)); authFailureHTMLString is a static var, we don't need to copy it, use SOUP_MEMORY_STATIC instead. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:854 > + Remove this empty line. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:863 > + // reset the retry count of the fake server when a page is loaded reset -> Reset. Finish the comment with period. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:896 > + kServer = new WebKitTestServer(); > + kServer->run(test->serverCallback); If this is going to be used only by this test, it doesn't need to be a global variable. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:898 > + // test no stored credentials Please fix all comments, see http://www.webkit.org/coding/coding-style.html#comments-sentences > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:906 > + // don't test port, it is randomly assigned by libsoup in WebKitTestServer You can test it, use kServer->baseURI() that returns a SoupURI that you can use to query both the port and the host instead of hardcoding 127.0.0.1. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:910 > + g_assert(!webkit_authentication_request_get_previous_failure_count(request)); Use g_assert_cmpuint in this case > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:913 > + g_assert(!webkit_authentication_request_can_save_credentials(request)); This is confusing here, I would test this in a separate comment. // Test credentials can't be saved when private brwosing is enabled. webkit_settings_set_enable_private_browsing(webkit_web_view_get_settings(test->m_webView), TRUE); g_assert(!webkit_authentication_request_can_save_credentials(request)); > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:920 > + // test cancel > + webkit_authentication_request_cancel(request); > + test->waitUntilLoadFinished(); > + g_assert(!webkit_web_view_get_title(test->m_webView)); What exactly happens in this case? How does the load finish? load failed is emitted with cancel error? I that's is the case we should test that, connect to load-failed and check the given error is cancel. This behaviour should probably be documented in the webkit_authentication_request_cancel API doc too. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:923 > + webkit_settings_set_enable_private_browsing(settings, FALSE); I would move this to the block that checks whether credentials can be stored, cheking that after setting this to FALSE webkit_authentication_request_can_save_credentials(request) returns TRUE. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:930 > + // doesn't ask for new credentials > + test->waitUntilLoadFinished(); > + g_assert(!webkit_web_view_get_title(test->m_webView)); Shouldn't it be "401 Authorization Required" when continuing without credentials? > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:940 > + g_assert(webkit_authentication_request_get_previous_failure_count(request) == 1); Use g_assert_cmpuint > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1405 > + AuthenticationTest::add("WebKitWebView", "authentication", testWebViewAuthenticationRequest); Any reason you added this at the beginning? It doesn't really matter, but we usually add new tests at the end

Comment on attachment 207922 [details] Rebased to master, addressed all comments and improved unit test View in context: https://bugs.webkit.org/attachment.cgi?id=207922&action=review Couple little things... > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:202 > + request->priv->realm = request->priv->authenticationChallenge->protectionSpace()->realm().utf8(); > + return request->priv->realm.data(); The realm doesn't change, so I think it makes sense to do this: if (request->priv->realm.isNull()) request->priv->realm = request->priv->authenticationChallenge->protectionSpace()->realm().utf8(); This will prevent an application that caches the realm from crashing if something else calls the method again. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:254 > +guint webkit_authentication_request_get_previous_failure_count(WebKitAuthenticationRequest* request) > +{ > + g_return_val_if_fail(WEBKIT_IS_AUTHENTICATION_REQUEST(request), 0); > + > + return request->priv->authenticationChallenge->previousFailureCount(); > +} We can't really expose this properly. The reason is that the authentication request we get from soup only exposes whether or not the challenge is a retry. This will always be one or zero, regardless of how many times it has failed. So I think you should either remove this method or turn it into webkit_authentication_request_is_retry. See AuthenticationChallenge::AuthenticationChallenge in AuthenticationChallengeSoup.cpp. > Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:125 > + credential->username = credential->credential.user().utf8(); Same comment here and for password about the realm above.

Comment on attachment 207922 [details] Rebased to master, addressed all comments and improved unit test View in context: https://bugs.webkit.org/attachment.cgi?id=207922&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:51 >> + * WebKitAuthenticationRequest object. > > I think this should be in the signal documentation. Since all this information is already in the signal documentation I'll just remove this paragraph. >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:202 >> + return request->priv->realm.data(); > > The realm doesn't change, so I think it makes sense to do this: > > if (request->priv->realm.isNull()) > request->priv->realm = request->priv->authenticationChallenge->protectionSpace()->realm().utf8(); > > This will prevent an application that caches the realm from crashing if something else calls the method again. I didn't realise calling it again would crash, so thanks for spotting that. >> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:254 >> +} > > We can't really expose this properly. The reason is that the authentication request we get from soup only exposes whether or not the challenge is a retry. This will always be one or zero, regardless of how many times it has failed. So I think you should either remove this method or turn it into webkit_authentication_request_is_retry. > > See AuthenticationChallenge::AuthenticationChallenge in AuthenticationChallengeSoup.cpp. I realised that when I was writing the unit test and its been on my mind. EFL exposes a is_retry function using soup underneath as well. >> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:920 >> + g_assert(!webkit_web_view_get_title(test->m_webView)); > > What exactly happens in this case? How does the load finish? load failed is emitted with cancel error? I that's is the case we should test that, connect to load-failed and check the given error is cancel. This behaviour should probably be documented in the webkit_authentication_request_cancel API doc too. Will do, thats a really good point. >> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1405 >> + AuthenticationTest::add("WebKitWebView", "authentication", testWebViewAuthenticationRequest); > > Any reason you added this at the beginning? It doesn't really matter, but we usually add new tests at the end I thought it was meant to be alphabetical...

Comment on attachment 207922 [details] Rebased to master, addressed all comments and improved unit test View in context: https://bugs.webkit.org/attachment.cgi?id=207922&action=review >>> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:51 >>> + * WebKitAuthenticationRequest object. >> >> I think this should be in the signal documentation. > > Since all this information is already in the signal documentation I'll just remove this paragraph. Carlos, it's probably fine to leave it in both places, no?

(In reply to comment #53) > (From update of attachment 207922 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207922&action=review > > >>> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:51 > >>> + * WebKitAuthenticationRequest object. > >> > >> I think this should be in the signal documentation. > > > > Since all this information is already in the signal documentation I'll just remove this paragraph. > > Carlos, it's probably fine to leave it in both places, no? There's a link to the signal documentation there. It looked weird to me there when I read it, but if you think it's useful there, it's fine with me.

Created attachment 208316 [details] Patch + docs + unit test vs

(In reply to comment #55) > Created an attachment (id=208316) [details] > Patch + docs + unit test vs I've gone through and addressed all the comments, including splitting the unit test into 5 separate tests and testing load sequences by inheriting from LoadTrackingTest as Carlos suggested.

Comment on attachment 208316 [details] Patch + docs + unit test vs View in context: https://bugs.webkit.org/attachment.cgi?id=208316&action=review Looks good to me, thank you very much. > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:282 > + * #WebKitWebView::load-failed signal with a #WebKitNetworkError of type #WEBKIT_NETWORK_ERROR_CANCELLED being emitted. #WEBKIT_NETWORK_ERROR_CANCELLED -> %WEBKIT_NETWORK_ERROR_CANCELLED > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1395 > + * entirely, connect to this signal and simply return %TRUE; s/;/./ > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1275 > + if (authorization && !strcmp(authorization, authExpectedAuthorization)) { You can use g_strcmp0 that hanldes NULL so that you don't need to check first if authorization != NULL. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1332 > + static WebKitTestServer* kServer = new WebKitTestServer(); > + kServer->run(test->serverCallback); You can make the server global like all other tests do, so that you don't need to create/delete the test for every test case. It will be run even if you launch tests that don't need it, but it's harmless. We could consider add a ensureWebKitTestServer() to start the server on demand, though, but it would be a separate patch. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1347 > + g_assert(!webkit_authentication_request_can_save_credentials(request)); You should also check this is TRUE when private browsing is FALSE and webkit has been compiled with libsecret. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1382 > + WebKitAuthenticationRequest* request = test->waitForAuthenticationRequest(); > + WebKitCredential* credential = webkit_credential_new(authTestUsername, "wrongpassword", WEBKIT_CREDENTIAL_PERSISTENCE_NONE); You could test here that the first time is_retry is FALSE.

Created attachment 208349 [details] Final patch

Comment on attachment 208349 [details] Final patch View in context: https://bugs.webkit.org/attachment.cgi?id=208349&action=review All changes have been applied - Carlos could you take a last look if they're ok and cq+? > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1428 > + if (authorization && !strcmp(authorization, authExpectedAuthorization)) { I tried g_strcmp0, but since authExpectedAuthorization is never NULL it always returns true and never gets through to sending a challenge.

(In reply to comment #59) > (From update of attachment 208349 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208349&action=review > > All changes have been applied - Carlos could you take a last look if they're ok and cq+? > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1428 > > + if (authorization && !strcmp(authorization, authExpectedAuthorization)) { > > I tried g_strcmp0, but since authExpectedAuthorization is never NULL it always returns true and never gets through to sending a challenge. g_strcmp0 doesn't return TRUE/False, but -1, 0, 1. It handles NULL by considering it lower than non NULL strings, but it will only return 0 if both are NULL or non-NULL and equal.

Created attachment 208408 [details] Final final patch

Comment on attachment 208408 [details] Final final patch Clearing flags on attachment: 208408 Committed r153882: <http://trac.webkit.org/changeset/153882>

All reviewed patches have been landed. Closing bug.