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

[bugzilla-daemon at webkit.org]

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 207731: Patch + docs + unit test
https://bugs.webkit.org/attachment.cgi?id=207731&action=review

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


API looks good to me in general, other reviewer should approve it too. Code
looks great too, I have just some minor comments. The main problem is that the
unit test is very poor. If possible, we should check all API methods, the most
important ones, authenticate and cancel are not even tested.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:57
> +   
webkit_authentication_request_authenticate(authDialog->priv->request.get(), 0);

>  }

I think we could destroy the dialog here, for consistency with the ok button
approach. We can even get rid of the helper methods, since they are used only
once in the callbacks.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:126
> + * Returns: (transfer full): A #WebKitCredential encapsulating credential
details.

or %NULL if there isn't any previous credential saved. It's important to
document this can return NULL.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:216
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_DEFAULT,
ProtectionSpaceAuthenticationSchemeDefault);
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_HTTP_BASIC,
ProtectionSpaceAuthenticationSchemeHTTPBasic);
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_HTTP_DIGEST,
ProtectionSpaceAuthenticationSchemeHTTPDigest);
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_HTML_FORM,
ProtectionSpaceAuthenticationSchemeHTMLForm);
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_NTLM,
ProtectionSpaceAuthenticationSchemeNTLM);
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_NEGOTIATE,
ProtectionSpaceAuthenticationSchemeNegotiate);
> +   
COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_CLIENT_CERTIFICATE_RE
QUESTED, ProtectionSpaceAuthenticationSchemeClientCertificateRequested);
> +   
COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_SERVER_TRUST_EVALUATI
ON_REQUESTED,
ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested);
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN,
ProtectionSpaceAuthenticationSchemeUnknown);

We normally add these checks at the beginning of the file, right before the
methods, not inside a function, this makes the function more difficult to read.


> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:247
> +guint
webkit_authentication_request_get_previous_failures(WebKitAuthenticationRequest
* request)

This returns the amount of previous failures, so I would call this either
webkit_authentication_request_get_n_previous_failures or
webkit_authentication_request_get_previous_failure_count.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:257
> + * @credential: (transfer none):a #WebKitCredential

:a -> : a

You should also add a (allow-none) annotation and say, a #WebKitCredential or
%NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:260
> + * supplied. To authenticate without credentials, pass NULL as the
credential.

"authenticate without credentials" sounds confusing to me. I would say, "To
continue without credentials pass %NULL as @credential"

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:268
> +    if (credential)	   {

Leave only one spaces between ) and {. The style checker should probably check
this.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:34
> +#define WEBKIT_TYPE_AUTHENTICATION_REQUEST  
(webkit_authentication_request_get_type())
> +#define WEBKIT_AUTHENTICATION_REQUEST(obj)		
(G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_AUTHENTICATION_REQUEST,
WebKitAuthenticationRequest))

Line up macros, please.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.h:74
> + * Since 2.2

Since: 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:64
> + * Since 2.2

Since: 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:70
> +    WebKitCredential* copy = g_slice_new(WebKitCredential);
> +    new (copy) WebKitCredential(*credential);
> +    return copy;

Maybe it would be simpler to simply return
webkitCredentialCreate(credential->credential);

> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:75
> + * @credential: (transfer full): A #WebKitCredential

Don't use the transfer full annotation in this case.

> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:176
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_CREDENTIAL_PERSISTENCE_NONE,
WebCore::CredentialPersistenceNone);
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_CREDENTIAL_PERSISTENCE_FOR_SESSION,
WebCore::CredentialPersistenceForSession);
> +    COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_CREDENTIAL_PERSISTENCE_PERMANENT,
WebCore::CredentialPersistencePermanent);

Move this out of this function too, please.

> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.h:44
> + * Since 2.2

Since: 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.h:63
> +WEBKIT_API WebKitCredential *
> +webkit_credential_new		  (const gchar		      
*username,
> +					   const gchar		      
*password,

Nit: the _new method is usually the first one, could you please move it?

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:220
> +WEBKIT_TYPE_CREDENTIAL

Move this to the other Standard subsection

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:221
> +webkit_credential_get_type

And this to the private one

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:825
> +	   const gchar *host = webkit_authentication_request_get_host(request);

> +	   guint port = webkit_authentication_request_get_port(request);

You get these but they are not checked at all.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:826
> +	   const gchar *realm =
webkit_authentication_request_get_realm(request);

Don't need to cache this, you can directly add
g_assert_cmpstr(webkit_authentication_request_get_realm(request), ==, "my
realm");

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:827
> +	   WebKitAuthenticationScheme scheme =
webkit_authentication_request_get_scheme(request);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:828
> +	   gboolean isProxy =
webkit_authentication_request_is_for_proxy(request);

Ditto, g_assert(!webkit_authentication_request_is_for_proxy(request));

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:829
> +	   guint previousFailures =
webkit_authentication_request_get_previous_failures(request);

This is not checked either.