386801 - nsAbsoluteContainingBlock doesn't need to store a child list name

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Remove nsFixedContainingBlock, store/use the child list name only if debug

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).

Comment 1

[Jeff Walden [:Waldo]]

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 2

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

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.

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

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

Comment on attachment 270829 [details] [diff] [review]
Remove nsFixedContainingBlock, store/use the child list name only if debug

a19=dbaron

Comment 5

[Jeff Walden [:Waldo]]

Fixed.