258513 - unify box and frame trees

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

One thing that contributes to overhead in the XUL implementation is that we're
constantly going back and forth between the box and frame trees.  It would help
the situation if we unified the two trees as follows:

- All nsIBox methods move onto nsIFrame
- Non-box frames implement nsIBox in the style of nsBoxToBlockAdaptor (if they
are the child of a non-box frame), so that box layout is translated to reflows
on the child.  If a non-box frame has a non-box parent, we assume that the box
layout methods will not be called.
- Box frames continue to implement nsIBox as they do now

What do we get out of this?

- No more calls to GetFrame() or QI to nsIBox
- Some box methods can become inlined on nsIFrame or simply go away since they
duplicate nsIFrame methods.  Separate parent/next sibling points for boxes can
go away too, since they're identical to the frame pointers.

And the disadvantages...

- BoxToBlockAdaptor cached metrics have to go in a frame property to not bloat
frames, hopefully this isn't a performance hit.
- Increased vtable size for frame implementations

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

implement the above idea.  patch saves about 14 KB of code size on linux.

Comment 2

[Brian Ryner (not reading)]

Attachment: patch with 8 lines of context requested by roc

Comment 3

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

do you want me to review this?

Comment 4

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

 #define NS_IFRAME_IID \
  { 0xa6cf9050, 0x15b3, 0x11d2,{0x93, 0x2e, 0x00, 0x80, 0x5f, 0x8a, 0xdd, 0x32}}
  
Better change this since you're changing nsIFrame just a little bit :-)

+  PRBool IsBoxWrapped() const

How about just naming it "IsBoxChild" or better still, "ParentIsBox"?

+  // Box layout methods

I think you should put a big comment in here, something like

  // BOX LAYOUT METHODS
  // These methods have been migrated from nsIBox and are in the process of
  // being refactored. DO NOT USE OUTSIDE OF XUL.
...
  // END OF BOX LAYOUT METHODS
  // The above methods have been migrated from nsIBox and are in the process of
  // being refactored. DO NOT USE OUTSIDE OF XUL.

+++ layout/base/public/nsIScrollableFrame.h	8 Sep 2004 21:44:13 -0000
+#include "nsIFrame.h"

I don't think you need this.

+NS_IMETHODIMP
+nsBlockFrame::SetParent(const nsIFrame* aParent)

Please add an XXX comment noting that when we change to !IsBoxWrapped, we maybe
should clear NS_BLOCK_SPACE_MGR, but it's hard to figure out whether we should
or not.

+// Struct containing cached metrics for box-wrapped frames.
+struct nsBoxLayoutMetrics
+{
+  nsSize prefSize;
+  nsSize minSize;
+  nsSize maxSize;
+
+  nsSize blockMinSize;
+  nsSize blockPrefSize;
+  nscoord blockAscent;
+
+  nscoord flex;
+  nscoord ascent;
+
+  nsSize lastSize;
+  nsSize overflow;
+
+  PRPackedBool includeOverflow;
+  PRPackedBool wasCollapsed;
+  PRPackedBool styleChange;
+};

Can't you make these fields "m..."?

-  // If the NS_REFLOW_CALC_MAX_WIDTH flag is set on the nsHTMLReflowMetrics,
-  // then we need to do a reflow so that aDesiredSize.mMaximumWidth will be set
-  // correctly.
-  needsReflow = needsReflow || (aDesiredSize.mFlags & NS_REFLOW_CALC_MAX_WIDTH);

Why'd you get rid of this?

+NS_IMETHODIMP
+nsFrame::SetParent(const nsIFrame* aParent)

Why are you keeping the BoxMetrics around even if you're no longer a child of a box?

I'm up to @@ -936,19 +924,45 @@ nsBoxFrame::GetPrefSize(nsBoxLayoutState

Comment 5

[Brian Ryner (not reading)]

> +  PRBool IsBoxWrapped() const
> 
> How about just naming it "IsBoxChild" or better still, "ParentIsBox"?

It's not quite that though.  It's "IsNotBoxAndParentIsBox".

