244581 - cleanup nsIFrame::Append/Remove/Insert/ReplaceFrames

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

These methods do not need to take a prescontext or presshell parameter. The
prescontext is always available by calling GetPresContext on the frame, and the
presshell can be retrieved from the prescontext, if needed. These parameters
should be removed. The other parameters look fine.

Append, Insert and Replace should continue to return nsresult since they could
require resource allocation which could fail. Remove might not ever fail; if
Remove never fails, or if callers assume it never fails, then it can return void.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I have a question.  Do we want to try to move to using nsFrameList (instead of
nsIFrame with a singly-linked assumption) for passing around frame lists?  If
so, would this be a good place to start implementing that policy?

Comment 2

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

Do you think we should? I guess that's OK. In that case the frame parameter for
AppendFrames and InsertFrames would become const nsFrameList&.

Comment 3

[Henrik Lynggaard Hansen]

I'll be happy to take this, only a few questions:

- nsIFrame vs. nsFrameList. when to use what ?

- killing prescontext/presshell, is this all methods ? what about init/destroy ?

Comment 4

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

> - nsIFrame vs. nsFrameList. when to use what ?

Use nsFrameList in Append and Insert, because they let you append or insert
multiple frames. Use nsIFrame* in Remove and Replace because they take just one
frame.

> - killing prescontext/presshell, is this all methods ? what about init/destroy ?

Yes, we can remove prescontext from almost all methods, and we should. But not
in this patch :-)

One more thing ... I suspect ReplaceFrame may not be really used. It looks like
all the callers of ReplaceFrame may just be from other implementations of
ReplaceFrame. It would be good if you can study the code to see if we can remove
ReplaceFrame.

Comment 5

[Henrik Lynggaard Hansen]

(In reply to comment #4)
> > - nsIFrame vs. nsFrameList. when to use what ?
> 
> Use nsFrameList in Append and Insert, because they let you append or insert
> multiple frames. Use nsIFrame* in Remove and Replace because they take just one
> frame.

ok
> 
> > - killing prescontext/presshell, is this all methods ? what about init/destroy ?
> 
> Yes, we can remove prescontext from almost all methods, and we should. But not
> in this patch :-)

 :-)
> 
> One more thing ... I suspect ReplaceFrame may not be really used. It looks like
> all the callers of ReplaceFrame may just be from other implementations of
> ReplaceFrame. It would be good if you can study the code to see if we can remove
> ReplaceFrame.

Will take a look, but might need help

queestion: should the params "just" be refactored, or should it be a
DeComtamination job ? 

Comment 6

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

> queestion: should the params "just" be refactored, or should it be a
> DeComtamination job ?

I think I've covered everything in comments in this bug. Are there any other
changes you can think of doing?

Comment 7

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

Henrik seems to be out of action.

Comment 8

[Vidar Haarr (not reading bugmail)]

I'll take a look at this once I'm done with bug 270708.

Comment 9

[Vidar Haarr (not reading bugmail)]

(In reply to comment #4)
> One more thing ... I suspect ReplaceFrame may not be really used. It looks like
> all the callers of ReplaceFrame may just be from other implementations of
> ReplaceFrame. It would be good if you can study the code to see if we can remove
> ReplaceFrame.

Apart from layout/base/nsCSSFrameConstructor.cpp, I can't find anyone using it
without being an implementation.

The same also goes for RemoveFrame, but layout/base/nsBidiPresUtils.cpp also
calls it in addition to nsCSSFrameConstructor.cpp

I guess that's a seperate bug, and I think we should do this bug and bug 270708
first.

Comment 10

[Vidar Haarr (not reading bugmail)]

Erm, terribly sorry about that.

Comment 11

[Vidar Haarr (not reading bugmail)]

Attachment: work in progress

This is a work in progress.

I've been a bit overzealous and silly, and have patched a lot more than
necessary. I hope that's OK, and I'll try never to run out of scope again.

This patch makes aFrameList into an actual nsFrameList& - the problem with
making it const was apparent when I looked in nsFrameList.h's AppendFrames
method:
<http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrameList.h#73>
where it sets the initial child to nsnull. Therefore, I couldn't make them
const.
roc: What is your take on the const ? Should we poke nsFrameList?

I'm not looking for "real reviews" here, but some comments would be nice.
Specifically, I know that this patch breakes some rables (it seems like it
offsets a row or column +1, but I have no idea yet).
I need some guidelines regarding all the simple
  nsFrameList tmp(aFrame);
  blah->AMethod(tmp);
stuff.
I haven't tested mathml or svg, but they build.
And there are lots of visual cleanups in method declarations and so forth that
can be done.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I need some guidelines regarding all the simple
>  nsFrameList tmp(aFrame);
>  blah->AMethod(tmp);

This can probably be

  blah->AMethod(nsFrameList(aFrame));

or maybe even

  blah->AMethod(aFrame);

(the latter will work because of the one-arg constructor nsFrameList has, I
bet).  I prefer the explicit constructor.

As for the const thing, I think having it non-const is ok as long as we clearly
document how the caller's list will be modified (and make sure all callers deal
with said modifications).  I do think that leaving the nulling-out part as-is is
a good idea.

