978203 - Don't include nsWindowMemoryReporter.h in nsINode.h

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

It is annoying to basically have to rebuild the world any time I touch this file, especially given that the reason it has to be included is a one line macro.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Don't include nsWindowMemoryReporter.h in nsINode.h (WIP)

Bootlegging ahoy!  I should make sure this builds without unified builds.

Comment 2

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=7896fab53a7b

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Don't include nsWindowMemoryReporter.h in nsINode.h.

The key here is moving the definition of NS_DECL_SIZEOF_EXCLUDING_THIS from nsWindowMemoryReporter.h to nsINode.h.  It seems like a silly reason for an entire include.  Then I fixed the fallout.  I confirmed locally and on try that nonunified builds work.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8384567 [details] [diff] [review]
Don't include nsWindowMemoryReporter.h in nsINode.h.

I thing we should just remove the macro and have 
virtual size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
explicitly in dom classes. That brings some consistency with other
code having the same method.

Comment 5

[Andrew McCreight [:mccr8]]

Nick, does it sound reasonable to inline NS_DECL_SIZEOF_EXCLUDING_THIS?  The macro looks like some vestigial thing from an earlier iteration of the memory reporters.

Comment 6

[Nicholas Nethercote [inactive]]

> I thing we should just remove the macro and have 
> virtual size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> const;
> explicitly in dom classes. That brings some consistency with other
> code having the same method.

Please don't remove the macro! We have bug 956400 open which requires the macro so that MOZ_WARN_UNUSED_RESULT can be used consistently.

Comment 7

[Andrew McCreight [:mccr8]]

Ok.  Is there some reason this macro is only used for 5 different node classes and nothing else?

Comment 8

[Nicholas Nethercote [inactive]]

> Ok.  Is there some reason this macro is only used for 5 different node
> classes and nothing else?

No deep reason, but bug 956400 will move that macro into MFBT and then use it much more widely.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

That is fine to me. I just want consistency. Either use the macro everywhere or don't have it at all.

Comment 10

[Andrew McCreight [:mccr8]]

So are you okay with me landing this as is, with a comment added about why it is there, or should I create a new MFBT thing with the existing macro or something and move it there (and get some mfbt person to review that)?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8384567 [details] [diff] [review]
Don't include nsWindowMemoryReporter.h in nsINode.h.

Ok.

(I hope you pushed this to try)

Comment 12

[Nicholas Nethercote [inactive]]

Moving the macro is fine, just don't remove its uses. Thanks.

Comment 13

[Andrew McCreight [:mccr8]]

I mean, I don't think changing it in 5 more places is really a big deal, but I just left it alone, moving it to nsINode.h and adding a comment about the other bug that will fancy it up and make it a full-fledged citizen.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8a7062ad8d

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8a7062ad8d for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=35617001&tree=Mozilla-Inbound


[14:11]	<KWierso>	mccr8: bustage
[14:11]	<dholbert>	mccr8, (the #include removal seems to have pulled the DebugOnly.h header out from under an unrelated .cpp file)
	(which was getting it indirectly, I guess)

Comment 15

[Andrew McCreight [:mccr8]]

Yeah, I tested on m-c right before I pushed, but I guess something landed on inbound since the last merge that broke it. :(   I'll test on inbound right before I push next time.

Comment 16

[Andrew McCreight [:mccr8]]

I built locally with a non-unified build on inbound tip, so it should be okay.  There was just that one DebugOnly.h include missing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/583aad7e386c

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/583aad7e386c