Summary: | [GTK] When in private mode WebKitGTK+ should not save HTTP authentication credentials to the persistent storage | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | berto, cgarcia, gustavo, webkit.review.bot, xan.lopez | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Martin Robinson
2012-12-13 06:39:42 PST
I'll take a look Created attachment 179463 [details]
Patch
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 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; (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. (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. Created attachment 179481 [details]
Patch v2
Comment on attachment 179481 [details] Patch v2 Clearing flags on attachment: 179481 Committed r137748: <http://trac.webkit.org/changeset/137748> All reviewed patches have been landed. Closing bug. |