[webkit-reviews] review denied: [Bug 62366] [GTK] Support authentication dialogs in WebKit2 : [Attachment 96571] Patch rebased to current git master
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 10 16:40:16 PDT 2011
Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 62366: [GTK] Support authentication dialogs in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=62366
Attachment 96571: Patch rebased to current git master
https://bugs.webkit.org/attachment.cgi?id=96571&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.
More information about the webkit-reviews
mailing list