Closed Bug 386801 Opened 13 years ago Closed 12 years ago

nsAbsoluteContainingBlock doesn't need to store a child list name

Categories

(Core :: Layout: Positioned, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(1 file)

In all non-debug cases where the virtual method nsAbsoluteContainingBlock::GetChildListName is called, the result of the call is already known.  This method is also the only virtual method on nsFixedContainingBlock, requiring use of a vtable.  Because of this, we can store the child list name in debug builds only, avoiding both the vtable and the nsFixedContainingBlock class which inherits from it (storing no extra data and implementing only the single method overriding the one on the superclass).
Flags: in-testsuite-
Attachment #270829 - Flags: superreview?(dbaron)
Attachment #270829 - Flags: review?(dbaron)
Comment on attachment 270829 [details] [diff] [review]
Remove nsFixedContainingBlock, store/use the child list name only if debug

>Index: layout/generic/nsAbsoluteContainingBlock.h

>   virtual ~nsAbsoluteContainingBlock() { } // useful for debugging

This should actually be non-virtual, so consider it that way.

I successfully built this patch in debug and non-debug configurations.
Comment on attachment 270829 [details] [diff] [review]
Remove nsFixedContainingBlock, store/use the child list name only if debug

>+#ifdef DEBUG
>+  nsAbsoluteContainingBlock(nsIAtom* aChildListName)
>+    : mChildListName(aChildListName)
>+  {
>+    NS_ASSERTION(mChildListName == nsGkAtoms::absoluteList ||
>+                 mChildListName == nsGkAtoms::fixedList,
>+                 "should either represent position:fixed or absolute content");
>+  }

You should make this constructor not conditional on DEBUG.  Just make the mChildListName initialization conditional on DEBUG.  The fact that it's inline should then make things compile to the same code as what the current patch does, but with less risk of compilation errors.

Then you can remove the #ifdef DEBUG around all the callers.

> 
>   virtual ~nsAbsoluteContainingBlock() { } // useful for debugging

Remove the declaration of the destructor entirely, given that it's empty and should be non-virtual.

r+sr=dbaron with that.  Sorry for the delay.
Attachment #270829 - Flags: superreview?(dbaron)
Attachment #270829 - Flags: superreview+
Attachment #270829 - Flags: review?(dbaron)
Attachment #270829 - Flags: review+
Comment on attachment 270829 [details] [diff] [review]
Remove nsFixedContainingBlock, store/use the child list name only if debug

Slight code simplification, shaves a vptr (4 bytes) off every nsViewportFrame, nsBlockFrame, and nsPositionedInlineFrame.
Attachment #270829 - Flags: approval1.9?
Comment on attachment 270829 [details] [diff] [review]
Remove nsFixedContainingBlock, store/use the child list name only if debug

a19=dbaron
Attachment #270829 - Flags: approval1.9? → approval1.9+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.