Comment 13

[Vidar Haarr (not reading bugmail)]

Attachment: Work in progress 2

I think I've cleaned up most of the spaces now, and also tested svg. SVG seems
to work fine, although I'm not completely sure - I compared some tests with tor
on IRC, but he had a patch in his tree that might have caused the one thing
that was "off" that I could find:
<http://www.croczilla.com/svg/samples/circles2/circles2.xml>
If you circle around the svg and move your mouse in on the lower right circle,
a transparent box surrounds it.

This patch still breaks reflowing tables on some sites. Startsiden.no's main
table doesn't look good, and firefox.no/stats.html sometimes cause trouble. I
can't seem to find a consistent behaviour - I'd like help to debug this.

I couldn't get rid of the tmp nsFrameLists, because I can't create them as a
reference. According to a book that biesi refered to, it was supposedly
impossible.

Comment 14

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.1

This patch removes the nsFrameList argument change, and some random changes I
had included. Once I removed nsFrameList, the table regression went away. I
think we should file a new bug on that once this has been fixed, and try to
figure out if we should either make nsFrameList not modify its input on
creation, or if we should perhaps create convenience methods in nsIFrame for
taking a nsIFrame* as input and creating the temporary nsFrameLists there, as
suggested by bz.

I have experienced no regressions with this patch yet, but I have been
unsuccessfull in running the automated layout test scripts.

As I've said before, it contains lots of cleanup that I really should have done
in a different bug - mostly in MathML, but also some in SVG and Tables. If you
want me to remove that, I can do that .. It just takes some time ;)

