Closed Bug 978203 Opened 6 years ago Closed 6 years ago

Don't include nsWindowMemoryReporter.h in nsINode.h

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → continuation
Bootlegging ahoy!  I should make sure this builds without unified builds.
Blocks: includehell
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.
Attachment #8383917 - Attachment is obsolete: true
Attachment #8384567 - Flags: review?(bugs)
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.
Attachment #8384567 - Flags: review?(bugs)
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.
Flags: needinfo?(n.nethercote)
> 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.
Flags: needinfo?(n.nethercote)
Ok.  Is there some reason this macro is only used for 5 different node classes and nothing else?
> 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.
That is fine to me. I just want consistency. Either use the macro everywhere or don't have it at all.
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 on attachment 8384567 [details] [diff] [review]
Don't include nsWindowMemoryReporter.h in nsINode.h.

Ok.

(I hope you pushed this to try)
Attachment #8384567 - Flags: review+
Moving the macro is fine, just don't remove its uses. Thanks.
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
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)
Flags: needinfo?(continuation)
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.
Flags: needinfo?(continuation)
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
https://hg.mozilla.org/mozilla-central/rev/583aad7e386c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.