Closed Bug 1367568 Opened 7 years ago Closed 7 years ago

For changes to CSS "overflow" on root elements, we don't need nsChangeHint_AllReflowHints

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(4 files)

Bug 1367568 part 1: Create reusable macros that represent our change hints for tweaks to CSS width or height (ISize/BSize).
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1367568 part 2: Rewrite nsChangeHint_ReflowHintsForBSizeChange in terms of nsChangeHint_AllReflowHints.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1367568 part 3: For CSS "overflow" changes that don't require frame reconstruction, send same change hints as if CSS "height" and "width" changed.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1367568 part 4: Add mochitest to ensure we don't reflow too much for "overflow" changes on <body>.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
In bug 1344398, we changed how we handle CSS "overflow" changes on the root elements (html/body).  Previously we'd always reconstruct frames; now we send nsChangeHint_AllReflowHints in some cases instead.  (Specifically, in cases where the restyled element is propagating its scrollbars to the viewport, and it's not supplanting some other element that was previously doing that).


However: nsChangeHint_AllReflowHints is overly aggressive.  (And it's one cause of continued reflow jank for large Twitter pages in bug 1342220.)  We can get away with sending fewer changehints -- it seems reasonable to send exactly the same hints that we'd send for "height" & "width" simultaneously changing, since the only reason we need to reflow is that the available space is potentially getting a bit smaller/larger to accomodate scrollbars appearing/hiding on the viewport.
Flags: needinfo?(bzbarsky)
co-opting needinfo for review, as discussed. Thanks! :D
High-level overview of patch stack:
 - Part 1 moves the change hint definitions.
 - Part 2 rewrites one change hint definition to be easier to compare to the other one (since they really only differ by one flag)
 - Part 3 makes the actual relevant behavior change for this bug.
 - Part 4 adds a test.
I verified locally that the test *fails* if I revert part 3, too -- it gives me this failure output:
========
failed | Reflow count when setting 'overflow: scroll' on body (20) shouldn't be greater than count for tweaking width/height on body (18)

failed | Reflow count when removing 'overflow: scroll' from body (30) shouldn't be greater than count for tweaking width/height on body (18)

failed | Reflow count when changing 'overflow' on body (20) shouldn't be greater than count for tweaking width/height on body (18)
========

The second failure in particular (comparing 30 frames reflowed vs 18 frames reflowed) seems like a large enough difference to give me confidence that the test *will* be able to warn us if we regress this.

(And with the full patch stack applied, the test passes locally. I'll give it a Try run to be sure this is true on other platforms, as well.)
Comment on attachment 8871488 [details]
Bug 1367568 part 1: Create reusable macros that represent our change hints for tweaks to CSS width or height (ISize/BSize).

https://reviewboard.mozilla.org/r/142908/#review146672

r=me
Attachment #8871488 - Flags: review+
Comment on attachment 8871489 [details]
Bug 1367568 part 2: Rewrite nsChangeHint_ReflowHintsForBSizeChange in terms of nsChangeHint_AllReflowHints.

https://reviewboard.mozilla.org/r/142910/#review146674

r=me
Attachment #8871489 - Flags: review+
Comment on attachment 8871490 [details]
Bug 1367568 part 3: For CSS "overflow" changes that don't require frame reconstruction, send same change hints as if CSS "height" and "width" changed.

https://reviewboard.mozilla.org/r/142912/#review146678

r=me
Attachment #8871490 - Flags: review+
Thanks for the reviews!

I've just updated part 4 to fix some bad alphabetization in the mochitest.ini manifest file.
Comment on attachment 8871491 [details]
Bug 1367568 part 4: Add mochitest to ensure we don't reflow too much for "overflow" changes on <body>.

https://reviewboard.mozilla.org/r/142914/#review146684

r=me
Attachment #8871491 - Flags: review+
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0ffbc62a8603142708a9f602ccd9e3fe9906a96
(not done yet, but looking good)
Flags: needinfo?(bzbarsky)
My Try build's mac test runs are still pending (!) after 14 hours, so I'm going to land this without waiting for them.

Since the reflow counts here are slightly inexact, there's a *small* chance I'll need to add one or two to the test's expected reflow-count, in order for the new test (test_viewport_scrollbar_causing_reflow.html) to pass on Mac.  We'll find out once some mac testruns complete (on Try or Autoland), and I'm happy to post/push a followup if that turns out to be the case.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81c038f7f40c
part 1: Create reusable macros that represent our change hints for tweaks to CSS width or height (ISize/BSize). r=bz
https://hg.mozilla.org/integration/autoland/rev/20d8dd705b0e
part 2: Rewrite nsChangeHint_ReflowHintsForBSizeChange in terms of nsChangeHint_AllReflowHints. r=bz
https://hg.mozilla.org/integration/autoland/rev/26651e1ae6ad
part 3: For CSS "overflow" changes that don't require frame reconstruction, send same change hints as if CSS "height" and "width" changed. r=bz
https://hg.mozilla.org/integration/autoland/rev/f7baae45da00
part 4: Add mochitest to ensure we don't reflow too much for "overflow" changes on <body>. r=bz
Summary: Changes to CSS "overflow" on root elements, we don't need nsChangeHint_AllReflowHints → For changes to CSS "overflow" on root elements, we don't need nsChangeHint_AllReflowHints
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: