In WebKit2 we currently don't add the Authentication Dialog feature to soup.
Created attachment 96570 [details] Patch This patch moves the common code to show an auth dialog from webkitsoupauthdialog to a new C++ class in WebCore. In WebKit1 webkitsoupauthdialog has been converted into a C++ file with the same interface, since it's public in the api, and it simply contains the soup feature interface implementation using the new GtkAuthenticationDialog class. In WebKit2 I added the soup feature interface impemlentation to WebProcess/gtk/WebProcessMainGtk.cpp since it's very simple, but I can move it into its own file if you think it's better.
Created attachment 96571 [details] Patch rebased to current git master
Attachment 96571 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:48: web_auth_dialog_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:55: web_auth_dialog_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:59: web_auth_dialog_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:71: Use 0 instead of NULL. [readability/null] [5] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 96571 [details] Patch rebased to current git master Attachment 96571 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8826077
(In reply to comment #4) > (From update of attachment 96571 [details]) > Attachment 96571 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/8826077 The bot needs a clean build, because there's a file renamed.
Comment on attachment 96571 [details] Patch rebased to current git master View in context: https://bugs.webkit.org/attachment.cgi?id=96571&action=review Looks very nice. I really appreciate the code reuse. I have some minor style nits, but I think it's enough to go one more round. > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:30 > +static GtkWidget* tableAddEntry(GtkTable* table, int row, const char* labelText) Function names typically start with the verb, so I think this should be "addEntryToTable." > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:89 > + // Build contents. You can omit this comment. > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:151 > +static bool getSavedLogin(SoupAuth* auth, const char** username, const char** password) Please use String& here instead pointers. > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:175 > + const char* username = 0; > + const char* password = 0; > + bool haveSavedLoging = getSavedLogin(m_auth, &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 you use String for username and password you can avoid the null check and simply do: username.utf8().data(). > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:176 > + if (m_rememberCheckButton && haveSavedLoging) haveSavedLoging -> haveSavedLogin > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:186 > + soup_session_unpause_message(m_session, m_message.get()); I'm surprised it's the responsibility of WebKit to pause and unpause the message! > Source/WebCore/platform/gtk/GtkAuthenticationDialog.h:69 > + GOwnPtr<char> m_username; > + GOwnPtr<char> m_password; These would definitely be better as CStrings. > Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:40 > +static void webkit_soup_auth_dialog_session_feature_init(SoupSessionFeatureInterface* feature_interface, gpointer interface_data); > +static void attach(SoupSessionFeature* manager, SoupSession* session); > +static void detach(SoupSessionFeature* manager, SoupSession* session); You should omit the variable names here now that this file is C++. > Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:55 > + GObjectClass* object_class = G_OBJECT_CLASS(klass); object_class -> objectClass. > Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:75 > + NULL, NULL, NULL -> 0 > Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:81 > +static void webkit_soup_auth_dialog_init(WebKitSoupAuthDialog* instance) Here you can avoid the variable name. > Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:86 > +static void webkit_soup_auth_dialog_session_feature_init(SoupSessionFeatureInterface *feature_interface, > + gpointer interface_data) The asterisk should move over, you can omit interface_data, feature_interface -> featureInterface and this should be one line. :) > Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:92 > +static void session_authenticate(SoupSession* session, SoupMessage* message, SoupAuth* auth, gboolean retrying, SoupSessionFeature* manager) You can omit "retrying" and "manager" here. Since this isn't a GObject method name, please use camelCase for the function name. > Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:42 > +typedef struct { > + GObject parent; > +} WebAuthDialog; I'd prefer this GObject to be in a separate file. >> Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:48 >> +static GType web_auth_dialog_get_type(void); > > web_auth_dialog_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Please omit the void from the empty argument list. > Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:76 > + g_signal_handlers_disconnect_by_func(session, (gpointer)sessionAuthenticate, 0); Please use a C++-style cast for sessionAuthenticate here. > Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:79 > +static void webAuthDialogSessionFeatureInit(SoupSessionFeatureInterface *feature, gpointer) The asterisk should move to the type name. > Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:114 > + SoupSessionFeature* authDialog = static_cast<SoupSessionFeature*>(g_object_new(web_auth_dialog_get_type(), NULL)); > + soup_session_add_feature(session, authDialog); > + g_object_unref(authDialog); > + Please use GRefPtr here.
(In reply to comment #6) > (From update of attachment 96571 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96571&action=review > > Looks very nice. I really appreciate the code reuse. I have some minor style nits, but I think it's enough to go one more round. Ok, thanks for the review, I'll adress all issues in a new patch.
(In reply to comment #6) > (From update of attachment 96571 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96571&action=review > > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:151 > > +static bool getSavedLogin(SoupAuth* auth, const char** username, const char** password) > > Please use String& here instead pointers. Note that pointers are const, using String would duplicate the strings and would convert to UTF-16 and later back to UTF-8 unnecessarily. > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:186 > > + soup_session_unpause_message(m_session, m_message.get()); > > I'm surprised it's the responsibility of WebKit to pause and unpause the message! I don't know libsoup api too much, this is copy-pasted from current code. > > Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:92 > > +static void session_authenticate(SoupSession* session, SoupMessage* message, SoupAuth* auth, gboolean retrying, SoupSessionFeature* manager) > > You can omit "retrying" and "manager" here. Since this isn't a GObject method name, please use camelCase for the function name. manager is used to emit the signal.
Created attachment 96947 [details] Updated patch according to review
Attachment 96947 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/WebProcess/gtk/WebAuthDialog.cpp:39: web_auth_dialog_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/WebProcess/gtk/WebAuthDialog.cpp:43: web_auth_dialog_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/WebProcess/gtk/WebAuthDialog.h:27: web_auth_dialog_get_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:71: Use 0 instead of NULL. [readability/null] [5] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 96947 [details] Updated patch according to review Attachment 96947 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8840005
Committed r88800: <http://trac.webkit.org/changeset/88800>