1786513 - Make SimpleResizeReflow not flush by default.

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1786513 - Make SimpleResizeReflow not flush by default. r=tnikkel,jfkthame

Only GeckoMVMContext really needs the flush, to measure scrolled height
afterwards. Do that explicitly.

This shouldn't change behavior, for the most part; there was a preload
test that relied on the flush when changing DPI to start a run really
clean, but other than that this looks green on try.

Should at best be neutral (just code clean-up), or be a performance
improvement.

In a follow-up, we can possibly remove the DelayedResize code from the
view manager, though I need to think how to possibly coalesce the MVM
reflows, so let's not do that yet.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e29b0fbf77f1
Make SimpleResizeReflow not flush by default. r=jfkthame,layout-reviewers

Comment 3

[Cristian Tuns]

Backed out for causing mochitest failures on /browser_unified_extensions.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_unified_extensions.js | uncaught exception - ResizeObserver loop completed with undelivered notifications. at :0:0

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I couldn't reproduce this locally, and I can't see any obvious ResizeObservers that could be involved other than this one. Will, it seems you're somewhat familiar with the code that this test is testing, do you know if I might be missing something? (No rush while on PTO, not super-urgent, thanks).

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1786513 - Don't use ResizeObserver to deal with window resizes in tabbrowser. r=Gijs

This was introduced alongside the MutationObserver1, according to the commit
message to "improve performance through coalesence of 'resize' events."

However, that's false, before this bug, resizes on the top level window would
flush layout and resize the document element instantly so there should be the
exact same amount of resize events as of ResizeObserver notifications. I'm not
sure what coalesence would this achieve.

This is causing the previous patch to get backed out, due to a failure on macOS
that I haven't been able to reproduce.

It's likely because on chrome windows some document element resizes can trigger
window resizes due to the size constraint propagation we have, so nothing
super-concerning or new all-in-all, my patch just changed the timing of how
this happened.

Also, clean up the MutationObserver and event listener properly while at it.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/logviewer?job_id=388923924&repo=try&lineNumber=5172 shows that that resizeobserver is indeed the relevant one to fire the error. I think I understand how it could happen, but I don't think it's problematic and I don't see a reason to use ResizeObserver there to begin with, so I think we should just remove it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/594bd45928b7
Don't use ResizeObserver to deal with window resizes in tabbrowser. r=Gijs

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c804d2af20e
Make SimpleResizeReflow not flush by default. r=jfkthame,layout-reviewers

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/594bd45928b7
https://hg.mozilla.org/mozilla-central/rev/9c804d2af20e

Comment 10

[William Durand [:willdurand]]

The issue with the initial patch has been resolved with the patch attached in comment 5.