Bug 51764 – [GTK] Port stock icon painting to GtkStyleContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 77700 [details]
Patch

[Martin Robinson]

Comment on attachment 77700 [details]
Patch

Removing review flag as there are some GTK+ 3.x issues.

[Martin Robinson]

Created attachment 77728 [details]
Patch with build fixes for GTK+ 3.x

[Carlos Garcia Campos]

+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();

[Carlos Garcia Campos]

Created attachment 77798 [details]
Updated patch using Page::scheduleForcedStyleRecalcForAllPages()

[Xan Lopez]

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?

[Carlos Garcia Campos]

Created attachment 77892 [details]
Updated patch according to review

[Martin Robinson]

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.

[Carlos Garcia Campos]

Created attachment 77898 [details]
Updated patch fixing coding style

PlatformRefPtr is still available in current git master.

[Martin Robinson]

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.

[Xan Lopez]

Comment on attachment 77898 [details]
Updated patch fixing coding style

*pulls lever*

[WebKit Commit Bot]

Comment on attachment 77898 [details]
Updated patch fixing coding style

Clearing flags on attachment: 77898

Committed r74997: <http://trac.webkit.org/changeset/74997>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74997 might have broken Leopard Intel Debug (Tests)