Bug 104910 - [GTK] When in private mode WebKitGTK+ should not save HTTP authentication credentials to the persistent storage
: [GTK] When in private mode WebKitGTK+ should not save HTTP authentication cre...
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-12-13 06:39 PST by
Modified: 2012-12-14 08:59 PST (History)


Attachments
Patch (11.92 KB, patch)
2012-12-14 03:35 PST, Alberto Garcia
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (12.40 KB, patch)
2012-12-14 07:46 PST, Alberto Garcia
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-12-13 06:39:42 PST
Instead of using persistent credential storage when private mode is active, WebKitGTK+ should just use session storage.
------- Comment #1 From 2012-12-13 08:33:41 PST -------
I'll take a look
------- Comment #2 From 2012-12-14 03:35:08 PST -------
Created an attachment (id=179463) [details]
Patch
------- Comment #3 From 2012-12-14 03:37:35 PST -------
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
------- Comment #4 From 2012-12-14 04:02:44 PST -------
(From update of attachment 179463 [details])
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;
------- Comment #5 From 2012-12-14 04:26:42 PST -------
(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.
------- Comment #6 From 2012-12-14 05:06:07 PST -------
(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.
------- Comment #7 From 2012-12-14 07:46:53 PST -------
Created an attachment (id=179481) [details]
Patch v2
------- Comment #8 From 2012-12-14 08:58:57 PST -------
(From update of attachment 179481 [details])
Clearing flags on attachment: 179481

Committed r137748: <http://trac.webkit.org/changeset/137748>
------- Comment #9 From 2012-12-14 08:59:00 PST -------
All reviewed patches have been landed.  Closing bug.