Bug 100775 – [GTK] Remove dependency on SoupPasswordManager

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

We should remove the dependency on SoupPasswordManager in WebKitGTK+ and use libsecret directly. This is one step in the direction of full-fledged support for authentication.

[Martin Robinson]

Created attachment 171542 [details]
Patch

[WebKit Review Bot]

Attachment 171542 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:64:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #2)

> If any of these errors are false positives, please file a bug against check-webkit-style.

The NULL is necessary here to avoid issues with varargs.

[kov's GTK+ EWS bot]

Comment on attachment 171542 [details]
Patch

Attachment 171542 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14647056

[Martin Robinson]

Looks like the EWS needs libsecret-1.

[kov's GTK+ EWS bot]

Comment on attachment 171542 [details]
Patch

Attachment 171542 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14660034

[Carlos Garcia Campos]

Comment on attachment 171542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171542&action=review

This looks great, the only problem I see is that I think we should use the async API of libsecret. I guess we need to install libsecret in EWS and the bots, should we update the jhbuild moduleset too?

> Source/WebCore/ChangeLog:11
> +        a dependency on SoupPasswordManager.

I guess you forgot to remove this line.

> Source/WebKit/gtk/ChangeLog:9
> +        Add a libsecret dependency to the build. This is necessary so that we can remove
> +        a dependency on SoupPasswordManager.

Note that we don't have a hard dependency on SoupPasswordManager, so maybe libsecret should also be used optionally. We could add ifdefs to the CredentialBackingStore implementation or make a specific class for libsecret similar to the geoclue geolocation manager class.

> Source/WebCore/GNUmakefile.list.am:4740
> +	Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp \
> +	Source/WebCore/platform/network/gtk/CredentialBackingStore.h \

Maybe we could use a different name for the directory than gtk, since libsecret doesn't depend on gtk, like we did for geoclue code. Libsecret uses glib to implement the freedesktop secrets spec (I think) so it could be used by other ports that depend on glib, I'm not sure though. In any case it's just a name, we could leave gtk and just renamed it later if needed.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:200
> -    bool haveSavedLogin = getSavedLogin(m_auth.get(), &username, &password);
>      soup_session_pause_message(m_session, m_message.get());
> -    gtk_entry_set_text(GTK_ENTRY(m_loginEntry), username ? username : "");
> -    gtk_entry_set_text(GTK_ENTRY(m_passwordEntry), password ? password : "");
> -    if (m_rememberCheckButton && haveSavedLogin)
> +
> +    // This is just a temporary kludge until GtkAuthenticationDialog works directly with AuthenticationChallenges.
> +    AuthenticationChallenge challenge(0 /* session */, m_message.get(), m_auth.get(), false, 0);
> +    Credential savedCredential = credentialBackingStore().getCredentialForChallenge(challenge);
> +    if (!savedCredential.isEmpty()) {

At this point in previous code, soup had already the users and passwords cached in hash tables, so that soup_auth_get_saved_users() and soup_auth_get_saved_password() can be used synchornously, but now we are using the sync api of libsecret that blocks the main loop until the service is created (the first time) and the users/passwords are retrieved. CredentialBackingStore API should be fully async, like the previous soup API, so that we don't show the dialog until the credential has been retrieved.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:243
> -    if (m_message.get()->status_code != 401 && m_message.get()->status_code < 500)
> -        soup_auth_save_password(m_auth.get(), m_username.data(), m_password.data());
> +    if (m_message.get()->status_code != 401 && m_message.get()->status_code < 500) {
> +        // This is just a temporary kludge until GtkAuthenticationDialog works directly with AuthenticationChallenges.
> +        AuthenticationChallenge challenge(0 /* session */, m_message.get(), m_auth.get(), false, 0);
> +        Credential credentialToSave = Credential(
> +            String::fromUTF8(m_username.data()),
> +            String::fromUTF8(m_password.data()),
> +            CredentialPersistencePermanent);
> +        credentialBackingStore().storeCredentialsForChallenge(challenge, credentialToSave);

Same here, this blocks the main loop, but we are not even interested in the result of the operation.

> Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:81
> +    GOwnPtr<GList> elements(secret_service_search_sync(
> +        0, // The default SecretService.
> +        SECRET_SCHEMA_COMPAT_NETWORK,
> +        createAttributeHashTableFromChallenge(challenge).get(),
> +        static_cast<SecretSearchFlags>(SECRET_SEARCH_UNLOCK | SECRET_SEARCH_LOAD_SECRETS), // The default behavior is to only return the most recent item.
> +        0, // cancellable
> +        &error.outPtr()));

I think we should use the async API.

> Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:113
> +    secret_service_store_sync(
> +        0, // The default SecretService.
> +        SECRET_SCHEMA_COMPAT_NETWORK,
> +        createAttributeHashTableFromChallenge(challenge, credential).get(),
> +        SECRET_COLLECTION_DEFAULT,
> +        _("WebKitGTK+ password"),
> +        newSecretValue.get(),
> +        0, // cancellable
> +        &error.outPtr());

Ditto. In this case we could just pass NULL for the async ready callback, since we are not interested in the result of the operation.

> Source/WebCore/platform/network/gtk/CredentialBackingStore.h:34
> +class KURL;

Is KURL needed here? Could we use forward declarations of AuthenticationChallenge and Credential here too instead of including the headers?

[Dan Winship]

(In reply to comment #7)
> Note that we don't have a hard dependency on SoupPasswordManager, so maybe libsecret should also be used optionally.

The soft dependency on SoupPasswordManager was because it has been planned-to-be-deprecated almost as long as WebKit has been using it, and there was the possibility that it might just disappear in a future release of libsoup.

> Maybe we could use a different name for the directory than gtk, since libsecret doesn't depend on gtk, like we did for geoclue code. Libsecret uses glib to implement the freedesktop secrets spec (I think) so it could be used by other ports that depend on glib

Yes, in theory the EFL port should be able to use it as well (thought it would only be useful if they were running on a system that had a secrets/keyring/wallet daemon).

If you're using the async API though, it doesn't just "depend on glib", it depends on you running a glib mainloop. So really, it's pretty much tied to the soup backend, so maybe it should just go into soup/ ?

[Martin Robinson]

(In reply to comment #7)
> (From update of attachment 171542 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171542&action=review
> 
> This looks great, the only problem I see is that I think we should use the async API of libsecret. I guess we need to install libsecret in EWS and the bots, should we update the jhbuild moduleset too?

Yes, I think so too, but I would like to do that in another patch. In this patch I would just like to establish feature parity with the libsoup code. I don't mind making code that sets the password asynchronous, since it's very simple. As for getting the password, I have not even decided yet where the code will go. It's even possible that the dialog will not show up for credentials that are saved cross-session (this is how the mac port works for instance), so writing a lot of wiring here for asynchronous behavior doesn't make sense yet.

> > Source/WebCore/ChangeLog:11
> > +        a dependency on SoupPasswordManager.
> 
> I guess you forgot to remove this line.
> 
> > Source/WebKit/gtk/ChangeLog:9
> > +        Add a libsecret dependency to the build. This is necessary so that we can remove
> > +        a dependency on SoupPasswordManager.
> 
> Note that we don't have a hard dependency on SoupPasswordManager, so maybe libsecret should also be used optionally. We could add ifdefs to the CredentialBackingStore implementation or make a specific class for libsecret similar to the geoclue geolocation manager class.

I've been mulling this a bit and I think a hard dependency is fine right now for the GTK+ port. By the time that 2.0 ships, most desktop distributions be shipping it in the installed base. In any case, I think that making libsecret an optional dependency can be done in a followup patch. It makes sense to save the #ifdefs until this code is in the final place. That should happen soon, but I need to land this patch and the dependent as well. This was the approach we followed for spell checking.

> 
> > Source/WebCore/GNUmakefile.list.am:4740
> > +	Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp \
> > +	Source/WebCore/platform/network/gtk/CredentialBackingStore.h \
> 
> Maybe we could use a different name for the directory than gtk, since libsecret doesn't depend on gtk, like we did for geoclue code. Libsecret uses glib to implement the freedesktop secrets spec (I think) so it could be used by other ports that depend on glib, I'm not sure though. In any case it's just a name, we could leave gtk and just renamed it later if needed.

In this case "gtk" is better since it's not shared. If it's shared then it can move to a directory with a different name. It's in the "gtk" directory because it's only used by the GTK+ port.
> 
> > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:200
> > -    bool haveSavedLogin = getSavedLogin(m_auth.get(), &username, &password);
> >      soup_session_pause_message(m_session, m_message.get());
> > -    gtk_entry_set_text(GTK_ENTRY(m_loginEntry), username ? username : "");
> > -    gtk_entry_set_text(GTK_ENTRY(m_passwordEntry), password ? password : "");
> > -    if (m_rememberCheckButton && haveSavedLogin)
> > +
> > +    // This is just a temporary kludge until GtkAuthenticationDialog works directly with AuthenticationChallenges.
> > +    AuthenticationChallenge challenge(0 /* session */, m_message.get(), m_auth.get(), false, 0);
> > +    Credential savedCredential = credentialBackingStore().getCredentialForChallenge(challenge);
> > +    if (!savedCredential.isEmpty()) {
> 
> At this point in previous code, soup had already the users and passwords cached in hash tables, so that soup_auth_get_saved_users() and soup_auth_get_saved_password() can be used synchornously, but now we are using the sync api of libsecret that blocks the main loop until the service is created (the first time) and the users/passwords are retrieved. CredentialBackingStore API should be fully async, like the previous soup API, so that we don't show the dialog until the credential has been retrieved.

As I said above, I plan to make this asynchronous and this is just a step in that direction. There's a chicken and egg problem with these patches, since you won't allow me to regress any features even between releases and I don't want to build up a catalog of 5 patches and land them at once. This patch breaks the cycle.

> 
> > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:243
> > -    if (m_message.get()->status_code != 401 && m_message.get()->status_code < 500)
> > -        soup_auth_save_password(m_auth.get(), m_username.data(), m_password.data());
> > +    if (m_message.get()->status_code != 401 && m_message.get()->status_code < 500) {
> > +        // This is just a temporary kludge until GtkAuthenticationDialog works directly with AuthenticationChallenges.
> > +        AuthenticationChallenge challenge(0 /* session */, m_message.get(), m_auth.get(), false, 0);
> > +        Credential credentialToSave = Credential(
> > +            String::fromUTF8(m_username.data()),
> > +            String::fromUTF8(m_password.data()),
> > +            CredentialPersistencePermanent);
> > +        credentialBackingStore().storeCredentialsForChallenge(challenge, credentialToSave);
> 
> Same here, this blocks the main loop, but we are not even interested in the result of the operation.
> 
> > Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:81
> > +    GOwnPtr<GList> elements(secret_service_search_sync(
> > +        0, // The default SecretService.
> > +        SECRET_SCHEMA_COMPAT_NETWORK,
> > +        createAttributeHashTableFromChallenge(challenge).get(),
> > +        static_cast<SecretSearchFlags>(SECRET_SEARCH_UNLOCK | SECRET_SEARCH_LOAD_SECRETS), // The default behavior is to only return the most recent item.
> > +        0, // cancellable
> > +        &error.outPtr()));
> 
> I think we should use the async API.
> 
> > Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:113
> > +    secret_service_store_sync(
> > +        0, // The default SecretService.
> > +        SECRET_SCHEMA_COMPAT_NETWORK,
> > +        createAttributeHashTableFromChallenge(challenge, credential).get(),
> > +        SECRET_COLLECTION_DEFAULT,
> > +        _("WebKitGTK+ password"),
> > +        newSecretValue.get(),
> > +        0, // cancellable
> > +        &error.outPtr());
> 
> Ditto. In this case we could just pass NULL for the async ready callback, since we are not interested in the result of the operation.
> 
> > Source/WebCore/platform/network/gtk/CredentialBackingStore.h:34
> > +class KURL;
> 
> Is KURL needed here? Could we use forward declarations of AuthenticationChallenge and Credential here too instead of including the headers?

These forward declarations aren't necessary. I think I can add one for AuthenticationChallenge, but not Credential. I'll update the patch.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 171542 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171542&action=review
> > 
> > This looks great, the only problem I see is that I think we should use the async API of libsecret. I guess we need to install libsecret in EWS and the bots, should we update the jhbuild moduleset too?
> 
> Yes, I think so too, but I would like to do that in another patch. In this patch I would just like to establish feature parity with the libsoup code. I don't mind making code that sets the password asynchronous, since it's very simple. As for getting the password, I have not even decided yet where the code will go. It's even possible that the dialog will not show up for credentials that are saved cross-session (this is how the mac port works for instance), so writing a lot of wiring here for asynchronous behavior doesn't make sense yet.

Ah, ok, blocking the UI for some time is better than a regression after all :-)

> > > Source/WebCore/ChangeLog:11
> > > +        a dependency on SoupPasswordManager.
> > 
> > I guess you forgot to remove this line.
> > 
> > > Source/WebKit/gtk/ChangeLog:9
> > > +        Add a libsecret dependency to the build. This is necessary so that we can remove
> > > +        a dependency on SoupPasswordManager.
> > 
> > Note that we don't have a hard dependency on SoupPasswordManager, so maybe libsecret should also be used optionally. We could add ifdefs to the CredentialBackingStore implementation or make a specific class for libsecret similar to the geoclue geolocation manager class.
> 
> I've been mulling this a bit and I think a hard dependency is fine right now for the GTK+ port. By the time that 2.0 ships, most desktop distributions be shipping it in the installed base. In any case, I think that making libsecret an optional dependency can be done in a followup patch. It makes sense to save the #ifdefs until this code is in the final place. That should happen soon, but I need to land this patch and the dependent as well. This was the approach we followed for spell checking.

I'm fine with a hard dependency for now, if people complain in the future we can easily make it optional for the final code like enchant or geoclue.

> > 
> > > Source/WebCore/GNUmakefile.list.am:4740
> > > +	Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp \
> > > +	Source/WebCore/platform/network/gtk/CredentialBackingStore.h \
> > 
> > Maybe we could use a different name for the directory than gtk, since libsecret doesn't depend on gtk, like we did for geoclue code. Libsecret uses glib to implement the freedesktop secrets spec (I think) so it could be used by other ports that depend on glib, I'm not sure though. In any case it's just a name, we could leave gtk and just renamed it later if needed.
> 
> In this case "gtk" is better since it's not shared. If it's shared then it can move to a directory with a different name. It's in the "gtk" directory because it's only used by the GTK+ port.

I guess it will be easier to decide later, yes, let's use gtk for now, we can always move it libsecret/soup or whatever. 

> > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:200
> > > -    bool haveSavedLogin = getSavedLogin(m_auth.get(), &username, &password);
> > >      soup_session_pause_message(m_session, m_message.get());
> > > -    gtk_entry_set_text(GTK_ENTRY(m_loginEntry), username ? username : "");
> > > -    gtk_entry_set_text(GTK_ENTRY(m_passwordEntry), password ? password : "");
> > > -    if (m_rememberCheckButton && haveSavedLogin)
> > > +
> > > +    // This is just a temporary kludge until GtkAuthenticationDialog works directly with AuthenticationChallenges.
> > > +    AuthenticationChallenge challenge(0 /* session */, m_message.get(), m_auth.get(), false, 0);
> > > +    Credential savedCredential = credentialBackingStore().getCredentialForChallenge(challenge);
> > > +    if (!savedCredential.isEmpty()) {
> > 
> > At this point in previous code, soup had already the users and passwords cached in hash tables, so that soup_auth_get_saved_users() and soup_auth_get_saved_password() can be used synchornously, but now we are using the sync api of libsecret that blocks the main loop until the service is created (the first time) and the users/passwords are retrieved. CredentialBackingStore API should be fully async, like the previous soup API, so that we don't show the dialog until the credential has been retrieved.
> 
> As I said above, I plan to make this asynchronous and this is just a step in that direction. There's a chicken and egg problem with these patches, since you won't allow me to regress any features even between releases and I don't want to build up a catalog of 5 patches and land them at once. This patch breaks the cycle.

Sure, I didn't know you were planning to make it async in future patches.

> > 
> > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:243
> > > -    if (m_message.get()->status_code != 401 && m_message.get()->status_code < 500)
> > > -        soup_auth_save_password(m_auth.get(), m_username.data(), m_password.data());
> > > +    if (m_message.get()->status_code != 401 && m_message.get()->status_code < 500) {
> > > +        // This is just a temporary kludge until GtkAuthenticationDialog works directly with AuthenticationChallenges.
> > > +        AuthenticationChallenge challenge(0 /* session */, m_message.get(), m_auth.get(), false, 0);
> > > +        Credential credentialToSave = Credential(
> > > +            String::fromUTF8(m_username.data()),
> > > +            String::fromUTF8(m_password.data()),
> > > +            CredentialPersistencePermanent);
> > > +        credentialBackingStore().storeCredentialsForChallenge(challenge, credentialToSave);
> > 
> > Same here, this blocks the main loop, but we are not even interested in the result of the operation.
>>
> > > Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:81
> > > +    GOwnPtr<GList> elements(secret_service_search_sync(
> > > +        0, // The default SecretService.
> > > +        SECRET_SCHEMA_COMPAT_NETWORK,
> > > +        createAttributeHashTableFromChallenge(challenge).get(),
> > > +        static_cast<SecretSearchFlags>(SECRET_SEARCH_UNLOCK | SECRET_SEARCH_LOAD_SECRETS), // The default behavior is to only return the most recent item.
> > > +        0, // cancellable
> > > +        &error.outPtr()));
> > 
> > I think we should use the async API.
> > 
> > > Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:113
> > > +    secret_service_store_sync(
> > > +        0, // The default SecretService.
> > > +        SECRET_SCHEMA_COMPAT_NETWORK,
> > > +        createAttributeHashTableFromChallenge(challenge, credential).get(),
> > > +        SECRET_COLLECTION_DEFAULT,
> > > +        _("WebKitGTK+ password"),
> > > +        newSecretValue.get(),
> > > +        0, // cancellable
> > > +        &error.outPtr());
> > 
> > Ditto. In this case we could just pass NULL for the async ready callback, since we are not interested in the result of the operation.

If we are going to keep the sync api for now, it would be better to pass NULL for the error, since we are not using it.

> > > Source/WebCore/platform/network/gtk/CredentialBackingStore.h:34
> > > +class KURL;
> > 
> > Is KURL needed here? Could we use forward declarations of AuthenticationChallenge and Credential here too instead of including the headers?
> 
> These forward declarations aren't necessary. I think I can add one for AuthenticationChallenge, but not Credential. I'll update the patch.

Great, thanks!

[Martin Robinson]

Created attachment 171676 [details]
Patch

[WebKit Review Bot]

Attachment 171676 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:65:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #10)

Thanks for the review! This should be asynchronous in the next patch in the series or, less likely, the one after.

> If we are going to keep the sync api for now, it would be better to pass NULL for the error, since we are not using it.

I'm a bit nervous about this because it isn't marked allow-none.

[Dan Winship]

GErrors (and GCancellables) are always allow-none

[kov's GTK+ EWS bot]

Comment on attachment 171676 [details]
Patch

Attachment 171676 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14685330

[Daniel Bates]

Comment on attachment 171676 [details]
Patch

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?

[Carlos Garcia Campos]

Comment on attachment 171676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171676&action=review

Patch looks good to me too, I guess we need to wait until libsecret is installed in EWS bots before landing. And either install libsecret in all the bots too, or add libsecret to our jhbuild moduleset. Thanks!

> Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:84
> +    if (error || !elements || !elements->data)
> +        return Credential();

Since we are not interested in the actual error, and elements will always be NULL in case of error, you can simply pass NULL for the error, and simply check elements and elements->data are not NULL.

[Martin Robinson]

(In reply to comment #16)
> (From update of attachment 171676 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171676&action=review

Thanks for the review!

> 
> > 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.

I'm just a bit too cautious when I write my ChangeLogs. :) Removed "should"

> 
> > 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.

I linked to the bug.

> 
> > 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.

I moved the empty bodies to the header. I'm not sure if there's any benefit of  having the compiler generate them other than less typing, but I kind of like being explicit. If this violates the style guide I can remove them.


> > Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:81
> > +        0, // cancellable
> 
> Nit: cancellable => cancelable
> 
> > Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:112
> > +        0, // cancellable
> 
> Ditto.

The interesting thing is that you can actually spell it both ways. :) I chose cancellable because that's what the documentation uses for the name.

http://developer.gnome.org/libsecret/unstable/SecretService.html#secret-service-search-sync
> 
> > Source/WebCore/platform/network/gtk/CredentialBackingStore.h:35
> > +class CredentialBackingStore {
> 
> Can we make this non-copyable?

Yes!

> > 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?


Nice catch.

[Martin Robinson]

(In reply to comment #17)
> (From update of attachment 171676 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171676&action=review
> 
> Patch looks good to me too, I guess we need to wait until libsecret is installed in EWS bots before landing. And either install libsecret in all the bots too, or add libsecret to our jhbuild moduleset. Thanks!
> 
> > Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:84
> > +    if (error || !elements || !elements->data)
> > +        return Credential();
> 
> Since we are not interested in the actual error, and elements will always be NULL in case of error, you can simply pass NULL for the error, and simply check elements and elements->data are not NULL.

Usually I check both for completeness sake, but if you insist... :)

[Martin Robinson]

Created attachment 171868 [details]
Patch to commit

[WebKit Review Bot]

Attachment 171868 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:57:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 171868 [details]
Patch to commit

Rejecting attachment 171868 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14656964

[WebKit Review Bot]

Comment on attachment 171868 [details]
Patch to commit

Clearing flags on attachment: 171868

Committed r133317: <http://trac.webkit.org/changeset/133317>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.