1397815 - stylo: Need memory reporting for button frame's mInnerFocusStyle

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and see bug 1397380 comment 8 item 4.

Comment 2

[Nicholas Nethercote [inactive]]

(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?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Add memory reporting for button frame's mInnerFocusStyle and other additional style contexts

MozReview-Commit-ID: cJar2LDrCt

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Good catch!

Comment 9

[Pulsebot]

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

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b109053be6ac