Closed Bug 1319650 Opened 7 years ago Closed 7 years ago

mimic gtk_style_context_save() in WidgetStyleCache

Categories

(Core :: Widget: Gtk, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

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.
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.
Attachment #8813499 - Flags: review?(stransky)
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 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.
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
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.
(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.
Depends on: 1320860
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 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 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 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...
Priority: -- → P2
Whiteboard: tpi:+
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.
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 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 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.
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
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)
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
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.
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)
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
Blocks: 1222704, 1269984
Depends on: 1319863
Depends on: 1322422
Depends on: 1323616
Depends on: 1323656
Depends on: 1323860
Depends on: 1362170
Attachment #8815170 - Attachment is obsolete: true
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
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
No longer blocks: 1222704, 1269984
https://hg.mozilla.org/mozilla-central/rev/17b3d389168e
https://hg.mozilla.org/mozilla-central/rev/4b5cc65019d8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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?
Flags: needinfo?(karlt)
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)
Blocks: 1396722
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: