Bug 162673 – [GTK] Cache RenderThemeGadget hierarchies for rendering themed elements with GTK+ 3.20+

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

[Andre Moreira Magalhaes]

GTK+ styling machinery is currently taking non-trivial amounts of time to render scrollbars on simple pages. The problem is that the current code on ScrollbarThemeGtk creates instances of RenderThemeGadget like there’s no tomorrow :D, and each of those creates a GtkStyleContext and sets it up. Given that GtkStyleContext is designed to be cached we should take advantage of that.

Patch to follow.

[Andre Moreira Magalhaes]

Created attachment 290082 [details]
Patch

[Michael Catanzaro]

Carlos Garcia, do you want to review this? I can do it, but it's rewriting a lot of your code so I guess you might prefer to.

[Carlos Garcia Campos]

Yes, it's in my todo, after a first glance my first thought was that we could try to make this more generic, so that it will work for RenderTheme as well, I guess a page with a lot of forms could be also affected. We used to cache the style contexts, but we removed that I don't remember why.

[Michael Catanzaro]

(In reply to comment #3)
> Yes, it's in my todo, after a first glance my first thought was that we
> could try to make this more generic, so that it will work for RenderTheme as
> well, I guess a page with a lot of forms could be also affected. We used to
> cache the style contexts, but we removed that I don't remember why.

I removed it because it was broken at the time, and I failed to fix it because GtkStyleContext APIs never do what you think (e.g. save() and restore() add or remove style nodes :S) and removing the cache simplified the code quite a bit. I don't mind adding it back if it improves performance and isn't buggy. Ideally we would switch layout tests to use GTK+ 3.22 before doing so, though.

[Michael Catanzaro]

It was in r193896. The commit message there says I intended to check the performance implications on the perf bot, but I never did and the bots are still running GTK+ 3.16 anyway. :)

[Michael Catanzaro]

(In reply to comment #3)
> Yes, it's in my todo, after a first glance my first thought was that we
> could try to make this more generic, so that it will work for RenderTheme as
> well, I guess a page with a lot of forms could be also affected. We used to
> cache the style contexts, but we removed that I don't remember why.

Hm, this bug still exists. We surely have bigger problems right now, but we don't want to forget about this.

[Carlos Garcia Campos]

Comment on attachment 290082 [details]
Patch

I've been looking at this in detail. It's tricky, as usual with GTK+ theming. This approach doesn't really work, mainly because gtk_style_context_save/restore breaks the gadgets hierarchy when using GtkWidgetPath. This might not be a problem for some themes, or some widgets like scrollbars, but will break for sure themes that rely on inheritance when querying css properties, and styles using  information about siblings like the combobox. Adding/removing classes to the GtkStyleContext doesn't work when usingGtkWidgetPath either, classes needs to be present when building the GtkWidgetPath. So, we can't cache style contexts or gadgets, we need to cache hierarchies of gadgets with fixed style classes. Fortunately, setting the state on a GtkStyleContext does work, so we can cache more generic hierarchies and apply the state later. In the particular case of scrollbars we would cache VerticalScrollbarRight, VerticalScrollbarLeft, HorizontalScrollbar, VerticalScrollIndicatorRight, VerticalScrollIndicatorLeft and HorizontalScrollIndicator. In practice, we will only have 4 of those at the same time in the cache.

[Carlos Garcia Campos]

Created attachment 311125 [details]
Patch

[Carlos Garcia Campos]

Created attachment 311127 [details]
Try to fix the build with older GTK+

[Build Bot]

Comment on attachment 311127 [details]
Try to fix the build with older GTK+

Attachment 311127 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3807939

New failing tests:
http/tests/xmlhttprequest/xmlhttprequest-50ms-download-dispatch.html

[Build Bot]

Created attachment 311140 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Michael Catanzaro]

Comment on attachment 311127 [details]
Try to fix the build with older GTK+

View in context: https://bugs.webkit.org/attachment.cgi?id=311127&action=review

I've restored to just skimming this, since I've had it open awaiting review for over a week now, and it looks fine. Thanks for working on this massive effort. Please don't backport it; you never know what odd themes could break.

> Source/WebCore/platform/gtk/RenderThemeWidget.cpp:176
> +    RenderThemeGadget::Info info = {
> +        .type = RenderThemeGadget::Type::Generic,
> +        .name = toggleType == Type::Check ? "checkbutton" : "radiobutton",
> +        .classList = { "text-button" }
> +    };

Let's not use nonstandard designated initializers (see bug #172667)

[Carlos Garcia Campos]

Committed r217702: <http://trac.webkit.org/changeset/217702>