Closed Bug 1786513 Opened 2 years ago Closed 2 years ago

Make SimpleResizeReflow not flush by default.

Categories

(Core :: Layout, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
106 Branch
Tracking Status
firefox106 --- fixed

People

(Reporter: emilio, Assigned: emilio)

Details

Attachments

(2 files)

No description provided.

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.

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e29b0fbf77f1 Make SimpleResizeReflow not flush by default. r=jfkthame,layout-reviewers

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
Flags: needinfo?(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).

Flags: needinfo?(emilio) → needinfo?(wdurand)

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.

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.

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/594bd45928b7 Don't use ResizeObserver to deal with window resizes in tabbrowser. r=Gijs
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9c804d2af20e Make SimpleResizeReflow not flush by default. r=jfkthame,layout-reviewers
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch

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

Flags: needinfo?(wdurand)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: