Bug 104910 – [GTK] When in private mode WebKitGTK+ should not save HTTP authentication credentials to the persistent storage

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

Instead of using persistent credential storage when private mode is active, WebKitGTK+ should just use session storage.

[Alberto Garcia]

I'll take a look

[Alberto Garcia]

Created attachment 179463 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 179463 [details]
Patch

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

Great patch! If you were a committer I'd r+ and have you land with a few small changes. I have just a few small aesthetic suggestions to make the code more WebKitty.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:260
> +        CredentialPersistence persistence;
> +
> +        if (rememberPassword && dialog->m_browsingMode == NormalMode)
> +            persistence = CredentialPersistencePermanent;
> +        else
> +            persistence = CredentialPersistenceForSession;

This could be one line.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.h:40
> +    enum BrowsingMode {
> +        NormalMode, // The user is asked whether to store credential information
> +        PrivateMode // Credential information is only kept in the session
> +    };

I think that it would make more sense for the authentication dialog to have no knowledge of the concept of browsing mode. Instead the naming of these could be something like:

enum PersistentStorageAllowed {
    AllowPersistentStorage,
    DisallowPersistentStorage,
}

WebKit comments that are complete sentences should end with a period, but I think it's also okay to omit the comments here since the naming is clear enough.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1622
> +    GtkAuthenticationDialog::BrowsingMode browsingMode;
> +
> +    if (webkit_settings_get_enable_private_browsing(webView->priv->settings.get()))
> +        browsingMode = GtkAuthenticationDialog::PrivateMode;
> +    else
> +        browsingMode = GtkAuthenticationDialog::NormalMode;
> +

This could probably just be one line:

PersistentStorageAllowed persistentStorage = webkit_settings_get_enable_private_browsing(webView->priv->settings.get()) ? GtkAuthenticationDialog::AllowPersistentStorage ? GtkAuthenticationDialog::DisallowPersistentStorage;

[Alberto Garcia]

(In reply to comment #4)
> > +        CredentialPersistence persistence;
> > +
> > +        if (rememberPassword && dialog->m_browsingMode == NormalMode)
> > +            persistence = CredentialPersistencePermanent;
> > +        else
> > +            persistence = CredentialPersistenceForSession;
>
> This could be one line.

I know, I did it on purpose because the alternative is a 150+ column
line, and I think it's much more readable like this.

But I can change it if you think it's better.

> I think that it would make more sense for the authentication dialog
> > to have no knowledge of the concept of browsing mode. Instead the
> > naming of these could be something like:
>
> enum PersistentStorageAllowed {
>     AllowPersistentStorage,
>     DisallowPersistentStorage,
> }

That sounds good to me.

> WebKit comments that are complete sentences should end with a period

I can also fix that.

[Martin Robinson]

(In reply to comment #5)

> I know, I did it on purpose because the alternative is a 150+ column
> line, and I think it's much more readable like this.
> 
> But I can change it if you think it's better.

You can always break the line too. Lines in WebKit typically go to 120 columns and beyond. I usually break mine around 120 columns. Up to you in the end.

[Alberto Garcia]

Created attachment 179481 [details]
Patch v2

[WebKit Review Bot]

Comment on attachment 179481 [details]
Patch v2

Clearing flags on attachment: 179481

Committed r137748: <http://trac.webkit.org/changeset/137748>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.