Summary: | [GTK] Use gtk_icon_set_render_icon() to render icons in RenderThemeGtk | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, mrobinson, pnormand, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2010-12-07 04:24:56 PST
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 "[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 "[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 "[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 |