Bug 50623 – [GTK] Use gtk_icon_set_render_icon() to render icons in RenderThemeGtk

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Using gtk_icon_set_render_icon() we use the gtk icon cache and icons are rendered using state and direction. So, icons are prelighted on hover (we can remove the TODO comment in paintSearchFieldCancelButton), although it doesn't currently work due to a gtk+ issue (https://bugzilla.gnome.org/show_bug.cgi?id=636691). Gtk icon cache is automatically cleared when the theme changes, so we don't need to connect to the theme changed signal.

[Carlos Garcia Campos]

Created attachment 75798 [details]
Patch to use gtk_icon_set_render_icon()

With this patch bugs #50225 and #50226 can be closed too.

[Carlos Garcia Campos]

*** Bug 50226 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

*** Bug 50225 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Attachment 75798 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75798 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75798 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75798 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75798 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 75798 [details]
Patch to use gtk_icon_set_render_icon()

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 75890 [details]
Updated patch addressing martin's comments

[Martin Robinson]

Comment on attachment 75890 [details]
Updated patch addressing martin's comments

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.

[Carlos Garcia Campos]

Created attachment 75901 [details]
A new update fixing more issues

[Martin Robinson]

Comment on attachment 75901 [details]
A new update fixing more issues

Thanks!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 75901 [details]:

inspector/debugger-breakpoints-not-activated-on-reload.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 75901 [details]
A new update fixing more issues

Clearing flags on attachment: 75901

Committed r73561: <http://trac.webkit.org/changeset/73561>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/73561 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug
The following tests are not passing:
fast/css/input-search-padding.html
fast/css/text-input-with-webkit-border-radius.html
fast/forms/box-shadow-override.html
fast/forms/control-restrict-line-height.html
fast/forms/input-appearance-height.html
fast/forms/placeholder-pseudo-style.html
fast/forms/placeholder-set-value.html
fast/forms/search-cancel-button-style-sharing.html
fast/forms/search-placeholder-value-changed.html
fast/forms/search-rtl.html
fast/forms/search-styled.html
fast/forms/search-transformed.html
fast/forms/search-vertical-alignment.html
fast/forms/search-zoomed.html
fast/forms/searchfield-heights.html