281387 - nsIFrame::Append/InsertFrames should take nsFrameList&

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Vidar Haarr (not reading bugmail)]

Derived from bug 244581, comments 1, 2, 3, 4, 11, 12, 13 and 14.

The basic problem is that, in
<http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrameList.h#73>, it
sets the initial childlist to nsnull, so we can't make it const.

As evidenced by attachment 170387 [details], using nsFrameList& breaks tables, so I
decided to drop this from that bug, and rather file a new one for this change.

Should we poke nsFrameList to be const'able, or should we perhaps just use
nsFrameList&, and investigate why it cause the table regression (which might
just be because I converted some of the nsIFrame -> nsFrameList loops
incorrectly, I don't really know..) ?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Another option is to just take an |nsFrameList| (not const, and not reference).

Frankly, I think the right approach to this bug is the following:

1)  Add Append/InsertFrames versions that take an nsFrameList&.
2)  Convert callers one at a time over to these methods; the conversions should
    be "in depth" as much as possible (that is, replace all uses of nsIFrame* for
    frame lists on the callstack with nsFrameList).
3)  When we can, eliminate the nsIFrame* versions of Append/InsertFrames.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Some notes on this patch:

1)  I apologize for the size of the table changes.  If desired, I can try to factor them out into a separate patch.  Bernd, I'd like you to look over those changes.  This applies on top of the patch queue in bug 504221 and the patch in bug 504972, if you want to test it locally.

2)  The nsFrameList.h change to IsDone() is needed because anonymous table column groups can get destroyed when non-anonymous ones are inserted.  This changes the mNextSibling of the last non-anonymous colgroup inserted to null while iterating over the newly-inserted non-anonymous colgroups.  Bernd, can you double-check me on this?  Especially on making sure that it can't become something _other_ than null?

The rest of this should be pretty straightforward.  I'd like David to look over the nsIFrame change.

Comment 3

[Bernd]

The size of the patch doesn't matter (I can always retaliate ;-) 43178, 28800, 452319), I am happy to review the table part.

Comment 4

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

Comment on attachment 389468 [details] [diff] [review]
Proposed fix

+dump('aaa');

Remove

+  // for InsertFrmes we do it in the other order?

InsertFrames

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: With roc's comments addressed, and updated to the changes in bug 504221

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to latest round of changes in bug 504221

Comment 7

[Bernd]

Comment on attachment 390610 [details] [diff] [review]
Merged to latest round of changes in bug 504221

+    PRBool IsDone() const {
+      // Can't just check mEnd, because some table code goes and destroys the
+      // tail of the frame list (including mEnd!) while iterating over the
+      // frame list.
+      return !mFrame || mFrame == mEnd;
+    }

Can you please be more specific and file bugs if the behavior is wrong, rather than "anonymous accusations".

     // Nothing special to do, just add the frame to our child list
+      NS_NOTREACHED("How did we get here?");
       mFrames.AppendFrame(nsnull, f);
       
Could you please state that this a frame construction issue so that the right people get alerted, this occurs at two places.

+  // XXXbz there shouldn't be any other ones here... can we just put
+  // them all in the array and not do all this QI nonsense?

Please NS_ASSERT on this and the other QI places.. If these are not bogus, Jesse will tell within days.  If it stays silent we can easily remove them together with the ASSERT.

<rant review="unrelated">
I think those xxx comments without bugs should be banned. If it is worth a XXX it should be filed as a bug. The source code should not become bugzilla2.0. More on, usually those comments are very actionable items. So loosing them in the source is just a waste of engineering resources.
</rant>

+  // XXXbz why do we append here first, then append to table, while
+  // for InsertFrames we do it in the other order?
I am not sure whats going on, philabug?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Can you please be more specific and file bugs if the behavior is wrong, rather
> than "anonymous accusations".

It's not clear to me that it's a bug.  What happens is that when colgroups are added we insert them into the framelist, then start iterating them and updating the cellmap and the first thing we do is destroy the anonymous colgroup we used to have.  This does mean that the nextsibling of the last-inserted colgroup becomes null instead of the anonymous colgroup.

If you think this is undesirable, I can certainly file a bug on changing the code ordering here.

> Could you please state that this a frame construction issue so that the right
> people get alerted, this occurs at two places.

Will do.

> Please NS_ASSERT on this and the other QI places..

Will do.

> I think those xxx comments without bugs should be banned

Fair enough.  ;)

> I am not sure whats going on, philabug?

Will do.

Comment 9

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

Comment on attachment 390610 [details] [diff] [review]
Merged to latest round of changes in bug 504221

sr=dbaron

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/0a0b0c3f614b with the above comments addressed.