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.
Created attachment 75798 [details] Patch to use gtk_icon_set_render_icon() With this patch bugs #50225 and #50226 can be closed too.
*** Bug 50226 has been marked as a duplicate of this bug. ***
*** Bug 50225 has been marked as a duplicate of this bug. ***
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.
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.
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.
(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.
Created attachment 75890 [details] Updated patch addressing martin's comments
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.
Created attachment 75901 [details] A new update fixing more issues
Comment on attachment 75901 [details] A new update fixing more issues Thanks!
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 loislo@chromium.org and podivilov@chromium.org. The commit-queue is continuing to process your patch.
Comment on attachment 75901 [details] A new update fixing more issues Clearing flags on attachment: 75901 Committed r73561: <http://trac.webkit.org/changeset/73561>
All reviewed patches have been landed. Closing bug.
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