1778931 - Refactor in FlowAndPlaceFloat() and ReflowFloat()

Status: RESOLVED FIXED

(Core :: Layout: Floats, task)

Description

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

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.

Comment 1

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

Attachment: Bug 1778931 Part 1 - Create a float's ReflowInput in FlowAndPlaceFloat() and pass it into ReflowFloat(). r?jfkthame,dholbert

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.

Comment 2

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

Attachment: Bug 1778931 Part 2 - Revise two if branches checking float's reflow status. r?jfkthame,dholbert

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

Comment 3

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

Attachment: Bug 1778931 Part 3 - Tweak floatRI's mIsTopOfPage bit in FlowAndPlaceFloat(). r?jfkthame,dholbert

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

Comment 4

[Pulsebot]

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

Comment 5

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/2eb43bec0b90
https://hg.mozilla.org/mozilla-central/rev/3f521b25ed5f
https://hg.mozilla.org/mozilla-central/rev/a568c2b2043b