[webkit-reviews] review denied: [Bug 51830] [GTK] Use GtkStyleContext to get platform colors : [Attachment 77821] Use GtkStyleContext API to get platform colors

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 51830: [GTK] Use GtkStyleContext to get platform colors
https://bugs.webkit.org/show_bug.cgi?id=51830

Attachment 77821: Use GtkStyleContext API to get platform colors
https://bugs.webkit.org/attachment.cgi?id=77821&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77821&action=review

Looks great. I have one suggested improvement below.

> WebCore/platform/gtk/RenderThemeGtk3.cpp:716
> +static Color getStyleColor(GType widgetType, bool background, GtkStateFlags
flags)

It think it's okay to use a bool here since this helper is so localized, but
using an enum as an argument type is generally preferred.

> WebCore/platform/gtk/RenderThemeGtk3.cpp:731
> +    gdkColor.pixel = 0;
> +    gdkColor.red = (guint16) (gdkRGBAColor.red * 65535);
> +    gdkColor.green = (guint16) (gdkRGBAColor.green * 65535);
> +    gdkColor.blue = (guint16) (gdkRGBAColor.blue * 65535);
> +

I think it makes sense to create a WebCore color here directly instead of
relying on a GdkColor intermediary. Perhaps this could just be:

return Color(gdkRGBAColor.red, gdkRGBAColor.green, gdkRGBAColor.blue,
gdkRGBAColor.alpha);

I could even see adding a specialized constructor in Color.h and Color.cpp for
GDK 3.x's GdkRGBA.