Closed Bug 978203 Opened 6 years ago Closed 6 years ago
Don't include ns
Window Memory Reporter .h in ns INode .h
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.
Bootlegging ahoy! I should make sure this builds without unified builds.
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 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.
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.
> 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.
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)
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.
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.