[webkit-reviews] review denied: [Bug 50623] [GTK] Use gtk_icon_set_render_icon() to render icons in RenderThemeGtk : [Attachment 75798] Patch to use gtk_icon_set_render_icon()

[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 75798: Patch to use gtk_icon_set_render_icon()
https://bugs.webkit.org/attachment.cgi?id=75798&action=review

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

Great patch. My biggest concern is whether or not the GTK+ icon caching
behavior is defined. If it's defined can we be sure that it's performant?

> WebCore/platform/gtk/RenderThemeGtk.cpp:85
> +    RefPtr<BitmapImage> img = BitmapImage::create(surface);
> +    return img.release();
> +}

I think this should just be return BitmapImage::create(surface);

> WebCore/platform/gtk/RenderThemeGtk.cpp:117
> +	       iconSet = gtk_icon_set_new();

If this variable isn't used outside this block, it should be delcared here.

> WebCore/platform/gtk/RenderThemeGtk.cpp:263
> +GtkStateType RenderThemeGtk::gtkIconState(RenderObject* renderObject)

I think this can just be a static function. It doesn't seem to need to be a
method.

> WebCore/platform/gtk/RenderThemeGtk.cpp:268
> +	   state = GTK_STATE_INSENSITIVE;

These should be early returns.

> WebCore/platform/gtk/RenderThemeGtk.cpp:478
> +    gint width, height;

Please initialize these values to 0.

> WebCore/platform/gtk/RenderThemeGtk.cpp:502
> +    RefPtr<Image> searchImage = loadStockIcon(style, GTK_STOCK_FIND,
gtkTextDirection(renderObject->style()->direction()),

Instead of blitting the icon GdkPixbuf to a cairo surface wrapping it in a
WebCore Image and then having WebCore paint one surface to another, it would be
more effecieint to simply call gdk_cairo_set_source_pixbuf and paint the
GdkPixbuf directly.

Does GtkIconSet cache the icons or does it load them every time?

> WebCore/platform/gtk/RenderThemeGtk.cpp:515
> +    gint width, height;

Please initialize these values before using them.

> WebCore/platform/gtk/RenderThemeGtk.cpp:525
> +    RefPtr<Image> cancelImage = loadStockIcon(style, GTK_STOCK_CLEAR,
gtkTextDirection(renderObject->style()->direction()),
> +						 gtkIconState(renderObject),
GTK_ICON_SIZE_MENU);

Same suggesiton here as above.

> WebCore/platform/gtk/RenderThemeGtk.cpp:789
> +bool RenderThemeGtk::paintMediaButton(RenderObject* renderObject,
GraphicsContext* context, const IntRect& rect, const char* iconName, Color
panelColor, int mediaIconSize)

Is it possible to make this a static function instead of a method?

> WebCore/platform/gtk/RenderThemeGtk.cpp:793
> +    RefPtr<Image> image = loadStockIcon(style, iconName,
gtkTextDirection(renderObject->style()->direction()),
gtkIconState(renderObject), getMediaButtonIconSize(mediaIconSize));

Ditto.

> WebCore/platform/gtk/RenderThemeGtk.h:142
> +    virtual void initMediaColors();
> +    virtual void initMediaButtons();

Please remove the 'virtual' here.

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

No need for "rect" variable name and the asterisk on iconName should be with
the type name.

> WebCore/platform/gtk/RenderThemeGtk.h:177
> +    GtkStateType gtkIconState(RenderObject* renderObject);

No need for the variable name.