Closed Bug 780469 Opened 12 years ago Closed 12 years ago

Reduce unneeded includes for key headers in content/

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: ayg, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

      No description provided.
Attached patch PatchSplinter Review
This cuts nsIContent.h a bunch, nsINode.h and nsIDocument.h a lot.  Let me know if you think the forward declarations of typedefs are excessive.  I moved NODE_FROM down in nsINode.h because nothing included by nsINode was forward-declaring nsINode anymore.  Naturally, tons of files were bootlegging nsDOMError.h off nsINode.h, which doesn't actually use it.

I was particularly pleased that nsGkAtoms.h doesn't need to include anything at all -- although realistically, most compilation units that include it probably include nsIAtom.h anyway.

Try: https://tbpl.mozilla.org/?tree=Try&rev=7158ee704dbd
Attachment #649096 - Flags: review?(bzbarsky)
You don't need to include members that are in nsCOMPtr, it is enough to forward declare them as long as you don't instantiate the template in the header.  I have a patch to remove all those headers from nsIDocument, I should finish it up.
But then you have to include all those headers in any caller that instantiates the class, right?  So if I didn't include those headers in nsIDocument.h itself, you could include it with no problems -- but as soon as you used the class in particular nonobvious ways, you'd suddenly have to include six random header files.  Granted, not many nsIDocument callers are going to do this, but do we want to make anyone who does that figure out the six correct files and include them by hand?  (This is why I annotated them differently, though, in case we do want to just forward-declare those.)

I did do what you suggested originally, but then I got compilation errors in a file that I'd have really expected to be able to just include nsIDocument.h and nothing else.

(Clang broke because of bug 780474, which I just filed.  New try: https://tbpl.mozilla.org/?tree=Try&rev=8de0e71e4bc6)
(In reply to :Aryeh Gregor from comment #3)
> But then you have to include all those headers in any caller that
> instantiates the class, right?  So if I didn't include those headers in
> nsIDocument.h itself, you could include it with no problems -- but as soon
> as you used the class in particular nonobvious ways, you'd suddenly have to
> include six random header files.  Granted, not many nsIDocument callers are
> going to do this, but do we want to make anyone who does that figure out the
> six correct files and include them by hand?  (This is why I annotated them
> differently, though, in case we do want to just forward-declare those.)
> 

You would have to include those headers in any files that actually use those members, not in all files that include nsIDocument.h, which is a bit of pain.  On the other hand, I don't think it's that unreasonable to require anyone calling nsIDocument::GetScriptGlobalObject() to include nsIScriptGlobalObject.h, especially if many files that include nsIDocument.h don't actually use the script global object.
In particular, IWYU will already point out that those files need to include the missing headers.
Comment on attachment 649096 [details] [diff] [review]
Patch

Yeah, I'd prefer not duplicating typedefs, because that allows them to get out of sync in the future.

r=me on the rest
Attachment #649096 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ac04d8e32b

(In reply to David Zbarsky (:dzbarsky) from comment #4)
> You would have to include those headers in any files that actually use those
> members, not in all files that include nsIDocument.h, which is a bit of
> pain.  On the other hand, I don't think it's that unreasonable to require
> anyone calling nsIDocument::GetScriptGlobalObject() to include
> nsIScriptGlobalObject.h, especially if many files that include nsIDocument.h
> don't actually use the script global object.

It's not just the files that actually use those members.  I get errors like this:

In file included from ../../../dist/include/nsWeakReference.h:12:0,
                 from ../../../dist/include/nsWindowMemoryReporter.h:12,
                 from ../../../dist/include/nsINode.h:17,
                 from ../../../dist/include/nsIDocument.h:16,
                 from ../../../dist/include/nsIContent.h:10,
                 from ../../../dist/include/mozilla/dom/FragmentOrElement.h:18,
                 from /mnt/extra/checkouts/central/content/base/src/nsGenericElement.h:17,
                 from /mnt/extra/checkouts/central/content/base/src/nsStyledElement.h:17,
                 from /mnt/extra/checkouts/central/content/base/src/nsMappedAttributeElement.h:15,
                 from /mnt/extra/checkouts/central/content/base/src/nsAttrAndChildArray.cpp:12:
../../../dist/include/nsIWeakReferenceUtils.h: In function ‘nsresult CallQueryReferent(T*, DestinationType**) [with T = nsIWeakReference, DestinationType = nsILoadGroup, nsresult = unsigned int]’:
../../../dist/include/nsIDocument.h:208:57:   instantiated from here
../../../dist/include/nsIWeakReferenceUtils.h:32:73: error: invalid use of incomplete type ‘struct nsILoadGroup’
../../../dist/include/nsIDocument.h:47:7: error: forward declaration of ‘struct nsILoadGroup’
In file included from ../../../dist/include/nsINodeInfo.h:26:0,
                 from /mnt/extra/checkouts/central/content/base/src/nsAttrName.h:15,
                 from /mnt/extra/checkouts/central/content/base/src/nsAttrAndChildArray.h:17,
                 from /mnt/extra/checkouts/central/content/base/src/nsAttrAndChildArray.cpp:11:
../../../dist/include/nsCOMPtr.h: In constructor ‘nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsILoadContext]’:
../../../dist/include/nsIDocument.h:883:73:   instantiated from here
../../../dist/include/nsCOMPtr.h:554:11: error: invalid use of incomplete type ‘struct nsILoadContext’
../../../dist/include/nsIDocument.h:46:7: error: forward declaration of ‘struct nsILoadContext’

Plus a bunch more.  This is in nsAttrAndChildArray.cpp, which does not call GetLoadContext(), or use any nsILoadContext anywhere.  Actually, it doesn't have any nsIDocument object either, or contain the string "Document".  It is not at all obvious why it should have to directly include nsILoadContext.h, but it does.  I *think* it has something to do with something somewhere calling an nsCOMPtr constructor or destructor or such, and thereby calling AddRef() or Release() on an nsILoadContext.  But I frankly don't know.  If you can figure out how to make these mysterious errors not happen, then by all means please write a patch.
Flags: in-testsuite-
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/07ac04d8e32b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Forgot about this:

(In reply to David Zbarsky (:dzbarsky) from comment #5)
> In particular, IWYU will already point out that those files need to include
> the missing headers.

That's fine; the point is that it will still *work* without them.  What I don't want is someone adding a new nsIDocument use to a file, adding #include "nsIDocument.h" at the top, and then getting a bunch of compile errors about seemingly unrelated classes being undefined.
Blocks: 815058
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: