[Webkit-unassigned] [Bug 99352] [GTK] [WebKit2] Add an 'authenticate' signal to WebKitWebView
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 31 02:23:50 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=99352
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #207731|review?, commit-queue? |review-, commit-queue-
Flag| |
--- Comment #46 from Carlos Garcia Campos <cgarcia at igalia.com> 2013-07-31 02:23:34 PST ---
(From update of attachment 207731)
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_REQUESTED, ProtectionSpaceAuthenticationSchemeClientCertificateRequested);
> + COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_AUTHENTICATION_SCHEME_SERVER_TRUST_EVALUATION_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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list