Closed Bug 1053190 Opened 5 years ago Closed 5 years ago

Reduce unnecessary inclusion of nsINode.h in headers

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(1 file)

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.
Attached patch PatchSplinter Review
Attachment #8472331 - Flags: review?(ehsan)
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...)
Attachment #8472331 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/6030bd4e48f8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
> 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.
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.
(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 :(
Unable to verify, this seems to be a back end issue.
QA Whiteboard: [QAnalyst-verify-]
QA Whiteboard: [QAnalyst-verify-] → [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.