Bug 50225 – [GTK] Use gtk_widget_render_icon() to render media buttons using gtk stock icons

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

[Carlos Garcia Campos]

gtk_widget_render_icon() returns the right version of the icon depending on the widget direction, so we don't need to change the icon name. It also uses the gtk icon cache, so we can avoid loading the icons from files.

[Carlos Garcia Campos]

Created attachment 75124 [details]
Patch to use gtk_widget_render_icon()

[Martin Robinson]

Comment on attachment 75124 [details]
Patch to use gtk_widget_render_icon()

View in context: https://bugs.webkit.org/attachment.cgi?id=75124&action=review

Great. Just a couple small issues.

> WebCore/platform/gtk/RenderThemeGtk.cpp:60
> +#define WEBKIT_MEDIA_BUTTON_SIZE "webkit-media-button-size"

I think you should just use this this directly.

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

No need to cache this. You can simply return BitmapImage::create();

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

Same here, I believe.

[Carlos Garcia Campos]

Created attachment 75368 [details]
New patch

It also uses gtk_widget_render_icon() for search entry icons.

[Martin Robinson]

Comment on attachment 75368 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75368&action=review

This patch seems to contain some bits from your other media styling patch. Other than that it looks pretty good. I have a few comments.

> WebCore/platform/gtk/RenderThemeGtk.cpp:72
> +    GdkPixbuf* icon = gtk_widget_render_icon(widget, iconName, iconSize, "button");

Consider using a PlatformRefPtr here.

> WebCore/platform/gtk/RenderThemeGtk.cpp:484
> -    // FIXME: This should not be hard-coded.
> -    IntSize size = IntSize(14, 14);
> -    style->setWidth(Length(size.width(), Fixed));
> -    style->setHeight(Length(size.height(), Fixed));
> +    gint width, height;
> +    gtk_icon_size_lookup(GTK_ICON_SIZE_MENU, &width, &height);
> +    style->setWidth(Length(width, Fixed));
> +    style->setHeight(Length(height, Fixed));

Instead of basing this on the size of the icon, it should be based on the height / width of the search box. Otherwise the icon might paint outside the box or be very small in relation to the field size.

> WebCore/platform/gtk/RenderThemeGtk.cpp:504
> +    static Image* searchImage = loadStockIcon(GTK_WIDGET(gtkEntry()), GTK_STOCK_FIND, GTK_ICON_SIZE_MENU).releaseRef();

Now might be a good time to make searchImage a member of RenderThemeGtk so that it can be properly freed.

> WebCore/platform/gtk/RenderThemeGtk.cpp:524
> +    static Image* cancelImage = loadStockIcon(GTK_WIDGET(gtkEntry()), GTK_STOCK_CLEAR, GTK_ICON_SIZE_MENU).releaseRef();

Maybe time to make this a member as well?

[Carlos Garcia Campos]

*** This bug has been marked as a duplicate of bug 50623 ***