Make SimpleResizeReflow not flush by default.
Categories
(Core :: Layout, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox106 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
Details
Attachments
(2 files)
Assignee | ||
Comment 1•2 years ago
|
||
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 3•2 years ago
|
||
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
Assignee | ||
Comment 4•2 years ago
|
||
I couldn't reproduce this locally, and I can't see any obvious ResizeObserver
s 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).
Assignee | ||
Comment 5•2 years ago
|
||
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.
Assignee | ||
Comment 6•2 years ago
|
||
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 9•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/594bd45928b7
https://hg.mozilla.org/mozilla-central/rev/9c804d2af20e
Comment 10•2 years ago
|
||
The issue with the initial patch has been resolved with the patch attached in comment 5.
Description
•