Summary: | [GTK] Port stock icon painting to GtkStyleContext | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, cgarcia, commit-queue, eric, webkit.review.bot, xan.lopez | ||||||||||||
Priority: | P3 | Keywords: | Gtk | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 50820, 51812, 51815, 51828, 51830, 51870, 51874 | ||||||||||||||
Attachments: |
|
Description
Martin Robinson
2010-12-30 16:31:29 PST
Created attachment 77700 [details]
Patch
Comment on attachment 77700 [details]
Patch
Removing review flag as there are some GTK+ 3.x issues.
Created attachment 77728 [details]
Patch with build fixes for GTK+ 3.x
+static void gtkStyleChangedCallback(GObject*, GParamSpec*) +{ + StyleContextMap::const_iterator end = styleContextMap().end(); + for (StyleContextMap::const_iterator iter = styleContextMap().begin(); iter != end; ++iter) + gtk_style_context_invalidate(iter->second.get()); + + RenderTheme::themeForPage(0)->platformColorsDidChange(); +} I would use RenderTheme::defaultTheme() instead, it's exactly the same but less confusing. But maybe we can just call Page::scheduleForcedStyleRecalcForAllPages(); to avoid creating a new RenderTheme only to call platformColorsDidChange(); Created attachment 77798 [details]
Updated patch using Page::scheduleForcedStyleRecalcForAllPages()
Comment on attachment 77798 [details] Updated patch using Page::scheduleForcedStyleRecalcForAllPages() View in context: https://bugs.webkit.org/attachment.cgi?id=77798&action=review r- in case we can avoid searching twice. > WebCore/platform/gtk/RenderThemeGtk3.cpp:85 > + GtkWidgetPath could possibly be added to our GOwnPtr stuff at some point. > WebCore/platform/gtk/RenderThemeGtk3.cpp:91 > + return iter->second.get(); Instead of doing contains() + find() I think you could just do one find() and then check that the iter is valid. Seems pointless to search twice? Created attachment 77892 [details]
Updated patch according to review
Comment on attachment 77892 [details] Updated patch according to review View in context: https://bugs.webkit.org/attachment.cgi?id=77892&action=review > WebCore/platform/gtk/RenderThemeGtk3.cpp:50 > +typedef HashMap<GType, PlatformRefPtr<GtkStyleContext> > StyleContextMap; This must be updated to say GRefPtr for this to land cleanly. PlatformRefPtr was renamed yesterday. > WebCore/platform/gtk/RenderThemeGtk3.cpp:78 > + std::pair<StyleContextMap::iterator, bool>result = styleContextMap().add(widgetType, 0); Missing a space here before 'result'. > WebCore/platform/gtk/RenderThemeGtk3.cpp:85 > + PlatformRefPtr<GtkStyleContext> context = adoptPlatformRef(gtk_style_context_new()); This should be GRefPtr and adoptGRef now. > WebCore/platform/gtk/RenderThemeGtk3.cpp:89 > + result.first->second = context; Wow. Does modifying the iterator actually update the hash table? I think it's much clearer anyway to just do an insertion. > WebCore/platform/gtk/RenderThemeGtk3.cpp:418 > +PlatformRefPtr<GdkPixbuf> RenderThemeGtk::getStockIcon(GType widgetType, const char* iconName, gint direction, gint state, gint iconSize) This must be updated to return GRefPtr now. > WebCore/platform/gtk/RenderThemeGtk3.cpp:437 > + return adoptPlatformRef(icon); Ditto. Created attachment 77898 [details]
Updated patch fixing coding style
PlatformRefPtr is still available in current git master.
Comment on attachment 77898 [details]
Updated patch fixing coding style
Looks good to me, but I think that Xan has to pull the lever officially since I worked on this.
Comment on attachment 77898 [details]
Updated patch fixing coding style
*pulls lever*
Comment on attachment 77898 [details] Updated patch fixing coding style Clearing flags on attachment: 77898 Committed r74997: <http://trac.webkit.org/changeset/74997> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/74997 might have broken Leopard Intel Debug (Tests) |