904264 - Don't #include js/MemoryMetrics.h in xpcpublic.h

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 789217 [details] [diff] [review]
Patch (v1)

Review of attachment 789217 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsWindowMemoryReporter.cpp
@@ +13,5 @@
>  #include "mozilla/Preferences.h"
>  #include "nsNetCID.h"
>  #include "nsPrintfCString.h"
>  #include "XPCJSMemoryReporter.h"
> +#include "js/MemoryMetrics.h"

What's this needed for?  I looked through the files and couldn't see what causes the dependency.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Comment on attachment 789217 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=789217
> Patch (v1)
> 
> Review of attachment 789217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsWindowMemoryReporter.cpp
> @@ +13,5 @@
> >  #include "mozilla/Preferences.h"
> >  #include "nsNetCID.h"
> >  #include "nsPrintfCString.h"
> >  #include "XPCJSMemoryReporter.h"
> > +#include "js/MemoryMetrics.h"
> 
> What's this needed for?  I looked through the files and couldn't see what
> causes the dependency.

There's a call to MemoryReportingSundriesThreshold further down in this file.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 789217 [details] [diff] [review]
Patch (v1)

Review of attachment 789217 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/xpcpublic.h
@@ +71,5 @@
> +namespace JS {
> +
> +struct RuntimeStats;
> +
> +}

just doing struct JS::RuntimeStats might be nicer.

Comment 5

[Nicholas Nethercote [inactive]]

> There's a call to MemoryReportingSundriesThreshold further down in this file.

We should move that into mfbt/MemoryReporting.h.  Alternatively, we could just define FRAME_SUNDRIES_THRESHOLD directly within nsWindowMemoryReporter.cpp.  There's no terribly compelling reason for the JS engine's threshold to be the same as this one, IMO.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Bobby Holley (:bholley) from comment #4)
> Comment on attachment 789217 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 789217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/src/xpcpublic.h
> @@ +71,5 @@
> > +namespace JS {
> > +
> > +struct RuntimeStats;
> > +
> > +}
> 
> just doing struct JS::RuntimeStats might be nicer.

I'd be surprised if that was supported everywhere.

Comment 7

[(no longer active)]

(In reply to :Ms2ger from comment #6)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > Comment on attachment 789217 [details] [diff] [review]
> > Patch (v1)
> > 
> > Review of attachment 789217 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/xpconnect/src/xpcpublic.h
> > @@ +71,5 @@
> > > +namespace JS {
> > > +
> > > +struct RuntimeStats;
> > > +
> > > +}
> > 
> > just doing struct JS::RuntimeStats might be nicer.
> 
> I'd be surprised if that was supported everywhere.

Yeah, that's not really C++.  ;-)

Comment 8

[(no longer active)]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> > There's a call to MemoryReportingSundriesThreshold further down in this file.
> 
> We should move that into mfbt/MemoryReporting.h.  Alternatively, we could
> just define FRAME_SUNDRIES_THRESHOLD directly within
> nsWindowMemoryReporter.cpp.  There's no terribly compelling reason for the
> JS engine's threshold to be the same as this one, IMO.

Can you please file a bug about that?  I know zero things about what I'm doing in this code.  ;-)_

Comment 9

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/419d5fb1dede

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/419d5fb1dede