nsFlexContainerFrame shouldn't create a reflow state for measuring flex items' cross size, if it doesn't actually need to reflow them to do that measurement

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla38
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Right now, we have this code for sizing flex items in the cross axis:
>3507         nsHTMLReflowState childReflowState(aPresContext, aReflowState,
>3508                                            item->Frame(), availSize);
[...]
>3516         SizeItemInCrossAxis(aPresContext, aAxisTracker,
>3517                             childReflowState, *item);

...where SizeItemInCrossAxis() is basically a no-op, if the cross axis is horizontal. (It just immediately returns childReflowState.ComputedWidth().)

So, we're creating a reflow state, and then possibly throwing it away without actually reflowing. This isn't kosher, now that (in bug 1054010) we may be skipping the item's final reflow.

This reflow state is bad if we don't end up reflowing because nsHTMLReflowState's constructor *makes changes to the frame*. In particular, it copies dirty bits from the flex container to the flex item.  And if we end up deciding we don't need to reflow the flex item after this point, then this code quoted above is setting the dirty bit without anyone clearing it.

Fortunately, we can get the information we need (childReflowState.ComputedWidth()) earlier on, in the FlexItem constructor -- at that point, we know we *are* going to be reflowing the flex item at least once, so the nsHTMLReflowState that we create there doesn't have the same problem.
(Assignee)

Updated

4 years ago
Summary: nsFlexContainerFrame shouldn't create a reflow state for measuring flex items' cross size, if it doesn't reflow them to do that measurement → nsFlexContainerFrame shouldn't create a reflow state for measuring flex items' cross size, if it doesn't actually need to reflow them to do that measurement
(Assignee)

Updated

4 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

4 years ago
Depends on: 1054054
(Assignee)

Updated

4 years ago
No longer depends on: 1054054
(Assignee)

Comment 1

4 years ago
Created attachment 8552711 [details] [diff] [review]
fix v1

As comments in the patch describe, this will become a bit more complex when we support flex items with vertical writing modes (whose computed heights will influence their desired widths).

But for now, we already assume that the computed width can just be taken off of the reflow state, and for scenarios where that's valid (all scenarios at the moment), we're better off avoiding recreating the reflow state just to pick off that value. (since as described in comment 0, that causes problems if we decide we don't need to reflow, after creating that second reflow state.)
Attachment #8552711 - Flags: review?(mats)
Comment on attachment 8552711 [details] [diff] [review]
fix v1

This looks fine to me, r=mats.

When you revisit this code for vertical writing mode later...
Since there's just one SizeItemInCrossAxis call site, it might be simpler
to move all the stuff above it in the if-block to SizeItemInCrossAxis and
let it decide when to create 'childReflowState' for itself?
just a thought...
Attachment #8552711 - Flags: review?(mats) → review+
(Assignee)

Comment 3

4 years ago
Thanks for the review! I'll land this later when the tree re-opens.

(RE moving the reflow state inside of SizeItemInCrossAxis -- that's a good suggestion, and I may end up doing that. I don't recall exactly why the reflow state lives outside of that method right now.)
https://hg.mozilla.org/mozilla-central/rev/1ee270f30171
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.