1050412 - nsSimplePageSequenceFrame.cpp should use a frame enumerator during reflow

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

During nsSimplePageSequence's reflow method, it walks over its child frames like this:

> 204   for (nsIFrame* kidFrame = mFrames.FirstChild(); nullptr != kidFrame; ) {
> [...]
> 249     kidFrame = kidFrame->GetNextSibling();
> }

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?rev=05eb059f70bf#204

I don't think there's any good reason that the "kidFrame = kidFrame->GetNextSibling()" line is outside of the "for" statement. It should move up into the "for" statement.

Better, though, we should just use a nsFrameList::Enumerator, since IIUC that's what we're trying to use for iterating over frame lists.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: Use nsFrameList::Enumerator (plus nsFrameList::GetLength() in one spot)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: assert that nsPageFrame static_cast is valid, and drop useless null-check

This second part fixes up some broken code that I noticed in one of the loops. 

In particular, this patch:
 (1) makes us assert before static_casting
 (2) drops a useless null-check of child-frame. (The loop ensures that the pointer must be non-null.  nsFrameList is a linked-list, which means it's impossible to have a null pointer in the middle of the list.)  I suspect the original author might've mistakenly thought that this null-check was testing the validity of the static_cast.

Comment 3

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c812537814
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac1b309271d

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b6c812537814
https://hg.mozilla.org/mozilla-central/rev/3ac1b309271d