838688 - Improve nsBlockFrame::ReparentFloats/CollectFloats

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Make ReparentFloats/CollectFloats not require a priori knowledge of which child list a float is expected to be on.

Removing a nFrameList::RemoveFrameIfPresent and a ContainsFrame in the
process (both O(n)).

Comment 2

[Mats Palmgren (inactive)]

Attachment: Optimize CollectFloats by doing an early return if this block doesn't have any floats.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 710784 [details] [diff] [review]
Make ReparentFloats/CollectFloats not require a priori knowledge of which child list a float is expected to be on.

>+        // Floats lives in mFloats, or in the PushedFloat or OverflowOutOfFlows

s/lives/live/

Again would be good to have asserts that the frame is in one of the lists in question....  r=me with that

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 710794 [details] [diff] [review]
Optimize CollectFloats by doing an early return if this block doesn't have any floats.

Why is returning early if the frame has no floats OK if aCollectSiblings is true?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 710794 [details] [diff] [review]
Optimize CollectFloats by doing an early return if this block doesn't have any floats.

r- pending explanation

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 710794 [details] [diff] [review]
Optimize CollectFloats by doing an early return if this block doesn't have any floats.

(In reply to Boris Zbarsky (:bz) from comment #4)
> Why is returning early if the frame has no floats OK if aCollectSiblings is
> true?

Because nsBlockFrame::CollectFloats ignores floats that belongs to some
other frame (can happen due to lazy reparenting in nsInlineFrame).
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#6877
So if 'this' has no floats, CollectFloats is a NOP.
(with the assumption that floats are correctly parented and present on
one of the lists of its parent -- which I'm pretty sure holds)

Comment 7

[Mats Palmgren (inactive)]

... and the above is independent of aCollectSiblings, which just says if
we should also traverse aFrame's siblings or not.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 710794 [details] [diff] [review]
Optimize CollectFloats by doing an early return if this block doesn't have any floats.

Oh, I see.  aFrame is not the block itself, gotcha.

r=me

Comment 9

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> s/lives/live/

fixed

> Again would be good to have asserts that the frame is in one of the lists in
> question....  r=me with that

Added an assertion for that:

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ac17d659f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/504e5b0d7d53

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/73ac17d659f6
https://hg.mozilla.org/mozilla-central/rev/504e5b0d7d53