cleanup nsIFrame::Append/Remove/Insert/ReplaceFrames

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
14 years ago
4 years ago

People

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

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 4 obsolete attachments)

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.
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?
Do you think we should? I guess that's OK. In that case the frame parameter for
AppendFrames and InsertFrames would become const nsFrameList&.
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 ?

Status: NEW → ASSIGNED
> - 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.
(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 ? 
> 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?
Henrik seems to be out of action.
Assignee: henrik → nobody
Status: ASSIGNED → NEW
Assignee: nobody → nobody
(Assignee)

Comment 8

13 years ago
I'll take a look at this once I'm done with bug 270708.
Whiteboard: [good first bug]
(Assignee)

Comment 9

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

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 10

13 years ago
Erm, terribly sorry about that.
Assignee: nobody → bugmail
Status: ASSIGNED → NEW
(Assignee)

Comment 11

13 years ago
Created attachment 170059 [details] [diff] [review]
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.
> 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.
(Assignee)

Comment 13

13 years ago
Created attachment 170387 [details]
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.
(Assignee)

Updated

13 years ago
Attachment #170059 - Attachment is obsolete: true
(Assignee)

Comment 14

13 years ago
Created attachment 172758 [details]
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 (!))
Attachment #170387 - Attachment is obsolete: true
Attachment #172758 - Flags: review?(bzbarsky)
(Assignee)

Updated

13 years ago
Attachment #172758 - Attachment is patch: false
Attachment #172758 - Attachment mime type: text/plain → application/octet-stream

Updated

13 years ago
Attachment #172758 - Attachment mime type: application/octet-stream → application/x-gzip
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.
(Assignee)

Comment 16

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

Comment 18

13 years ago
Created attachment 173470 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #172758 - Attachment is obsolete: true
Attachment #173470 - Flags: review?(bzbarsky)
(Assignee)

Updated

13 years ago
Attachment #172758 - Flags: review?(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.
Attachment #173470 - Flags: review?(bzbarsky) → review-
(Assignee)

Updated

13 years ago
Blocks: 281317
(Assignee)

Comment 20

13 years ago
Created attachment 173579 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #173470 - Attachment is obsolete: true
Attachment #173579 - Flags: review?(bzbarsky)
Comment on attachment 173579 [details] [diff] [review]
version 0.3

r+sr=bzbarsky
Attachment #173579 - Flags: superreview+
Attachment #173579 - Flags: review?(bzbarsky)
Attachment #173579 - Flags: review+
(Assignee)

Comment 23

13 years ago
Thanks!
Marking FIXED.

Bug 281387 filed for const nsFrameList&.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
So... any reason this could have regressed Tp by 1% or so?
You need to log in before you can comment on or make changes to this bug.