[webkit-reviews] review denied: [Bug 50225] [GTK] Use gtk_widget_render_icon() to render media buttons using gtk stock icons : [Attachment 75368] New patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 2 05:03:59 PST 2010
Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 50225: [GTK] Use gtk_widget_render_icon() to render media buttons using gtk
stock icons
https://bugs.webkit.org/show_bug.cgi?id=50225
Attachment 75368: New patch
https://bugs.webkit.org/attachment.cgi?id=75368&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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?
More information about the webkit-reviews
mailing list