Bug 104910

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.