514634 - some misc block frame code cleanup

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Noticed a few things while working on another bug that I wanted to followup on.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Add some assertions to catch problems.

nsBlockFrame::PullFrameFrom calls SetNextSibling twice, doing what should be
the same thing twice.

Fix the comment above nsBlockFrame::PullFrameFrom.

Comment 2

[fantasai]

-      aLine->LastChild()->SetNextSibling(frame);
+      NS_ASSERTION(aState.mPrevChild == aLine->LastChild(),
+        "mPrevChild should be the LastChild of the line we are adding to");
+      // The frame is being pulled from a next-in-flow; therefore we
+      // need to add it to our sibling list.
+      if (nsnull != aState.mPrevChild) {
+        aState.mPrevChild->SetNextSibling(frame);
+      }

So what happens when aState.mPrevChild is null? (If that's not supposed to happen, then we should assert here and not have an if-clause.)

       frame->SetNextSibling(nsnull);

This line should also be moved so that the two list-splicing operations are together. You'll need to cache the nextSibling in a newFirstChild variable or something, but I think the code will be more understandable that way.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

(In reply to comment #2)
> So what happens when aState.mPrevChild is null? (If that's not supposed to
> happen, then we should assert here and not have an if-clause.)

The NS_ASSERTION(aState.mPrevChild == aLine->LastChild(),...) already asserts for the null mPrevChild case. Removed the if-clause.

>        frame->SetNextSibling(nsnull);
> 
> This line should also be moved so that the two list-splicing operations are
> together. You'll need to cache the nextSibling in a newFirstChild variable or
> something, but I think the code will be more understandable that way.

Done.

Comment 4

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

This is continuation from bug 513394 comment 7, bug 513394 comment 12, etc.

Comment 5

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

http://hg.mozilla.org/mozilla-central/rev/f712fa4cd5ad