(The patch needs to be a gzip, since it's originally ~560 kb (!))

Comment 15

[Boris Zbarsky [:bzbarsky]]

It'll take me a bit of time to get to this.  In the meantime, please mail me
about the exact problems you ran into with the regression tests and I'll try to
help you get them running.

Comment 16

[Vidar Haarr (not reading bugmail)]

I got them running now (with very good help and guidance from bz), and the only
difference between before or after my patch, was that two files succeded instead
of failed. I have no idea if that's actually good, but I can't imagine it being
bad ..

I didn't run the net-tests, because I unfortunately don't have enough diskspace
:/ I only ran the tests in layout/html/tests/block/rtest.lst. If you'd like me
to run some tests specific for MathML (I have, of course, been looking at the
manual tests in projects/mathml, but nothing automated) or anything else, please
let me know.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Index: base/nsBidiPresUtils.cpp
-  parent->InsertFrames(aPresContext, *presShell, nsLayoutAtoms::nextBidi,
+  parent->InsertFrames(nsLayoutAtoms::nextBidi,
                        aFrame, *aNewFrame);

Please fix this to not have the linebreak unless the linebreak is
needed for 80-column purposes.

Index: base/nsCSSFrameConstructor.cpp
-      rv = containingBlock->InsertFrames(mPresContext, *mPresShell, aChildListName,
+      rv = containingBlock->InsertFrames(aChildListName,
                                          insertionPoint, firstNewFrame);

Similar here -- pull things up to the prev line as needed.

-  if (nsLayoutAtoms::tableFrame == aParentFrame->GetType()) { 
+  if (nsLayoutAtoms::tableFrame == aParentFrame->GetType()) {

Please, no random whitespace changes in this patch -- it's long enough
already.

-        if (outerTable) { 
+        if (outerTable) {
           AppendFrames(aPresContext, shell, state.mFrameManager, aContainer,
-                       outerTable, captionItems.childList); 
+                       outerTable, captionItems.childList);

Again.

@@ -9443,1 +9438,2 @@ nsCSSFrameConstructor::ContentInserted(n
-                                          nsLayoutAtoms::captionList, newFrame);
+                                          nsLayoutAtoms::captionList,
+                                          newFrame);
}
       else {
         state.mFrameManager->InsertFrames(parentFrame,
-                                          nsnull, prevSibling, newFrame);
+                                          nsnull, prevSibling,
+                                          newFrame);

And don't make random changes like this either, unless you're already
changing the code....

@@ -12107,9 +12104,10 @@ nsCSSFrameConstructor::AppendFirstLineFr
-  aState.mFrameManager->AppendFrames(lineFrame, nsnull, firstInlineFrame);
+  aState.mFrameManager->AppendFrames(lineFrame, nsnull,
+                                     firstInlineFrame);

Again.

In the interests of having time to do something else this weekend, I'm going to
stop reading now.  Could you please remove all the unrelated changes from this
patch?  That will make it a lot quicker to review.  Basic guidelines:

1)  Don't change whitespace unless you're changing the code in that line or
    right next to it (eg in a function call, pull up args as I asked above).
2)  Don't change whitespace in function declarations just because you remove
    the longest type in the arguments.  If they're already lined up, just leave
    them as-is.  The extra space or two won't hurt anyone....

Comment 18

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.2

Sorry about all the seemingly random space changes. They were mostly due to
adding nsFrameList and removing it again since it didn't work properly. Some of
them were because I tried cleaning up a bit, and didn't realize how it made the
review job more difficult.

I've tried to keep the space changes to a minimum now.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173470 [details] [diff] [review]
version 0.2

>Index: base/nsCSSFrameConstructor.cpp
>@@ -1332,8 +1331,8 @@ nsFrameConstructorState::ProcessFrameIns

>-      rv = containingBlock->InsertFrames(mPresContext, *mPresShell, aChildListName,
>+
>+      rv = containingBlock->InsertFrames(aChildListName,
>                                          insertionPoint, firstNewFrame);

Like I said in comment 17, please pull the args up to the previous line as
needed.

>@@ -9467,7 +9463,8 @@ nsCSSFrameConstructor::ContentInserted(n
>-                                          nsLayoutAtoms::captionList, newFrame);
>+                                          nsLayoutAtoms::captionList,
>+                                          newFrame);

And _this_ exact change I told you not to make, in comment 17.

>Index: forms/nsFieldSetFrame.cpp
>-  virtual PRBool CanPaintBackground();

Why did you move where this method is declared?  Put it back where it was?

>@@ -653,31 +635,27 @@ nsFieldSetFrame::RemoveFrame(nsPresConte
>+    mFrames.DestroyFrame(GetPresContext(), mLegendFrame);
>+      GetParent()->ReflowDirtyChild(GetPresContext()->GetPresShell(), this);

Want to cache the return value of GetPresContext() here, since you'll need it
twice?

>Index: generic/nsAbsoluteContainingBlock.cpp
>+nsAbsoluteContainingBlock::AppendFrames(nsIFrame*      aDelegatingFrame,
>                                         nsPresContext* aPresContext,

Why leave aPresContext here?  Just use aDelegatingFrame->GetPresContext().

>+nsAbsoluteContainingBlock::InsertFrames(nsIFrame*      aDelegatingFrame,
>                                         nsPresContext* 
aPresContext,

Same.

> nsAbsoluteContainingBlock::RemoveFrame(nsIFrame*       
>+                                       nsPresContext*  aPresContext,

Same.

>+nsAbsoluteContainingBlock::ReplaceFrame(nsIFrame*      aDelegatingFrame,
>                                         nsPresContext* 

Same (note that it's even unused in this method).

Don't forget to fix callers, of course (blockframe, positioned inline,
viewport, and page).

>Index: generic/nsBlockFrame.cpp
>+nsBlockFrame::DoRemoveFrame(nsIFrame* aDeletedFrame)

>+  nsIPresShell *presShell = GetPresContext()->PresShell();

Again, it looks like you use the prescontext later in this method, so it may be
worth keeping a pointer to it locally.

>Index: mathml/base/src/nsMathMLContainerFrame.cpp

>@@ -337,14 +333,14 @@ nsMathMLContainerFrame::Stretch(nsPresCo
>-        mathMLFrame->Stretch(aPresContext, aRenderingContext,
>+        mathMLFrame->Stretch(aRenderingContext,
>                              mEmbellishData.direction, containerSize, childSize);

This should maybe pull up mEmbellishData.direction to the prev line?

I didn't read most of the matml changes.  Please put those in a separate bug,
and ask rbs to review....

Note to self: need to review the actual changes for this bug in MathML, once I
can find them...

>Index: svg/base/src/nsSVGForeignObjectFrame.cpp
>+nsSVGForeignObjectFrame::InsertFrames(nsIAtom*        aListName,

>+	rv = nsSVGForeignObjectFrameBase::InsertFrames(aListName, aPrevFrame,
>+                                                   aFrameList);

Weird indent.

>+nsSVGForeignObjectFrame::ReplaceFrame(nsIAtom*        aListName,
>+	rv = nsSVGForeignObjectFrameBase::ReplaceFrame(aListName, aOldFrame,
>+                                                   aNewFrame);

Again,

>Index: tables/nsTableCellFrame.cpp

>+void nsTableCellFrame::VerticallyAlignChild(const 

You use GetPresContext() in a few places in this method.  Cache the pointer,
please.

>Index: tables/nsTableColGroupFrame.cpp
>+NS_METHOD nsTableColGroupFrame::IR_TargetIsChild(nsHTMLReflowMetrics&     aDesiredSize,

Same in this method.

>Index: tables/nsTableFrame.cpp

>+nsTableFrame::CreateAnonymousColGroupFrame(nsTableColGroupType aColGroupType)

And here.

>@@ -886,15 +877,14 @@ nsTableFrame::CreateAnonymousColFrames(n
>-  CreateAnonymousColFrames(aPresContext, colGroupFrame, aNumColsToAdd, 
>+  CreateAnonymousColFrames(colGroupFrame, aNumColsToAdd, 
>                            aColType, PR_TRUE, prevCol, &firstNewFrame);

Pull up aColType a line?

>+nsTableFrame::CreateAnonymousColFrames(nsTableColGroupFrame* 

Multiple GetPresContext() uses.

>+nsTableFrame::GetBCMargin() const

And here.

>+nsTableFrame::InitChildReflowState(nsHTMLReflowState& 

And here.

>+nsTableFrame::IR_TargetIsChild(nsTableReflowState&  aReflowState,

And here.

>@@ -3295,8 +3253,8 @@ nsTableFrame::ReflowChildren(nsPresConte
>+            GetPresContext()->PresShell()->FrameConstructor()->
>+              CreateContinuingFrame(GetPresContext(), kidFrame, this,

And this.  Applies to this whole method, though.

>Index: xul/base/src/nsBoxFrame.cpp
>+nsBoxFrame::RemoveFrame(nsIAtom*        aListName,

And here.

>Index: xul/base/src/nsMenuFrame.cpp
>+nsMenuFrame::RemoveFrame(nsIAtom*        aListName,

And here.

>+nsMenuFrame::InsertFrames(nsIAtom*        aListName,
>-  if (aFrameList && NS_SUCCEEDED(CallQueryInterface(aFrameList, &menuPar))) {
>+  if (NS_SUCCEEDED(CallQueryInterface(aFrameList, &menuPar))) {

Why the logic change?  Don't do that in this patch, please.

>@@ -1863,17 +1857,17 @@ nsMenuFrame::AppendFrames(nsPresContext*
>-  if (aFrameList && NS_SUCCEEDED(CallQueryInterface(aFrameList, &menuPar))) {
>+  if (NS_SUCCEEDED(CallQueryInterface(aFrameList, &menuPar))) {

Again.

Comment 20

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.3

Fixes bz's comments in comment 19, and leaves out all the mathml changes.
I've filed bug 281317 on that and attached the changes there.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173579 [details] [diff] [review]
version 0.3

r+sr=bzbarsky

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in

Comment 23

[Vidar Haarr (not reading bugmail)]

Thanks!
Marking FIXED.

Bug 281387 filed for const nsFrameList&.

Comment 24

[Boris Zbarsky [:bzbarsky]]

So... any reason this could have regressed Tp by 1% or so?