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.

> 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 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?

"comparing iterators over different lists"

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

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!

(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.

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

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