Last Comment Bug 780469 - Reduce unneeded includes for key headers in content/
: Reduce unneeded includes for key headers in content/
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: includehell 815058
  Show dependency treegraph
 
Reported: 2012-08-05 06:17 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2012-11-26 00:10 PST (History)
2 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (39.11 KB, patch)
2012-08-05 06:25 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
bzbarsky: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-05 06:17:42 PDT

    
Comment 1 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-05 06:25:51 PDT
Created attachment 649096 [details] [diff] [review]
Patch

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
Comment 2 David Zbarsky (:dzbarsky) 2012-08-05 08:16:14 PDT
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.
Comment 3 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-05 08:49:34 PDT
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)
Comment 4 David Zbarsky (:dzbarsky) 2012-08-05 09:57:59 PDT
(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.
Comment 5 David Zbarsky (:dzbarsky) 2012-08-05 09:59:06 PDT
In particular, IWYU will already point out that those files need to include the missing headers.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-08-05 11:47:27 PDT
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
Comment 7 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-06 05:54:50 PDT
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.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-08-06 17:11:50 PDT
https://hg.mozilla.org/mozilla-central/rev/07ac04d8e32b
Comment 9 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-07 01:50:06 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.