728908 - Make the Overflow lines property have a nsFrameList

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Mats Palmgren (inactive)]

There's a number of nice things that falls out from this --
removing *all* line->LastChild() calls and some SetNextSibling() calls,
and to support GetChildList() returning const nsFrameList&.

The bad thing is that it uses memory for an extra nsFrameList per block,
if the block has overflow lines.  I think it's rare that it has, and it's
transient - when the block is fully reflowed the property will have been
deleted.

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1, overflow lines

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2, make nsLineBox::LastChild() DEBUG-only

Comment 3

[Mats Palmgren (inactive)]

> removing some SetNextSibling() calls

Well, all SetNextSibling() calls in nsBlockFrame.cpp, since they are illegal here now,
but there's still a few SetNextSibling() elsewhere in the code is what I meant...

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 598899 [details] [diff] [review]
part 1, overflow lines

> nsBlockFrame::PushLines(nsBlockReflowState&  aState,
>                         nsLineList::iterator aLineBefore)
> {
>+  // NOTE: aLineBefore is always a normal line, not an overflow line.
>+  // The following expression will assert otherwise.
>+  DebugOnly<bool> check = aLineBefore == mLines.begin();
>+

What's asserting here?

Comment 5

[Mats Palmgren (inactive)]

"comparing iterators over different lists"

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 598900 [details] [diff] [review]
part 2, make nsLineBox::LastChild() DEBUG-only

Nice.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 598899 [details] [diff] [review]
part 1, overflow lines

>+++ b/layout/generic/nsBlockFrame.cpp
>+  FrameLines* overflowLines = RemoveOverflowLines();
>   if (overflowLines) {
>-    nsLineBox::DeleteLineList(presContext, *overflowLines, aDestructRoot);
>+    nsLineBox::DeleteLineList(presContext, overflowLines->mLines,
>+                              aDestructRoot);
>     delete overflowLines;

Should there be an assert that overflowLines->mFrames is empty?

>+RemoveFirstLine(nsLineList* aFromLines, nsFrameList* aFromFrames,

Make the in parameters non-const references, please?

And document what the return value means (specifically, whether aFromLines is empty after this call).

> nsBlockFrame::DrainOverflowLines()
>+      for (nsFrameList::Enumerator e(overflowLines->mFrames);
>+           !e.AtEnd(); e.Next()) {
>+        ReparentFrame(e.get(), prevBlock, this);

This can use your new ReparentFrames method.

>+        for (nsFrameList::Enumerator e(oofs.mList); !e.AtEnd(); e.Next()) {
>+          ReparentFrame(e.get(), prevBlock, this);

This too.

r=me.  Looks nice!

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 1, overflow lines

(In reply to Boris Zbarsky (:bz) from comment #7)
> >-    nsLineBox::DeleteLineList(presContext, *overflowLines, aDestructRoot);
> >+    nsLineBox::DeleteLineList(presContext, overflowLines->mLines,
> >+                              aDestructRoot);
> >     delete overflowLines;
> 
> Should there be an assert that overflowLines->mFrames is empty?

overflowLines->mFrames is invalid at this point because DeleteLineList
use SetNextSibling.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsLineBox.cpp#341

I'll file a follow-up bug on fixing that (passing the frame list too probably)

Fixed your other comments as suggested.

Comment 9

[Mats Palmgren (inactive)]

Filed bug 733991 to followup on improving nsLineBox::DeleteLineList.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d68d568d3538
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4095ea6949b

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a4095ea6949b
https://hg.mozilla.org/mozilla-central/rev/d68d568d3538