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

[bugzilla-daemon at webkit.org]

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





--- Comment #39 from Martin Robinson <mrobinson at webkit.org>  2013-07-29 10:16:16 PST ---
(From update of attachment 207661)
View in context: https://bugs.webkit.org/attachment.cgi?id=207661&action=review

Looks pretty good to me. Just a few comments...

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:44
> + *
> + * Whenever the user attempts to load a page protected by HTTP
> + * authentication, the user will need to provide their credentials.
> + * This #WebKitAuthenticationRequest object provides client side
> + * authentication support and is emitted with the
> + * #WebKitWebView::authenticate signal.

For the WebKitPolicyDecision we talk about how to handle the request asynchronously. I think similar language here could be useful.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:211
> +        scheme = static_cast<WebKitAuthenticationScheme>(psScheme - 1); /* ProtectionSpaceAuthScheme enum starts at 1 */

Doesn't C support manually assigning the enum values? I recommend just making them match up exactly and casting the value. Then you can use ASSERT_MATCHING_ENUM to prevent breaking it in the future. That will make this method a lot simpler.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationRequest.cpp:214
> +    default:
> +        scheme = WEBKIT_AUTHENTICATION_SCHEME_UNKNOWN;

Seems unnecessary since ProtectionSpaceAuthenticationSchemeUnknown is already there.

> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:94
> +    delete credential;

I think it's an error to allocate memory with the slice allocator and then delete it. Instead I believe you should manually call the destructor and then deallocate it with the slice allocation API.

> Source/WebKit2/UIProcess/API/gtk/WebKitCredential.cpp:115
> +    return webkitCredentialCreate(WebCore::Credential(
> +        String::fromUTF8(username),
> +        String::fromUTF8(password),
> +        static_cast<WebCore::CredentialPersistence>(persistence)));

this can be one line. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:441
> +    CredentialStorageMode credentialStorageMode;
> +    if (webkit_authentication_request_can_save_credentials(request))
> +        credentialStorageMode = AllowPersistentStorage;
> +    else
> +        credentialStorageMode = DisallowPersistentStorage;

I think I'd prefer an inline conditional here.

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