[webkit-reviews] review denied: [Bug 50623] [GTK] Use gtk_icon_set_render_icon() to render icons in RenderThemeGtk : [Attachment 75890] Updated patch addressing martin's comments

[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 50623: [GTK] Use gtk_icon_set_render_icon() to render icons in
RenderThemeGtk
https://bugs.webkit.org/show_bug.cgi?id=50623

Attachment 75890: Updated patch addressing martin's comments
https://bugs.webkit.org/attachment.cgi?id=75890&action=review

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

Looking good!

> WebCore/platform/gtk/RenderThemeGtk.cpp:73
> +    PlatformRefPtr<GdkPixbuf> icon = gtk_icon_set_render_icon(iconSet,
style, direction, state, iconSize, 0, 0);

If gtk_icon_set_render_icon returns a new reference this should be wrapped in
adoptPlatformRef. If it doesn't, this can just be a raw GdkPixbuf* since we
don't need to take a reference.

> WebCore/platform/gtk/RenderThemeGtk.cpp:102
> +    if (!iconsInitialized) {

Please use an early return here.

> WebCore/platform/gtk/RenderThemeGtk.cpp:103
> +	   GtkIconFactory* iconFactory = gtk_icon_factory_new();

This is a situation where a PlatformRefPtr with adoptPlatformRef would make
sense.

> WebCore/platform/gtk/RenderThemeGtk.cpp:480
> +	   return IntPoint(rect.x(), rect.y());

This should just be rect.topLeft()

> WebCore/platform/gtk/RenderThemeGtk.h:175
> +    bool paintMediaButton(RenderObject*, GraphicsContext*, const IntRect&,
const char* iconName, Color panelColor, int mediaIconSize);

It looks like panelColor is always m_panelColor and mediaIconSize is always
m_mediaIconSize, so you can remove these parameters.