Closed Bug 1577258 Opened 24 days ago Closed 9 days ago

Simplify PresShell::ResizeReflowIgnoreOverride.

Categories

(Core :: Layout, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Whiteboard: [geckoview:m1909])

Attachments

(4 files)

I want this to:

  • Thread all the resizes via that (this includes allowing it to not flush).
  • Making it less scary to touch in general.

This avoids doing wasted work and sending spurious resize
events if this case would be hit.

In practice, it cannot be hit yet, I think, because
callers do check for this and bail out earlier. But
there's no assertion to that respect so this shouldn't
hurt.

This is much easier than the existing ResizeReflowIgnoreOverride function, and
this will allow me to avoid flushing if needed (we already kinda do that
already with the "suppressResizeReflow" thing), which in turn allows me to
consolidate a bunch of the logic for resizes.

The function should be much easier to follow:

  • Set the new viewport (async, doesn't do any work).
  • Invalidate layout as needed due to the viewport change (that is, resize hint
    for the root frame, and invalidate isizes if needed). Also async and doesn't
    do any reflowing itself.
  • Flush layout / do the reflowing all at once. I think we can stop doing this
    much more often now, but that's follow-up work.

Now that this code path is on its own, we can write more straight-forward code.

Going to try landing these as incrementally as possible since this code is non-trivial.

Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/907ee4a0aa61
early-return for noop resizes. r=botond

We have an optimization to avoid an expensive reflow from SetFullZoom, see
mSuppressResizeReflow1.

That was done because we used to do a full synchronous reflow right after. We no
longer do that, but due to that member we also don't invalidate!

My second patch in this bug changes the behavior of that flag so that we don't
synchronously reflow, but we do invalidate. So in turn this test before the
change wasn't really testing the zoomed code-path since it was using the clean
layout from before the zoom operation.

a11y getBounds and co. don't flush layout (they probably should), but since with
my patch we dirty the frame tree, and dirty frames return bogus offsets, the
test starts failing.

Flush layout explicitly to ensure we're testing the zoomed code path.

Regressions: 1560378
No longer regressions: 1560378
Assignee: nobody → emilio
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49ff97acd5ce
Explicitly flush layout in an a11y test. r=eeejay

Adding this bug to GV's September sprint because it blocks Fenix image bug 1551659.

Whiteboard: [geckoview:m1909]

Yeah, sorry, I'll be on PTO until Thursday. I replied, but I'll update the patch following your suggestions before re-requesting review.

Flags: needinfo?(emilio)

Yep, no problem. Thank you for following up!

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37fd8d379530
Add a simpler resize reflow function for when we're not shrink-wrapping. r=bzbarsky
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8eeddedeedb8
Streamline the shrink-wrapping resize reflow code. r=botond
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 9 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.