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

Mon Jul 29 09:43:34 PDT 2013

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

--- Comment #35 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-07-29 09:43:20 PST ---
(From update of attachment 207532)
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.

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