1053190 - Reduce unnecessary inclusion of nsINode.h in headers

Status: RESOLVED FIXED

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

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I sat around waiting for several minutes for Gecko to build after making a change to nsINode.  I don't know how much this improves things, but it can't hurt.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

Comment 2

[(no longer active)]

Comment on attachment 8472331 [details] [diff] [review]
Patch

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

Nice!  (Please make sure this builds on all platforms on try...)

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6030bd4e48f8

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6030bd4e48f8

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Please make sure this builds on all platforms on try...

That should have been "in a non-unified build".  Because the change to nsINodeList.h is wrong: it returns nsIContent* from methods that bindings expect to return nsINode*, so it needs to include nsIContent.h so that callers will see that nsIContent inherits from nsINode.  It happened to build because of how the unified build stars aligned, but randomly broke other people's patches that didn't touch this code at all.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Based on the errors I get on try, it looks like at least one or two of the platforms are building non-unified, and I did have to fix a number of errors that looked like non-unified-only errors.  It might be that there was a platform-specific non-unified-only problem.

If includes are needed for non-obvious reasons, it's a good idea to explain why in a comment.  Manual or semi-automated (iwyu) inclusion reduction will remove them otherwise, and I think removing unnecessary includes is considered a good thing, so chances are it might happen again.

Comment 7

[(no longer active)]

(In reply to :Aryeh Gregor from comment #6)
> Based on the errors I get on try, it looks like at least one or two of the
> platforms are building non-unified, and I did have to fix a number of errors
> that looked like non-unified-only errors.  It might be that there was a
> platform-specific non-unified-only problem.

Yeah, IIRC linux32 perhaps among others, which is why I suggested the try run...  Sorry if this broke people's patches :(

Comment 8

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Unable to verify, this seems to be a back end issue.