[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





--- Comment #10 from Carlos Garcia Campos <cgarcia at igalia.com>  2010-12-08 02:01:13 PST ---
(In reply to comment #9)
> (From update of attachment 75798 [details])
> 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?

GTK+ icon caching behaviour? icons are cached by gtkiconfactory, for any of their states and direction the first time they are rendered, and cache is cleared when the theme changes. Take a look at the code if you want to make sure:

http://git.gnome.org/browse/gtk+/tree/gtk/gtkiconfactory.c#n1082

Why wouldn't it be 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.

Ok.

> > 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.

I don't think so, isEnabled(), isPressed() and isHovered() are methods of RenderTheme, I can make it static and pass a pointer to RenderTheme, but I don't think it's worth it. 

> > WebCore/platform/gtk/RenderThemeGtk.cpp:268
> > +        state = GTK_STATE_INSENSITIVE;
> 
> These should be early returns.

Ok

> > 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.

It's mostly the same, gdk_cairo_set_source_pixbuf() converts the pixbuf into a cairo surface and sets the surface as the current source, so you don't paint the pixbuf directly. 

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

Yes, icons are cached by gtk+.

> > 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?

I don't think so. we are using gtkContainer()

> > 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.