[Webkit-unassigned] [Bug 99352] [GTK] [WebKit2] Add an 'authenticate' signal to WebKitWebView
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 24 00:12:33 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=99352
--- Comment #20 from Carlos Garcia Campos <cgarcia at igalia.com> 2013-07-24 00:12:23 PST ---
(From update of attachment 207310)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list