Is there any reason to not make nsIFrame::AddSizeOfExcludingThisForTree virtual, so subclasses can easily report stuff?

Oh, and see bug 1397380 comment 8 item 4.

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #0) > Is there any reason to not make nsIFrame::AddSizeOfExcludingThisForTree > virtual, so subclasses can easily report stuff? The only subclass of nsIFrame I can see is nsBox. And I don't see how nsButtonFrameRenderer is related. What am I missing?

(In reply to Nicholas Nethercote [:njn] from comment #2) > (In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from > comment #0) > > Is there any reason to not make nsIFrame::AddSizeOfExcludingThisForTree > > virtual, so subclasses can easily report stuff? > > The only subclass of nsIFrame I can see is nsBox. And I don't see how > nsButtonFrameRenderer is related. What am I missing? nsIFrame has tons of subclasses... 154 instances from DXR: https://dxr.mozilla.org/mozilla-central/search?q=%2Bderived%3AnsIFrame

> And I don't see how nsButtonFrameRenderer is related. What am I missing? The nsButtonFrameRenderer lives in the mRenderer member of nsHTMLButtonControlFrame, whose inheritance chain looks like this: nsHTMLButtonControlFrame -> nsContainerFrame -> nsSplittableFrame -> nsFrame -> nsBox -> nsIFrame If we want to memory-report things hanging off the nsButtonFrameRenderer, we need to reach it from memory-reporting code somehow. One way is to do it while we're doing our frametree walk anyway in AddSizeOfExcludingThisForTree, but that involves making that method virtual, or adding a virtual hook to it, or explicitly checking for a button control frame in the method and calling something on it. We could also look into other ways to register an nsButtonFrameRenderer or nsHTMLButtonControlFrame with memory-reporting code, of course...

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #0) > Is there any reason to not make nsIFrame::AddSizeOfExcludingThisForTree > virtual, so subclasses can easily report stuff? No. We have other similar classes where *SizeOf* functions are virtual.

MozReview-Commit-ID: cJar2LDrCt

Comment on attachment 8917956 [details] [diff] [review] Add memory reporting for button frame's mInnerFocusStyle and other additional style contexts Review of attachment 8917956 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +10717,5 @@ > } > + > + // And our additional style contexts. > + int32_t index = 0; > + while (auto* extra = GetAdditionalStyleContext(index++)->AsServo()) { AsServo dereferences the pointer in debug builds, so I'm not sure this will work as such. With that fixed, like: while (auto* extra = GetAdditionalStylecontext(index++)) { // ... extra->AsServo()->AddSizeOfIncludingThis(aSizes, ...) } r=me

Good catch!

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b109053be6ac Add memory reporting for button frame's mInnerFocusStyle and other additional style contexts. r=emilio