[webkit-reviews] review granted: [Bug 100775] [GTK] Remove dependency on SoupPasswordManager : [Attachment 171676] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 100775: [GTK] Remove dependency on SoupPasswordManager
https://bugs.webkit.org/show_bug.cgi?id=100775

Attachment 171676: Patch
https://bugs.webkit.org/attachment.cgi?id=171676&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171676&action=review


This patch looks sane to me. Feel free to have a GTK expert review this patch
if you want a more thorough review.

> Source/WebCore/ChangeLog:10
> +	   CredentialStoreGtk. The name is based on the name of a similar class
from the Blackberry port.

Nit: Blackberry => BlackBerry

> Source/WebCore/ChangeLog:12
> +	   No new tests. This should not change behavior.

Your use of the word "should" gives the impression that you are unsure whether
this change may alter existing behavior. It would be beneficial to elaborate on
any uncertainties.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:197
> +    // This is just a temporary kludge until GtkAuthenticationDialog works
directly with AuthenticationChallenges.

You may want to consider filing a bug (if one doesn't already exist (*)) and
referencing the bug in this comment for removing this kludge so as to make it
actionable.

(*) If there is an existing bug for this issue then we should update this
comment to reference it.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:237
> +	   // This is just a temporary kludge until GtkAuthenticationDialog
works directly with AuthenticationChallenges.

Ditto.

> Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:53
> +CredentialBackingStore::~CredentialBackingStore()
> +{
> +}

I take it you either plan to have this class to do something non-trivial in the
destructor or explicitly don't want the default destructor inlined? If neither
then I suggest we remove this implementation and the declaration in
CredentialBackingStore.h as the compiler will generate	such a destructor.

> Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:81
> +	   0, // cancellable

Nit: cancellable => cancelable

> Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:112
> +	   0, // cancellable

Ditto.

> Source/WebCore/platform/network/gtk/CredentialBackingStore.h:35
> +class CredentialBackingStore {

Can we make this non-copyable?

> Source/WebCore/platform/network/gtk/CredentialBackingStore.h:40
> +    Credential getCredentialForChallenge(const AuthenticationChallenge&);

We tend to only use the prefix "get" for functions that have out arguments.
Maybe credentialForChallenge?