Summary: | [GTK] Cache RenderThemeGadget hierarchies for rendering themed elements with GTK+ 3.20+ | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andre Moreira Magalhaes <andrunko> | ||||||||||
Component: | WebKitGTK | Assignee: | Andre Moreira Magalhaes <andrunko> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bugs-noreply, buildbot, cgarcia, commit-queue, esprehn+autocc, glenn, kondapallykalyan, mcatanzaro | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Andre Moreira Magalhaes
2016-09-28 06:52:04 PDT
Created attachment 290082 [details]
Patch
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. 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. (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. 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. :) (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. 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.
Created attachment 311125 [details]
Patch
Created attachment 311127 [details]
Try to fix the build with older GTK+
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 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
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) Committed r217702: <http://trac.webkit.org/changeset/217702> |