Refactor and cleanup nsBlockFrame::ComputeFinalSize() when the block size is constrained
Categories
(Core :: Layout: Block and Inline, task, P3)
Tracking
()
| 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.
|
Assignee | |
Comment 1•5 years ago
|
||
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.
|
|
Assignee | |
Comment 2•5 years ago
|
||
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
|
|
Assignee | |
Comment 3•5 years ago
|
||
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
|
|
Assignee | |
Comment 4•5 years ago
|
||
Depends on D36290
|
|
Assignee | |
Comment 5•5 years ago
|
||
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
|
||
Comment 7•5 years ago
|
||
| bugherder | ||
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
Description
•