Closed Bug 1778931 Opened 7 months ago Closed 6 months ago

Refactor in FlowAndPlaceFloat() and ReflowFloat()

Categories

(Core :: Layout: Floats, task)

task

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(3 files)

I found some opportunities to simplify the code for reflowing floats. It is also a preparation for bug 1464761. Please see the upcoming patches for the details.

Summary: Refactor around FlowAndPlaceFloat() and ReflowFloat() → Refactor in FlowAndPlaceFloat() and ReflowFloat()

There is some advantage to creating the float's ReflowInput in
FlowAndPlaceFloat().

  1. ReflowFloat() doesn't need to output the float's margin and offset via the
    output arguments. FlowAndPlaceFloat() can get them from the local ReflowInput
    directly.

  2. Since we are going to reflow the float, we have to create a ReflowInput
    anyway. FloatMarginISize() can take the ReflowInput and be simplified. No need
    to waste time to create a separate SizeComputationInput.

Also, delete the comment "Pass floatRS so the frame hierarchy can be
used (redoFloatRS has the same hierarchy)" because I believe it is obsolete.

This patch also lays the foundation for other improvements in the later patches.

Assignee: nobody → aethanyc
Status: NEW → ASSIGNED

The if-branch is incorrect because ShouldAvoidBreakInside() [1] is called via
the parent block instance with float child's ReflowInput as the argument. In
principle, the child frame should report break-before status if it cannot fit
the page/column and wants to avoid breaking. Removing this if-branch is
potentially a behavior change, but it is the correct thing to do. (Note that
ReflowFloat()'s caller FlowAndPlaceFloat() already handles
IsInlineBreakBefore(). If the float frame type already implements break-avoid,
we are fine.)

The else-if branch can never be reached except when the float is a letter frame,
and we've tested that scenario a few lines just before the this added assertion.
The code coverage also shows this branch in never reached.
https://coverage.moz.tools/#revision=e6e2286d2ac25001127a1cf54a87a95fb435c734&path=layout%2Fgeneric%2FnsBlockFrame.cpp&view=file&line=6682

[1] Note: ShouldAvoidBreakInside() is a protected member function, which can
only be called for concrete frame classes inheriting from nsContainerFrame.

Depends on D151455

The old code tweaks mIsTopOfPage in ReflowFloat(). However
aAvailableSize.ISize() (in containing block's writing-mode) always equals to
ContentISize() because it is computed via ComputeAvailableSizeForFloat().
Therefore, the only reason the floatRI's mIsTopOfPage can be false is when
some floats are already being placed, and we only check this in the
!earlyFloatReflow branch.

By tweaking mIsTopOfPage bit in FlowAndPlaceFloat(), we can also remove two
unused parameters in ReflowFloat().

This patch shouldn't change the behavior.

Depends on D151456

Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2eb43bec0b90
Part 1 - Create a float's ReflowInput in FlowAndPlaceFloat() and pass it into ReflowFloat(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3f521b25ed5f
Part 2 - Revise two if branches checking float's reflow status. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a568c2b2043b
Part 3 - Tweak floatRI's mIsTopOfPage bit in FlowAndPlaceFloat(). r=dholbert
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
You need to log in before you can comment on or make changes to this bug.