Closed
Bug 1319650
Opened 8 years ago
Closed 8 years ago
mimic gtk_style_context_save() in WidgetStyleCache
Categories
(Core :: Widget: Gtk, defect, P2)
Core
Widget: Gtk
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: karlt, Assigned: karlt)
References
(Depends on 1 open bug)
Details
(Whiteboard: tpi:+)
Attachments
(3 files, 1 obsolete file)
We can construct a GtkStyleContext that matches the same CSS rules as a
context that has been passed to gtk_style_context_save().
Doing this would mean that balancing with gtk_style_context_restore()/ReleaseStyleContext() would be unnecessary, and the style resolution cached in the style contexts would not be invalidated so frequently.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
My intention is to follow-up with removal of ReleaseStyleContext() and s/ClaimStyleContext/GetStyleContext/ but that is likely to conflict with other changes and so could be easier to do separately.
Assignee | ||
Updated•8 years ago
|
Attachment #8813499 -
Flags: review?(stransky)
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8813499 [details]
bug 1319650 mimic gtk_style_context_save() in WidgetStyleCache with a new context
https://reviewboard.mozilla.org/r/94918/#review95506
::: widget/gtk/WidgetStyleCache.cpp:823
(Diff revision 1)
> + GtkStyleContext* parentStyle = GetWidgetRootStyle(aWidgetType);
> +
> + // Create a new context that behaves like the parentStyle would after
> + // gtk_style_context_save(parentStyle).
> + //
> + // gtk_style_context_save() creates a copy of the GtkCssNodeDeclaration of
That comment is a bit confusing to me.
gtk_style_context_save() actually duplicates the last element on the path so it's listed twice and this one is updated. We generally want to avoid this behavior here to get rid of https://bugzilla.gnome.org/show_bug.cgi?id=761870#c2 and so.
What we do here is that we copy the entire style (no extra entries added) so we have a standalone copy what can be modified freely.
::: widget/gtk/WidgetStyleCache.cpp:1209
(Diff revision 1)
> //
> - // Avoid calling invalidate on saved contexts to avoid performing
> - // build_properties() (in 3.16 stylecontext.c) unnecessarily early.
> - if (stateChanged && !sStyleContextNeedsRestore) {
> + // Avoid calling invalidate on contexts that are not owned and constructed
> + // by widgets to avoid performing build_properties() (in 3.16 stylecontext.c)
> + // unnecessarily early.
> + if (stateChanged && sWidgetStorage[aNodeType]) {
> gtk_style_context_invalidate(style);
Can we add a gtk version check here and call gtk_style_context_invalidate() for gtk < 3.18 only?
Attachment #8813499 -
Flags: review?(stransky) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8813499 [details]
bug 1319650 mimic gtk_style_context_save() in WidgetStyleCache with a new context
https://reviewboard.mozilla.org/r/94918/#review95506
> That comment is a bit confusing to me.
>
> gtk_style_context_save() actually duplicates the last element on the path so it's listed twice and this one is updated. We generally want to avoid this behavior here to get rid of https://bugzilla.gnome.org/show_bug.cgi?id=761870#c2 and so.
>
> What we do here is that we copy the entire style (no extra entries added) so we have a standalone copy what can be modified freely.
The code here does actually add the extra entry, like
gtk_style_context_save(), intentionally to produce the same behavior.
I'll expand the comment to explain better.
> Can we add a gtk version check here and call gtk_style_context_invalidate() for gtk < 3.18 only?
I suspect that would be possible, but I don't know the associated GTK internals and so the failure to reproduce bug 1272194 is the only reason to suspect that. The invalidate should be mostly harmless and will go away once style contexts are no longer borrowed from GtkWidgets, so I don't think there's a need to be overly conservative here.
Assignee | ||
Comment 6•8 years ago
|
||
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb4be9ed211e
mimic gtk_style_context_save() in WidgetStyleCache with a new context r=stransky+263117
Comment 8•8 years ago
|
||
Comment 9•8 years ago
|
||
I wonder which elements fails the reftest, is it possible to dig it from the results somewhere? From a quick look (I used https://bugzilla.mozilla.org/query.cgi?query_format=advanced) it looks the same on Gtk 3.16 and the patch fixes combobox dropdown (the list) rendering on Gtk 3.22.
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Martin Stránský from comment #9)
> I wonder which elements fails the reftest, is it possible to dig it from the
> results somewhere?
Clicking on a reftest failure on treeherder will open a panel at the bottom.
There is a bar graph icon, clicking on which opens the reftest analyser. e.g.
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/ISvBmIjaQSy5Jn05eNtX3A/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
Some uses of contexts in gtk3drawing.c are depending on the
style_context_restore() to remove temporary classes.
I'm working on some fixes there.
> the patch fixes combobox dropdown (the list) rendering on Gtk 3.22.
I wonder whether that is using animations, or if there is some other reason
for the change in behaviour.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68fffd525df207612c770da42da872a49905d044
(In reply to Karl Tomlinson (:karlt) from comment #13)
> bug 1319650 mimic gtk_style_context_save() in WidgetStyleCache with a new
> context
>
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/94918/diff/2-3/
This is just a rebase, which involved similar changes to some new cases.
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8815170 [details]
bug 1319650 implement pre-3.20 MOZ_GTK_PROGRESS_CHUNK in WidgetStyleCache
https://reviewboard.mozilla.org/r/96200/#review96762
Makes sense, Thanks!
Attachment #8815170 -
Flags: review?(stransky) → review+
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8813499 [details]
bug 1319650 mimic gtk_style_context_save() in WidgetStyleCache with a new context
https://reviewboard.mozilla.org/r/94918/#review96764
I discussed that with Company and he suggested that for full _save() emulation we also need to copy state flags as _save() does. The state cache mechanism we use in ClaimStyleContext() may be affected with that which could be a cause of failed tests.
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8815171 [details]
bug 1319650 draw tab gap with tabpanel style context
https://reviewboard.mozilla.org/r/96202/#review96856
I'd need to find some tabpanels application in Firefox while the tabs from certificates dialog went away...
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: tpi:+
Assignee | ||
Comment 18•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8813499 [details]
bug 1319650 mimic gtk_style_context_save() in WidgetStyleCache with a new context
https://reviewboard.mozilla.org/r/94918/#review96764
Yes, _save() does copy the state flags, but the state for this context will be set with the flags passed to ClaimStyleContext(), and so there is no value in setting it here. The state on |parentStyle| may be quite unrelated to what is passed to ClaimStyleContext().
Our current model sets the state only on the context returned by ClaimStyleContext(). I assume themes may also match against state of a parent context, which is not explicitly managed. That is unchanged by this patch, as the state was also not set on the widget root context before calling _save(). If themes are matching on ancestor node state, then we may be getting away with that because the ancestor is drawn behind, and so before, the descendant.
The unrestored modifications to style contexts were the cause of the failing reftests. That is addressed by the additional patches and bug 1320860.
Assignee | ||
Comment 19•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8815171 [details]
bug 1319650 draw tab gap with tabpanel style context
https://reviewboard.mozilla.org/r/96202/#review96856
If you add a boolean pref with name dom.allow_XUL_XBL_for_file and value true, then layout/reftests/native-theme/470711-1.xul will draw a tab panel.
There are things that are not right in the rendering, but I'm not seeing (nor really expecting) a change in behavior.
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8815171 [details]
bug 1319650 draw tab gap with tabpanel style context
https://reviewboard.mozilla.org/r/96202/#review97094
Attachment #8815171 -
Flags: review?(stransky) → review+
Comment 21•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8813499 [details]
bug 1319650 mimic gtk_style_context_save() in WidgetStyleCache with a new context
https://reviewboard.mozilla.org/r/94918/#review96764
Good, Thanks, I din't know that the failure was caused by the missing text window there.
Comment 22•8 years ago
|
||
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83021e7eb511
implement pre-3.20 MOZ_GTK_PROGRESS_CHUNK in WidgetStyleCache r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/70d7c1534715
mimic gtk_style_context_save() in WidgetStyleCache with a new context r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/dceea26b46ae
draw tab gap with tabpanel style context r=stransky+263117
Comment 23•8 years ago
|
||
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dceea26b46aecc0920f44128cde85539107b4dd0
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7596767&repo=autoland
[task 2016-12-01T20:54:38.331324Z] 20:54:38 INFO - TEST-PASS | layout/base/tests/chrome/test_printpreview.xul | Comparing print preview didn't succeed [<input type='reset'> : <input type='reset'>]
[task 2016-12-01T20:54:38.332050Z] 20:54:38 INFO - TEST-PASS | layout/base/tests/chrome/test_printpreview.xul | Should have called beforeprint listener!
[task 2016-12-01T20:54:38.332626Z] 20:54:38 INFO - TEST-PASS | layout/base/tests/chrome/test_printpreview.xul | Should have called afterprint listener!
[task 2016-12-01T20:54:38.333240Z] 20:54:38 INFO - TEST-PASS | layout/base/tests/chrome/test_printpreview.xul | Should have called beforeprint listener!
[task 2016-12-01T20:54:38.333777Z] 20:54:38 INFO - TEST-PASS | layout/base/tests/chrome/test_printpreview.xul | Should have called afterprint listener!
[task 2016-12-01T20:54:38.333853Z] 20:54:38 INFO - Buffered messages finished
[task 2016-12-01T20:54:38.334449Z] 20:54:38 INFO - TEST-UNEXPECTED-FAIL | layout/base/tests/chrome/test_printpreview.xul | Comparing print preview didn't succeed [<input type='checkbox'> : <input type='checkbox'>] - got false, expected true
[task 2016-12-01T20:54:38.334995Z] 20:54:38 INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:271:5
[task 2016-12-01T20:54:38.335095Z] 20:54:38 INFO - compareFormElementPrint@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:220:3
[task 2016-12-01T20:54:38.335668Z] 20:54:38 INFO - runTest3@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:195:5
[task 2016-12-01T20:54:38.336254Z] 20:54:38 INFO - setTimeout handler*compareFormElementPrint@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:222:3
[task 2016-12-01T20:54:38.336830Z] 20:54:38 INFO - setTimeout handler*compareFormElementPrint@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:222:3
[task 2016-12-01T20:54:38.337395Z] 20:54:38 INFO - setTimeout handler*compareFormElementPrint@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:222:3
[task 2016-12-01T20:54:38.337504Z] 20:54:38 INFO - setTimeout handler*compareFormElementPrint@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:222:3
[task 2016-12-01T20:54:38.338068Z] 20:54:38 INFO - setTimeout handler*compareFormElementPrint@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:222:3
[task 2016-12-01T20:54:38.338612Z] 20:54:38 INFO - setTimeout handler*compareFormElementPrint@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:222:3
[task 2016-12-01T20:54:38.339162Z] 20:54:38 INFO - setTimeout handler*runTest2@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:156:3
[task 2016-12-01T20:54:38.339716Z] 20:54:38 INFO - setTimeout handler*finalizeTest1@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:148:3
[task 2016-12-01T20:54:38.339819Z] 20:54:38 INFO - setTimeout handler*startTest1@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:138:3
[task 2016-12-01T20:54:38.341004Z] 20:54:38 INFO - runTests@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:87:5
[task 2016-12-01T20:54:38.341090Z] 20:54:38 INFO - onload@chrome://mochitests/content/chrome/layout/base/tests/chrome/printpreview_helper.xul:1:1
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7596769&repo=autoland
[task 2016-12-01T21:07:04.350391Z] 21:07:04 INFO - TEST-PASS | editor/libeditor/tests/browser_bug629172.js | The dir attribute must be correctly updated - "ltr" == "ltr" -
[task 2016-12-01T21:07:04.352349Z] 21:07:04 INFO - TEST-PASS | editor/libeditor/tests/browser_bug629172.js | input event count must be 1 after - 1 == 1 -
[task 2016-12-01T21:07:04.354674Z] 21:07:04 INFO - Buffered messages finished
[task 2016-12-01T21:07:04.361024Z] 21:07:04 INFO - TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browser_bug629172.js | Textarea should appear correctly after switching the direction (rtl) - false == true -
[task 2016-12-01T21:07:04.363199Z] 21:07:04 INFO - Stack trace:
[task 2016-12-01T21:07:04.366268Z] 21:07:04 INFO - chrome://mochikit/content/tests/BrowserTestUtils/content-task.js line 52 > eval:null:10
[task 2016-12-01T21:07:04.367892Z] 21:07:04 INFO - chrome://mochikit/content/tests/BrowserTestUtils/content-task.js:null:53
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7596255&repo=autoland
TEST-UNEXPECTED-FAIL | layout/base/tests/test_reftests_with_caret.html | reftest comparison: == http://mochi.test:8888/tests/layout/base/tests/bug106855-1.html http://mochi.test:8888/tests/layout/base/tests/bug106855-1-ref.html
Flags: needinfo?(karlt)
Comment 24•8 years ago
|
||
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/78e5f82eb83a
Backed out changeset dceea26b46ae
https://hg.mozilla.org/integration/autoland/rev/58e2fd0bf04d
Backed out changeset 70d7c1534715
https://hg.mozilla.org/integration/autoland/rev/6368fb6d8a09
Backed out changeset 83021e7eb511 for various form styling issues on Linux. r=backout
Assignee | ||
Comment 25•8 years ago
|
||
mozreview-review |
Comment on attachment 8813499 [details]
bug 1319650 mimic gtk_style_context_save() in WidgetStyleCache with a new context
https://reviewboard.mozilla.org/r/94918/#review97294
::: widget/gtk/WidgetStyleCache.cpp:985
(Diff revision 3)
> case MOZ_GTK_TAB_TOP:
> {
> // TODO - create from CSS node
> - style = GetWidgetStyleWithClass(MOZ_GTK_NOTEBOOK,
> + style = CreateSubStyleWithClass(MOZ_GTK_NOTEBOOK,
> GTK_STYLE_CLASS_TOP);
> gtk_style_context_add_region(style, GTK_STYLE_REGION_TAB,
> static_cast<GtkRegionFlags>(0));
> return style;
This should break, not return, so that the style is cached, and not leaked. I'll update this and MOZ_GTK_TAB_BOTTOM.
This was incorrect for >3.20 only.
Assignee | ||
Comment 26•8 years ago
|
||
The failures for the landing in comment 22 were two existing intermittents,
bug 1222704 and bug 1269984, which became much more frequent, and a similar asan-only
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browser_bug629172.js | Textarea
should appear correctly after switching the direction (rtl) - false == true
These failures can be caused the GTK 3.4 bug described in
https://bugzilla.mozilla.org/show_bug.cgi?id=1223198#c23
I'll add a patch to work around that.
Flags: needinfo?(karlt)
Assignee | ||
Comment 27•8 years ago
|
||
The workaround is avoiding bug 1222704 and bug 1269984, but there are still
sometimes failures seen only with asan e10s:
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browser_bug629172.js | Textarea should appear correctly after switching back the direction (ltr) - false == true -
TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browser_bug629172.js | Textarea should appear correctly after switching the direction (rtl) - false == true -
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6e170d3b823cb0efe0da9e32b91f5faeff270d6
https://treeherder.mozilla.org/#/jobs?repo=try&revision=803c57b7ff4407a24a37721ad829051afe0e3ad8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b27acfd6f4bef37aefbba21172d9e818aeeb84&selectedJob=32198438
Additional logging here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea6bc9447455698ffaf4f6d4fb49cc8c40d90e8a
Padding and border-radius are sometimes incorrect, and at least one other
property is causing the border not to be drawn at all sometimes. This
behaviour is consistent with bug 1223198 comment 27. It doesn't affect Ubuntu
16.04
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d84431d562e4e6da8027cc8ab4b908fb516d9c6
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8815170 -
Attachment is obsolete: true
Assignee | ||
Comment 30•8 years ago
|
||
Rebase involved no conflicts. Attachment 8815170 [details] landed for bug 1323860.
Bug 1323616 is now fixed by changes for bug 1319863, and so ASAN results are now looking good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dc6c5e91fb42c78099d83840802cf9f447f1a04
Comment 31•8 years ago
|
||
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17b3d389168e
mimic gtk_style_context_save() in WidgetStyleCache with a new context r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/4b5cc65019d8
draw tab gap with tabpanel style context r=stransky+263117
Assignee | ||
Updated•8 years ago
|
Comment 32•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/17b3d389168e
https://hg.mozilla.org/mozilla-central/rev/4b5cc65019d8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Karl, want to uplift this to beta 54? Or do you think it's better to let the fix go to release with 55?
Assignee | ||
Comment 34•7 years ago
|
||
This is mostly for code cleanup with perhaps a small efficiency improvement.
I'm not aware of an appearance issues fixed by this and the changes are fairly risky,
so no need to uplift, thanks.
Flags: needinfo?(karlt)
You need to log in
before you can comment on or make changes to this bug.
Description
•