[webkit-reviews] review granted: [Bug 99352] [GTK] [WebKit2] Add an 'authenticate' signal to WebKitWebView : [Attachment 208316] Patch + docs + unit test vs

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted 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 208316: Patch + docs + unit test vs
https://bugs.webkit.org/attachment.cgi?id=208316&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=208316&action=review


Looks good to me, thank you very much.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:282
> + * #WebKitWebView::load-failed signal with a #WebKitNetworkError of type
#WEBKIT_NETWORK_ERROR_CANCELLED being emitted.

#WEBKIT_NETWORK_ERROR_CANCELLED -> %WEBKIT_NETWORK_ERROR_CANCELLED

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1395
> +	* entirely, connect to this signal and simply return %TRUE;

s/;/./

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1275
> +	       if (authorization && !strcmp(authorization,
authExpectedAuthorization)) {

You can use g_strcmp0 that hanldes NULL so that you don't need to check first
if authorization != NULL.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1332
> +    static WebKitTestServer* kServer = new WebKitTestServer();
> +    kServer->run(test->serverCallback);

You can make the server global like all other tests do, so that you don't need
to create/delete the test for every test case. It will be run even if you
launch tests that don't need it, but it's harmless. We could consider add a
ensureWebKitTestServer() to start the server on demand, though, but it would be
a separate patch.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1347
> +    g_assert(!webkit_authentication_request_can_save_credentials(request));

You should also check this is TRUE when private browsing is FALSE and webkit
has been compiled with libsecret.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1382
> +    WebKitAuthenticationRequest* request =
test->waitForAuthenticationRequest();
> +    WebKitCredential* credential = webkit_credential_new(authTestUsername,
"wrongpassword", WEBKIT_CREDENTIAL_PERSISTENCE_NONE);

You could test here that the first time is_retry is FALSE.