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

Status: RESOLVED FIXED

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

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1562122 Part 1 - Move the logic after ComputeFinalBSize() into ComputeFinalBSize().

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.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1562122 Part 2 - Replace effectiveComputedBSize with computedBSizeLeftOver.

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

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1562122 Part 3 - Delete the duplicate logic computing the final block size.

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

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1562122 Part 4 - Return early in the overflow incomplete case, and add an assertion for the bound of final block size.

Depends on D36290

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a82f8b85057e
https://hg.mozilla.org/mozilla-central/rev/818f4038280d
https://hg.mozilla.org/mozilla-central/rev/a19c5b59a5be
https://hg.mozilla.org/mozilla-central/rev/321be77f2c08