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

Tue Aug 6 23:48:39 PDT 2013

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

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #207922|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #50 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-08-06 23:48:18 PST ---
(From update of attachment 207922)
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

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