Bug 62366 - [GTK] Support authentication dialogs in WebKit2
Summary: [GTK] Support authentication dialogs in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-06-09 03:43 PDT by Carlos Garcia Campos
Modified: 2011-06-14 07:37 PDT (History)
4 users (show)

See Also:


Attachments
Patch (37.72 KB, patch)
2011-06-09 03:56 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch rebased to current git master (37.72 KB, patch)
2011-06-09 04:19 PDT, Carlos Garcia Campos
mrobinson: review-
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff
Updated patch according to review (41.39 KB, patch)
2011-06-13 05:17 PDT, Carlos Garcia Campos
mrobinson: review+
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-06-09 03:43:24 PDT
In WebKit2 we currently don't add the Authentication Dialog feature to soup.
Comment 1 Carlos Garcia Campos 2011-06-09 03:56:14 PDT
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.
Comment 2 Carlos Garcia Campos 2011-06-09 04:19:00 PDT
Created attachment 96571 [details]
Patch rebased to current git master
Comment 3 WebKit Review Bot 2011-06-09 04:22:07 PDT
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 4 Collabora GTK+ EWS bot 2011-06-09 10:05:40 PDT
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
Comment 5 Carlos Garcia Campos 2011-06-09 10:22:41 PDT
(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 6 Martin Robinson 2011-06-10 16:40:17 PDT
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.
Comment 7 Carlos Garcia Campos 2011-06-13 01:03:44 PDT
(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.
Comment 8 Carlos Garcia Campos 2011-06-13 03:08:10 PDT
(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.
Comment 9 Carlos Garcia Campos 2011-06-13 05:17:30 PDT
Created attachment 96947 [details]
Updated patch according to review
Comment 10 WebKit Review Bot 2011-06-13 05:20:39 PDT
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 11 Collabora GTK+ EWS bot 2011-06-13 05:27:57 PDT
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
Comment 12 Carlos Garcia Campos 2011-06-14 07:37:08 PDT
Committed r88800: <http://trac.webkit.org/changeset/88800>