1824236 - Stop using XUL layout for scrollbars.

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, task, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1824236 - Stop using XUL layout for scrollbars. r=jwatt

This rewrites scrollbar layout to work with regular reflow rather than
box layout.

Overall it's about the same amount of code (mostly because
nsScrollbarFrame::Reflow is sorta hand-rolled), but it cleans up a bit
and it is progress towards removing XUL layout altogether, without
getting into much deeper refactoring.

The setup of abusing the available size in ReflowInput to make sizing
decisions on the children feels a bit funky, but I think scrollbars are
special enough that it probably doesn't matter much... Otherwise I
would've had to rewrite the thumb sizing code which is a bit gnarly.

This also blocks some other performance improvements and refactorings I
want to make in this code.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/jobs?repo=try&revision=71649249479899d414b9acd4aa078a4a93888923

Gah, so Linux and macOS are green, but it seems I still need some work to do on Windows / Android, for some reason.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(It's fully green now fwiw)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7da2469ac949
Stop using XUL layout for scrollbars. r=jwatt

Comment 5

[Sandor Molnar[:smolnar]]

Backed out for causing assertion failures in layout/generic/crashtests/369038-1.xhtml

Backout link: https://hg.mozilla.org/integration/autoland/rev/e89a49f86a0642a75cabc4f06a62d9841ad54e65

Push with failures

Failure log -> Assertion failure: appearance == StyleAppearance::ScrollbarHorizontal || appearance == StyleAppearance::ScrollbarVertical, at /builds/worker/checkouts/gecko/layout/xul/nsScrollbarFrame.cpp:241

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ab63cda14e8
Stop using XUL layout for scrollbars. r=jwatt

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6885642f5246
Remove a crashtest.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/897bab3fdfc6
Remove a crashtest using <slider> without a scrollbar.

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5ab63cda14e8
https://hg.mozilla.org/mozilla-central/rev/6885642f5246
https://hg.mozilla.org/mozilla-central/rev/897bab3fdfc6

Comment 10

[Andrej (:aglavic)]

== Change summary for alert #37912 (as of Fri, 31 Mar 2023 00:56:37 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
12%	perf_reftest_singletons scrollbar-styles-1.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	327.24 -> 287.24
12%	perf_reftest_singletons scrollbar-styles-1.html	linux1804-64-shippable-qr	e10s fission stylo webrender	444.93 -> 392.41
10%	perf_reftest_singletons scrollbar-styles-1.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	330.34 -> 297.01
10%	perf_reftest_singletons scrollbar-styles-1.html	windows10-64-shippable-qr	e10s fission stylo webrender	490.52 -> 442.27

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37912

Comment 11

[Alex Finder]

(In reply to Sandor Molnar from comment #5)

Backed out for causing assertion failures in layout/generic/crashtests/369038-1.xhtml

Backout link: https://hg.mozilla.org/integration/autoland/rev/e89a49f86a0642a75cabc4f06a62d9841ad54e65

Push with failures

Failure log -> Assertion failure: appearance == StyleAppearance::ScrollbarHorizontal || appearance == StyleAppearance::ScrollbarVertical, at /builds/worker/checkouts/gecko/layout/xul/nsScrollbarFrame.cpp:241

== Change summary for alert #37953 (as of Sat, 01 Apr 2023 06:57:26 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
0.12%	installer size	osx-aarch64-shippable	aarch64 nightly	84,599,407.12 -> 84,702,245.17

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37953