1624247 - Flexbox refactoring to prepare for caching reflowed sizes of flex items

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, task, P3)

Description

[Daniel Holbert [:dholbert]]

Filing this bug for some refactoring patches that support bug 1492538.

(These will need a rebase once bug 1623225 has landed; I'll keep an eye on that bug and rebase when that happens.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1624247 part 1: When caching flex item BSize measurement, cache the content-box size instead of border-box size. r?TYLin

This patch should not impact behavior; it is purely a refactoring change to do the same arithmetic in a different spot.

The content-box size is the size that we actually care about when we use this
cached data, so we might as well just store that size directly, instead of
storing the border-box size and making adjustments at usage time, which is what
we do right now.

Note that one of the usage sites had an informational NS_WARNING_ASSERTION, for
cases where the BorderPadding subtraction might drop us into negative
territory. I'm not particularly concerned with preserving that warning at this
point (in part because it's not necessarily an error when it fails, since it's
possible to make it fail with huge sizes; and in part because it's non-fatal,
which means we're not likely to notice it if it did "legitimately" fail
anyway). So I've just removed it for simplicity & consistency.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1624247 part 2: Rename flex item helper-struct from CachedMeasuringReflowResult to CachedBAxisMeasurement. r?TYLin

This patch should not impact behavior; it is purely a rename.

The "reflow result" naming is too generic for this struct, now that I'm
planning to cache other results from flex item reflow (in later patches).

This particular struct is caching block-axis measurements (a BSize and an ascent), so let's rename it to reflect that.

Depends on D67800

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1624247 part 3: Refactor CachedBAxisMeasurement to be an optional member in a more general CachedFlexItemData class. r?TYLin

This patch should not impact behavior; it is purely a refactoring change.

In a later patch, I will add more (non-optional) members to this new class, to
cache additional data.

One happy outcome of the current patch is that CachedBAxisMeasurement's members
can now become (mostly) 'const' again, now that we can reconstruct it in-place
by virtue of it being stored in a Maybe<>.

Depends on D67801

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

(These will need a rebase once bug 1623225 has landed; I'll keep an eye on that bug and rebase when that happens.)

I've now rebased them on top of bug 1623225's patch-stack (imported from phabricator), so this should apply cleanly when it's ready to land.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

In Part 1's extended commit message,

The content-box size is the size that we actually care about when we use this cached data

This reminds me of bug 1614888, where the flex item has percentage padding, so the padding's size depends on the flex container's width. When flex container's width shrinks, the flex item's border-box changes, but not its content-box.

I know it is an existing bug, but is the refactoring direction we are heading makes such bug harder to fix?

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1624247 part 4: Refactor our existing logic for skipping some flex items' "final reflow" when they've had an earlier measuring reflow. r?TYLin

This patch isn't expected to change behavior; it's just refactoring some logic
into a helper function (and inverting some conditions to hopefully make things
easier to follow & reason about).

The new function here ("NeedsFinalReflow()") is intended to be extended later
on, to add more cases where it can return false & cause the final reflow to be
skipped. (This will probably happen in bug 1492538.)

Note that this patch also moves a static helper function
"FrameHasRelativeBSizeDependency()" upwards in the file; this is required in
order to keep that function declared/defined above its first usage (which has
now moved upwards as well). This helper isn't changing at all; it's just
moving.

Depends on D67802

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #5)

This reminds me of bug 1614888, where the flex item has percentage padding, so the padding's size depends on the flex container's width. When flex container's width shrinks, the flex item's border-box changes, but not its content-box.

Good food for thought. A few thoughts in response:

• To the extent that this is a problem, this bug might actually fix the problem...? Previously we were caching the measured border-box BSize, including padding, which could theoretically be bogus if we use this cached measurement in later reflows and subtract off a larger/smaller resolved padding to determine an incorrect measured content-box-BSize value. Now, with this patch's changes, we'll be caching the content-box BSize as our measurement and using that in later reflows (and that shouldn't change when the available ISize changes such that percent padding is resolved differently).

• I know this isn't the scenario you're discussing, but in the same neighborhood: I'm not concerned with restyles of padding (or border or width/height/etc) since those should trigger FrameNeedsReflow calls, via NS_STYLE_HINT_REFLOW e.g. here, which results in MarkIntrinsicISizesDirty() being called (which purges this cached data).

• If this turns out to be a problem for measurements (i.e. if bug 1614888 is in fact hitting a version of this precise problem where a measurement is bogus due to such a change in the flex container's ISize impacting a percent BSize), then we can extend the "key" part of this struct to include the computed ISize of the flex container (the containing block ISize for the flex item).

• In bug 1492538, we'll definitely need to be careful to ensure that we do perform a "final" reflow when there are any such changes (to the computed ISize of the flex container), to ensure that we leave the frame tree in the correct state to be painted (with a larger/smaller frame size to account for a larger/smaller resolved percent padding, for example). We probably just need to check ReflowInput::IsIResize() on the nsFlexContainerFrame's ReflowInput to handle this particular scenario.

Comment 8

[Daniel Holbert [:dholbert]]

Sanity-check try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b5075246bbd36c0b3018843ae5b5036f3036f6f

I see you wrote bug 1624514 to layer on top of this - thanks for the bitrot consideration! Assuming that try run succeeds, I'll land this in the morning and then you can land bug 1624514.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/782a1cdba0ae
part 1: When caching flex item BSize measurement, cache the content-box size instead of border-box size. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/7d8eddd3ede1
part 2: Rename flex item helper-struct from CachedMeasuringReflowResult to CachedBAxisMeasurement. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/a03ed46ab68a
part 3: Refactor CachedBAxisMeasurement to be an optional member in a more general CachedFlexItemData class. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/3bd0695c40a1
part 4: Refactor our existing logic for skipping some flex items' "final reflow" when they've had an earlier measuring reflow. r=TYLin

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/782a1cdba0ae
https://hg.mozilla.org/mozilla-central/rev/7d8eddd3ede1
https://hg.mozilla.org/mozilla-central/rev/a03ed46ab68a
https://hg.mozilla.org/mozilla-central/rev/3bd0695c40a1