nsIFrame::Append/InsertFrames should take nsFrameList&

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Vidar Haarr (not reading bugmail), Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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..) ?
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.

Blocks: 233463
Depends on: 504221
Depends on: 504972
Blocks: 505231
Created attachment 389468 [details] [diff] [review]
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #389468 - Flags: superreview?(dbaron)
Attachment #389468 - Flags: review?(roc)
Attachment #389468 - Flags: review?(bernd_mozilla)

Comment 3

8 years ago
The size of the patch doesn't matter (I can always retaliate ;-) 43178, 28800, 452319), I am happy to review the table part.
Attachment #389468 - Flags: review?(roc) → review+
Comment on attachment 389468 [details] [diff] [review]
Proposed fix

+dump('aaa');

Remove

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

InsertFrames
Created attachment 389618 [details] [diff] [review]
With roc's comments addressed, and updated to the changes in bug 504221
Attachment #389618 - Flags: superreview?(dbaron)
Attachment #389618 - Flags: review?(bernd_mozilla)
Created attachment 390610 [details] [diff] [review]
Merged to latest round of changes in bug 504221
Attachment #389468 - Attachment is obsolete: true
Attachment #389618 - Attachment is obsolete: true
Attachment #390610 - Flags: superreview?(dbaron)
Attachment #390610 - Flags: review?(bernd_mozilla)
Attachment #389468 - Flags: superreview?(dbaron)
Attachment #389468 - Flags: review?(bernd_mozilla)
Attachment #389618 - Flags: superreview?(dbaron)
Attachment #389618 - Flags: review?(bernd_mozilla)

Comment 7

8 years ago
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?
Attachment #390610 - Flags: review?(bernd_mozilla) → review+
> 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.
Attachment #390610 - Flags: superreview?(dbaron)
Attachment #390610 - Flags: superreview+
Comment on attachment 390610 [details] [diff] [review]
Merged to latest round of changes in bug 504221

sr=dbaron
Pushed http://hg.mozilla.org/mozilla-central/rev/0a0b0c3f614b with the above comments addressed.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Summary: nsIFrame::Append/InsertFrames should take const nsFrameList& → nsIFrame::Append/InsertFrames should take nsFrameList&
You need to log in before you can comment on or make changes to this bug.