386142 - fantasai's li'l Need More Comments bug

Status: RESOLVED INACTIVE

(Core :: Layout, defect)

Description

[fantasai]

Splitting out some comment fixes from bug 379349.

Comment 1

[fantasai]

Attachment: patch A

Comment 2

[fantasai]

Well, there's a couple things that aren't quite comment/whitespace fixes. One removal of an unused variable, one removal of a duplicated 3 lines of code....

I forgot the context on that, here it is:

  // If we have a next-in-flow, then we're not complete
  // XXXldb This used to be done only for the incremental reflow codepath.
  if (GetNextInFlow()) {
    aStatus = NS_FRAME_NOT_COMPLETE;
  }

  SetHasStyleHeight((NS_UNCONSTRAINEDSIZE != aReflowState.mComputedHeight) &&
                    (aReflowState.mComputedHeight > 0)); 
  
  // just set our width to what was available. The table will calculate the width and not use our value.
  aDesiredSize.width = aReflowState.availableWidth;

  // if we have a nextinflow we are not complete
  if (GetNextInFlow()) {
    aStatus |= NS_FRAME_NOT_COMPLETE;
  }
  aDesiredSize.mOverflowArea.UnionRect(aDesiredSize.mOverflowArea, nsRect(0, 0, aDesiredSize.width,

Comment 3

[fantasai]

Attachment: patch A (with context)

Comment 4

[Jesse Ruderman]

> -  NS_ASSERTION(mFrameCount == 0, "Some frame destructors were not called");
> +  NS_ASSERTION(mFrameCount == 0, "Some frame or linebox destructors were not called");

While you're trying to make my assertion message from bug 334514 more accurate, you might be interested in bug 365909 comment 5 and bug 365909 comment 15.

Comment 5

[fantasai]

In that case, I suggest
"Some frames or other AllocateFrame'd objects not destroyed"

Comment 6

[Eli Friedman]

Checked in.

Comment 7

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

Comment on attachment 270122 [details] [diff] [review]
patch A (with context)

>Index: layout/base/nsPresShell.cpp
>-  NS_ASSERTION(mFrameCount == 0, "Some frame destructors were not called");
>+  NS_ASSERTION(mFrameCount == 0, "Some frame or linebox destructors were not called");

A bunch of other things are allocated in the frame arena.  It would be clearer not to mention line boxes specifically, but make it more general.

Comment 8

[Eli Friedman]

(In reply to comment #7)
> (From update of attachment 270122 [details] [diff] [review])
> >Index: layout/base/nsPresShell.cpp
> >-  NS_ASSERTION(mFrameCount == 0, "Some frame destructors were not called");
> >+  NS_ASSERTION(mFrameCount == 0, "Some frame or linebox destructors were not called");
> 
> A bunch of other things are allocated in the frame arena.  It would be clearer
> not to mention line boxes specifically, but make it more general.

I generalized it to "Some objects allocated with AllocateFrame were not freed" when I checked it in.

Comment 9

[fantasai]

Attachment: patch to fix comments for nsIFrame::Init

Couldn't find anything about an AppendToFlow function, but I did check that nsSplittableFrame::Init sets up both sides of the two-way link.

Comment 10

[Jeff Walden [:Waldo]]

nsIFrame::Init patch committed.

Comment 11

[fantasai]

Attachment: Strip carriage returns in nsVideoFrame.cpp

Comment 12

[fantasai]

carriage returns patch pushed to mozilla-central
  http://hg.mozilla.org/mozilla-central/rev/90d2c483697b

Comment 13

[Wayne Mery (:wsmwk)]

It looks like all the patches landed, or at least two of the three.
Surely anything that hasn't landed is well obsolete?

Comment 14

[Wayne Mery (:wsmwk)]

Sean, see comment 13

Comment 15

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

comment 6, comment 10, and comment 12 makes it look like all three