> +// Struct containing cached metrics for box-wrapped frames.
> +struct nsBoxLayoutMetrics
> +{
...

> Can't you make these fields "m..."?

I think that just looks uglier for a struct where all of the members are public,
you're always writing foo->bar.

> 
> -  // If the NS_REFLOW_CALC_MAX_WIDTH flag is set on the nsHTMLReflowMetrics,
> -  // then we need to do a reflow so that aDesiredSize.mMaximumWidth will be set
> -  // correctly.
> -  needsReflow = needsReflow || (aDesiredSize.mFlags & NS_REFLOW_CALC_MAX_WIDTH);
> 
> Why'd you get rid of this?

Unintentional, probably the result of fixing a merge conflict.

> 
> +NS_IMETHODIMP
> +nsFrame::SetParent(const nsIFrame* aParent)
> 
> Why are you keeping the BoxMetrics around even if you're no longer a child of
a box?

Yeah, I guess they can go away in that case.

Comment 6

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

> I think that just looks uglier for a struct where all of the members are public,
> you're always writing foo->bar.

Yeah but ... consistency.

> It's not quite that though.  It's "IsNotBoxAndParentIsBox".

Okay, leave it then.

Comment 7

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

Comment on attachment 158242 [details] [diff] [review]
patch with 8 lines of context requested by roc

+  if (!orderBoxes || childCount == 0)
+    return;

Make it childCount < 2

+  // sort the array by ordinal group, selection sort

Can you put a big red XXX here that we should be using an fast sort?

+//#define NS_STATE_IS_HORIZONTAL	    0x00400000

Add a comment explaining that these are commented out because they're defined
in nsIFrame now

+	 mFrames.RemoveFrame(currFrame);
+	 if (mLayoutManager)
+	   mLayoutManager->ChildrenRemoved(this, state, currFrame);
+	 currFrame->Destroy(mPresContext);

This occurs many times in nsListBoxBodyFrame. Can you factor it out into its
own method --- ListBoxRemoveFrames?

Can you explain why you removed the AddInset calls from the layout managers?
I'm sure you have a good reason but for the moment I can't see what it is.

Comment 8

[Brian Ryner (not reading)]

(In reply to comment #7)
> Can you explain why you removed the AddInset calls from the layout managers?
> I'm sure you have a good reason but for the moment I can't see what it is.
> 

Hm, I'm trying to figure that out myself.  The inset code is definitely only
utilized #ifdef DEBUG_LAYOUT, and I'd really like to just remove it... could be
that I started to do that and changed my mind.  I can add it back if you'd like,
but I'd like to avoid that function call overhead if we're not using DEBUG_LAYOUT.

Comment 9

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

Add it back in but make nsIFrame::GetInsets be an inline non-virtual method
#ifndef DEBUG_LAYOUT?

Comment 10

[Brian Ryner (not reading)]

(In reply to comment #9)
> Add it back in but make nsIFrame::GetInsets be an inline non-virtual method
> #ifndef DEBUG_LAYOUT?

Seems like I should do that for AddInsets as well.

Comment 11

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

Sure.

Comment 12

[Brian Ryner (not reading)]

Attachment: patch with roc's suggestions fixed

Comment 13

[kmike]

Seems like this broke my GTK2+Xft build. It crashes even before displaying
profile manager. Wiping out .mozilla and clobbering the tree didn't help.
Mozilla was built using gcc 3.2.3 with "-Os -march=athlon-xp -mcpu=athlon-xp
-m3dnow -mfpmath=sse", if that matters.

Stack:

#0  0x41195412 in nsHTMLContainerFrame::CreateViewForFrame(nsIFrame*,
nsIFrame*,int) () from /archives/mozilla/dist/bin/components/libgklayout.so
#1  0x4118e78e in nsFrame::BoxMetrics() const ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#2  0x4122cbed in nsBoxFrame::~nsBoxFrame() ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#3  0x411f1056 in nsCSSFrameConstructor::InitAndRestoreFrame(nsPresContext*,
nsFrameConstructorState&, nsIContent*, nsIFrame*, nsStyleContext*, nsIFrame*,
nsIFrame*) () from /archives/mozilla/dist/bin/components/libgklayout.so
#4  0x411ef3b2 in nsCSSFrameConstructor::ConstructXULFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int, int&) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#5  0x411f1d67 in nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#6  0x411f1a84 in nsCSSFrameConstructor::ConstructFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#7  0x411f7eb4 in nsCSSFrameConstructor::ProcessChildren(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, int,
nsFrameItems&, int, nsTableCreator*) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#8  0x411ef389 in nsCSSFrameConstructor::ConstructXULFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int, int&) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#9  0x411f1d67 in nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#10 0x411f1a84 in nsCSSFrameConstructor::ConstructFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#11 0x411f7eb4 in nsCSSFrameConstructor::ProcessChildren(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, int,
nsFrameItems&, int, nsTableCreator*) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#12 0x411ef389 in nsCSSFrameConstructor::ConstructXULFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int, int&) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#13 0x411f1d67 in nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#14 0x411f1a84 in nsCSSFrameConstructor::ConstructFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#15 0x411f7eb4 in nsCSSFrameConstructor::ProcessChildren(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, int,
nsFrameItems&, int, nsTableCreator*) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#16 0x411ef389 in nsCSSFrameConstructor::ConstructXULFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int, int&) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#17 0x411f1d67 in nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#18 0x411f1a84 in nsCSSFrameConstructor::ConstructFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#19 0x411f7eb4 in nsCSSFrameConstructor::ProcessChildren(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, int,
nsFrameItems&, int, nsTableCreator*) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#20 0x411ec0ff in nsCSSFrameConstructor::ConstructDocElementFrame(nsIPresShell*,
nsPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIFrame*&)
    () from /archives/mozilla/dist/bin/components/libgklayout.so
#21 0x411f4987 in nsCSSFrameConstructor::ContentInserted(nsPresContext*,
nsIContent*, nsIFrame*, nsIContent*, int, nsILayoutHistoryState*, int) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#22 0x411b4983 in PresShell::SetPrefFocusRules() ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#23 0x413b9162 in nsXULDocument::StartLayout() ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#24 0x413ba85a in nsXULDocument::ResumeWalk() ()
   from /archives/mozilla/dist/bin/components/libgklayout.so
#25 0x413baffb in nsXULDocument::LoadScript(nsXULPrototypeScript*, int*) ()
   from /archives/mozilla/dist/bin/components/libgklayout.so

Comment 14

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

That regression is being tracked in bug 262054.

Comment 15

[Scott MacGregor]

Any chance the fall out from this bug is causing this new layout crash when
hitting enter in the mail compose window? Bug #262244?

Comment 16

[Brian Ryner (not reading)]

not sure why i didn't close this one.