[webkit-reviews] review denied: [Bug 99352] [GTK] [WebKit2] Add an 'authenticate' signal to WebKitWebView : [Attachment 207922] Rebased to master, addressed all comments and improved unit test

Tue Aug 6 23:48:37 PDT 2013

Carlos Garcia Campos <cgarcia at igalia.com> has denied Brian Holt
<brian.holt at samsung.com>'s request for review:
Bug 99352: [GTK] [WebKit2] Add an 'authenticate' signal to WebKitWebView
https://bugs.webkit.org/show_bug.cgi?id=99352

Attachment 207922: Rebased to master, addressed all comments and improved unit
test
https://bugs.webkit.org/attachment.cgi?id=207922&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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