257216 - More columns-related block fixes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll attach a patch, extracted from my columns work, that does a few small things:

-- Adds some needed #ifdef DEBUG code so that noisy block reflow doesn't leak
indent counts along certain control flow paths
-- Factors out the code that gets a float from its placeholder (and detects
whether the frame is in fact a float placeholder) into an nsLayoutUtils method.
It's still lame but now the lameness is confined. Use this method from a couple
of places, Collect(Overflow)Floats and nsLineLayout::ReflowFrame.
-- Fixes a bug: when a float is reflowed in a line, and the line ends up being
pushed to the overflow lines, we did not remove the float from the space
manager. This could end up giving the block an unnecessarily large height when
ComputeFinalSize checks the space manager's vertical advance. The fix removes
all overflowing floats from the space manager in BuildFloatList which is before
ComputeFinalSize. This will be even more important when we start extracting the
floats region from the space manager after reflow and apply it to impact other
columns.
-- Renamed CollectOverflowFloats to CollectFloats because I'm going to use it
for floats in normal lines in a future patch.
-- Added an important assertion to nsBlockReflowState. This might fire during
printing certain pages until I land some more changes I have, which fix float
parenting bugs when lines are pushed and pulled.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

As described

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 157238 [details] [diff] [review]
fix

the contents of this patch are described in the bug.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm, there's a bunch of stuff in the space manager that assumes that regions
aren't removed.  I think you need to replace mXMost with something that actually
computes it.

With that change, rubber-stamp r+sr=dbaron on the whole patch.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 157238 [details] [diff] [review]
fix

(Really, mLowestTop needs fixing too, although it may not be an issue.)  And
I've read the whole patch now -- it wasn't that big, so that is really the only
thing I have a problem with.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: OK

OK. I decided to partially fix the space manager so that at least XMost and
YMost work after RemoveRegions, which I renamed to RemoveTrailingRegions.
That's all we need, I think. I added comments making clear that you can't do
much after calling RemoveTrailingRegions, it's only intended to be called near
the end of the life of the space manager.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 158702 [details] [diff] [review]
OK

okay, this should do it

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in