Closed Bug 1562122 Opened 5 months ago Closed 5 months ago

Refactor and cleanup nsBlockFrame::ComputeFinalSize() when the block size is constrained

Categories

(Core :: Layout: Block and Inline, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(4 files)

I notice when the block size is constrained, the computation of final block size of a block frame is hard to understand. The logic involve nsBlockFrame::ComputeFinalBSize() and the if-hunk after it, and both place contains some redundant logic.

I want to move the if-hunk into nsBlockFrame::ComputeFinalBSize, and see if I can further simplify the computation. My goal is to make no behavior changes.

This patch only moves the logic, and rename some variables. More
clean-up follows.

Note in the middle of ComputeFinalBSize(), ShouldAvoidBreakInside() can
do early return under the condition that aStatus is complete. The logic
moved in this patch is executed only when aStatus is incomplete, so no
behavior is changed after applying this change.

No need to call GetEffectiveComputedBSize() twice.

Also, calling aState.ConsumedBSize() instead of using
aState.mConsumedBSize() directly because the accessor function cache
mConsumedBSize properly if it is called the first time.

Depends on D36288

After the deleted logic

aFinalSize.BSize(wm) =
    std::max(aReflowInput.AvailableBSize(), aContentBSize);

aStatus changes to incomplete, so it computes the same thing again.

Depends on D36289

One thing I'm not so sure how to refactor is the ComputeFinalBSize()'s third argument aContentBSize.

The value aState.mBCoord + nonCarriedOutBDirMargin passing to ComputeFinalBSize() is somewhat like a raw version of blockEndEdgeOfChildren a few lines above, which includes the block start of border and padding, because aState.mBCoord is initialized as mBorderPadding.BStart(wm). Calling it aContentBSize is definitely wrong.

We should maybe do either 1) rename aContentBSize to aBlockEndEdgeOfChild, and figure out whether we want to use the current aState.mBCoord + nonCarriedOutBDirMargin value, or use the blockEndEdgeOfChildren computed a few lines above; or 2) subtract mBorderPadding.BStart(wm) from it, and add back mBorderPadding.BStart(wm) in ComputeFinalBSize(). Again, these all depend on which is one is the correct behavior, and I'm still struggling to figure out.

Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a82f8b85057e
Part 1 - Move the logic after ComputeFinalBSize() into ComputeFinalBSize(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/818f4038280d
Part 2 - Replace effectiveComputedBSize with computedBSizeLeftOver. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a19c5b59a5be
Part 3 - Delete the duplicate logic computing the final block size. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/321be77f2c08
Part 4 - Return early in the overflow incomplete case, and add an assertion for the bound of final block size. r=dholbert
You need to log in before you can comment on or make changes to this bug.