[Webkit-unassigned] [Bug 50623] [GTK] Use gtk_icon_set_render_icon() to render icons in RenderThemeGtk

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #75798|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #9 from Martin Robinson <mrobinson at webkit.org>  2010-12-08 01:29:19 PST ---
(From update of attachment 75798)
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.

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