[Webkit-unassigned] [Bug 51830] [GTK] Use GtkStyleContext to get platform colors

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=51830


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #77821|review?                     |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2011-01-03 15:21:39 PST ---
(From update of attachment 77821)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.