[webkit-reviews] review denied: [Bug 136615] [GTK] Use a nicer HTTP authentication dialog : [Attachment 237810] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 136615: [GTK] Use a nicer HTTP authentication dialog
https://bugs.webkit.org/show_bug.cgi?id=136615

Attachment 237810: Patch
https://bugs.webkit.org/attachment.cgi?id=237810&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237810&action=review


> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

You should remove this line.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:-151
> -    GtkWidget* icon =
gtk_image_new_from_stock(GTK_STOCK_DIALOG_AUTHENTICATION,
GTK_ICON_SIZE_DIALOG);
> -    gtk_misc_set_alignment(GTK_MISC(icon), 0.5, 0.0);
> -    gtk_box_pack_start(GTK_BOX(authWidget), icon, FALSE, FALSE, 0);
> -    gtk_widget_show(icon);

Why removing the dialog icon?

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:140
> +	   /* Prompt on HTTP authentication dialog */

Use C++ comment, and finish it with period.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:141
> +	   _("Authentication required by %s:"),

Why removing the port?

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:144
> +    gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5);

Can we take advantage to not use GtkMisc?

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:158
> +    /* Check button on the HTTP authentication dialog */

C++ comments finished with a period.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:159
> +    priv->rememberCheckButton =
gtk_check_button_new_with_mnemonic(_("_Remember password"));

Why removing the ':' ? Were these strings translated? Since the file was not
listed in the POTFILES.in. If we already have translations of these, maybe we
can try to improve the dialog without changing strings, since it's late in the
release cycle.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:162
> +    /* Entry on the HTTP authentication dialog */

C++ comments finished with a period.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:163
> +    GtkWidget* username_label = gtk_label_new_with_mnemonic(_("_Username"));


username_label -> usernameLabel

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:164
> +    GtkWidget* username_entry = createEntry(&priv->loginEntry);

username_entry -> usernameEntry

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:167
> +    /* Entry on the HTTP authentication dialog */

C++ comments finished with a period.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:168
> +    GtkWidget* password_label = gtk_label_new_with_mnemonic(_("_Password"));


password_label -> passwordLabel

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:169
> +    GtkWidget* password_entry = createEntry(&priv->passwordEntry);

password_entry -> passwordEntry

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:175
> +#if GTK_CHECK_VERSION(3, 0, 0)
> +   
gtk_style_context_add_class(gtk_widget_get_style_context(username_label),
"dim-label");
> +   
gtk_style_context_add_class(gtk_widget_get_style_context(password_label),
"dim-label");
> +#endif

What is this dim-label class for? We can remove the GTK+2 support here, maybe
we can do it that first, and then this patch would be simpler.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:71
> +    /* Title of the HTTP authentication dialog */

C++ comments finished with a period.