Bug 51764 - [GTK] Port stock icon painting to GtkStyleContext
Summary: [GTK] Port stock icon painting to GtkStyleContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 50820 51812 51815 51828 51830 51870 51874
  Show dependency treegraph
 
Reported: 2010-12-30 16:31 PST by Martin Robinson
Modified: 2011-01-04 14:12 PST (History)
6 users (show)

See Also:


Attachments
Patch (11.39 KB, patch)
2010-12-30 17:32 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch with build fixes for GTK+ 3.x (11.43 KB, patch)
2010-12-31 10:52 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Updated patch using Page::scheduleForcedStyleRecalcForAllPages() (11.10 KB, patch)
2011-01-03 02:20 PST, Carlos Garcia Campos
xan.lopez: review-
Details | Formatted Diff | Diff
Updated patch according to review (11.06 KB, patch)
2011-01-04 09:13 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch fixing coding style (11.06 KB, patch)
2011-01-04 10:28 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2010-12-30 16:31:29 PST
This bug tracks porting only the stock icon painting to the new GtkStyleContext API. Since this is the first patch which uses GtkStyleContext, it will also include the initial machinary for all GtkStyleContext usage.
Comment 1 Martin Robinson 2010-12-30 17:32:40 PST
Created attachment 77700 [details]
Patch
Comment 2 Martin Robinson 2010-12-31 09:32:38 PST
Comment on attachment 77700 [details]
Patch

Removing review flag as there are some GTK+ 3.x issues.
Comment 3 Martin Robinson 2010-12-31 10:52:18 PST
Created attachment 77728 [details]
Patch with build fixes for GTK+ 3.x
Comment 4 Carlos Garcia Campos 2011-01-03 00:48:19 PST
+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();
Comment 5 Carlos Garcia Campos 2011-01-03 02:20:07 PST
Created attachment 77798 [details]
Updated patch using Page::scheduleForcedStyleRecalcForAllPages()
Comment 6 Xan Lopez 2011-01-04 04:15:29 PST
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?
Comment 7 Carlos Garcia Campos 2011-01-04 09:13:20 PST
Created attachment 77892 [details]
Updated patch according to review
Comment 8 Martin Robinson 2011-01-04 09:17:58 PST
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.
Comment 9 Carlos Garcia Campos 2011-01-04 10:28:53 PST
Created attachment 77898 [details]
Updated patch fixing coding style

PlatformRefPtr is still available in current git master.
Comment 10 Martin Robinson 2011-01-04 10:31:41 PST
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 11 Xan Lopez 2011-01-04 10:34:08 PST
Comment on attachment 77898 [details]
Updated patch fixing coding style

*pulls lever*
Comment 12 WebKit Commit Bot 2011-01-04 13:09:38 PST
Comment on attachment 77898 [details]
Updated patch fixing coding style

Clearing flags on attachment: 77898

Committed r74997: <http://trac.webkit.org/changeset/74997>
Comment 13 WebKit Commit Bot 2011-01-04 13:09:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Review Bot 2011-01-04 14:12:24 PST
http://trac.webkit.org/changeset/74997 might have broken Leopard Intel Debug (Tests)