[Webkit-unassigned] [Bug 99352] [GTK] [WebKit2] Add an 'authenticate' signal to WebKitWebView
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jul 20 06:50:28 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=99352
--- Comment #14 from Carlos Garcia Campos <cgarcia at igalia.com> 2013-07-20 06:50:22 PST ---
(From update of attachment 207193)
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.
--
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