[Webkit-unassigned] [Bug 99352] [GTK] [WebKit2] Add an 'authenticate' signal to WebKitWebView

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99352





--- Comment #19 from Mario Sanchez Prada <mario at webkit.org>  2013-07-23 11:42:31 PST ---
(From update of attachment 207310)
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! :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.