Bug 41941 – Clean up as many unnecessary includes in header files as possible

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Dumitru Daniliuc]

We should use WebKitTools/Scripts/check-header-includes to identify as many unnecessary #includes in header files as possible and delete them.

[Dumitru Daniliuc]

Created attachment 61021 [details]
patch #1: WebCore/accessibility

[Eric Seidel (no email)]

Comment on attachment 61021 [details]
patch #1: WebCore/accessibility

AX is actually a horrible offender of header includes.  lots and lots of needlessly inline functions which cause massive header cascades. :(

I would think this type of patch would be a really good candidate for the cq?   I'm surprised you cq-'d it, as the cq would be happy to build it for you before landing.

[Eric Seidel (no email)]

Dumi, these patches do not require review.  If you like, rs=me on all of them.

If the bots build, that's all we care about.  Headers have no magical function.

Don't change public headers (in WebKit implementations).  And don't move hot functions out of headers into .cpp files.

If you're moving inline functions, you probably want to get review on those.  But if you're just removing headers, feel free to land them unreviewed so long as you don't break the bots. :)

[Dumitru Daniliuc]

Great!

The plan for phase 1 is to remove #includes from header files without moving/changing any code. I will upload every patch I have, wait for the EWS bots to go green, then manually submit them and watch the build bots.

In phase 2 I'm planning to look at the inline code and try to move it to .cpp files. I will definitely ask for reviews for those patches.

[Darin Adler]

Comment on attachment 61021 [details]
patch #1: WebCore/accessibility

> Index: WebCore/accessibility/AccessibilityListBox.cpp
> ===================================================================
> --- WebCore/accessibility/AccessibilityListBox.cpp	(revision 62919)
> +++ WebCore/accessibility/AccessibilityListBox.cpp	(working copy)
> @@ -31,6 +31,7 @@
>  
>  #include "AXObjectCache.h"
>  #include "AccessibilityListBoxOption.h"
> +#include "AccessibilityObject.h"
>  #include "HTMLNames.h"
>  #include "HTMLSelectElement.h"
>  #include "HitTestResult.h"

There's no need to add this. AccessibilityListBox is derived from a class that's derived from AccessibilityObject. If the script is telling you to add this, we need to improve the script.

> +class AccessibilityObject;
> +
>  class AccessibilityListBox : public AccessibilityRenderObject {

There's no need to add the forward declaration for AccessibilityObject here. AccessibilityRenderObject is derived from AccessibilityObject. Again, if the script is telling you to add this, we need to improve the script.

Otherwise, the patch seems fine as long as it doesn't break the build on any platform.

[Dumitru Daniliuc]

Created attachment 61062 [details]
patch #1: WebCore/accessibility

[Dumitru Daniliuc]

> There's no need to add this. AccessibilityListBox is derived from a class that's derived from AccessibilityObject. If the script is telling you to add this, we need to improve the script.

> There's no need to add the forward declaration for AccessibilityObject here. AccessibilityRenderObject is derived from AccessibilityObject. Again, if the script is telling you to add this, we need to improve the script.

The script tells me that AccessibilityObject doesn't need to be included. However, it doesn't know that AccessibilityRenderObject is derived from AccessibilityObject, so it doesn't know if we need to forward declare it + include it in the .cpp file, or not. I think it should be fairly easy to get that information too, I'll open a bug to track that.

[Dumitru Daniliuc]

Comment on attachment 61062 [details]
patch #1: WebCore/accessibility

patch #1 landed: r62978

[Dumitru Daniliuc]

Created attachment 61082 [details]
patch #2: WebCore/css

[Eric Seidel (no email)]

Attachment 61082 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3495069

[Dumitru Daniliuc]

Created attachment 61083 [details]
patch #2: WebCore/css

[Dumitru Daniliuc]

Comment on attachment 61083 [details]
patch #2: WebCore/css

patch #2 landed: r63014.

[Darin Adler]

Comment on attachment 61083 [details]
patch #2: WebCore/css

> +#include "PlatformString.h"
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>

Including PlatformString.h includes both RefCounted.h and PassRefPtr.h I don’t understand why you added explicit includes of those in a patch that’s about reducing includes.

[Dumitru Daniliuc]

(In reply to comment #13)
> (From update of attachment 61083 [details])
> > +#include "PlatformString.h"
> > +#include <wtf/PassRefPtr.h>
> > +#include <wtf/RefCounted.h>
> 
> Including PlatformString.h includes both RefCounted.h and PassRefPtr.h I don’t understand why you added explicit includes of those in a patch that’s about reducing includes.

Didn't notice that. :( Will clean it up.

[Dumitru Daniliuc]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 61083 [details] [details])
> > > +#include "PlatformString.h"
> > > +#include <wtf/PassRefPtr.h>
> > > +#include <wtf/RefCounted.h>
> > 
> > Including PlatformString.h includes both RefCounted.h and PassRefPtr.h I don’t understand why you added explicit includes of those in a patch that’s about reducing includes.
> 
> Didn't notice that. :( Will clean it up.

Once all header files are fairly clean, I can change the script to complain about cases like:

B.h:
#include "A.h"

C.h:
#include "A.h" <--- not needed
#include "B.h"

But until then, I'll try to be more careful about this.

[Dumitru Daniliuc]

Created attachment 61140 [details]
patch #3: WebCore/dom + fix to WebCore/css/StyleMedia.h

[Darin Adler]

Comment on attachment 61140 [details]
patch #3: WebCore/dom + fix to WebCore/css/StyleMedia.h

Looks good.

You should know that in many cases we can compile headers that declare, but do not define, functions that use PassRefPtr.h without even including PassRefPtr.h. Instead we can include <wtf/Forward.h>. That’s the purpose of the Forward.h function. To be a smaller header with forward declarations of templates where you can’t just forward declare the class in the usual way. I don’t know if that’s an important optimization, though. Code that uses PassRefPtr is going to need to include PassRefPtr.h eventually anyway.

[Darin Adler]

(In reply to comment #15)
> Once all header files are fairly clean, I can change the script to complain about cases like:
> 
> B.h:
> #include "A.h"
> 
> C.h:
> #include "A.h" <--- not needed
> #include "B.h"

That’s the whole thing find-extra-includes does.

[Dumitru Daniliuc]

patch #3 landed: r63042.

[Dumitru Daniliuc]

Created attachment 61277 [details]
patch #4: WebCore/editing, WebCore/history and WebCore/html

Please do not r+ the patch before all EWS bots have turned green.

[Early Warning System Bot]

Attachment 61277 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3426254

[Darin Adler]

Comment on attachment 61277 [details]
patch #4: WebCore/editing, WebCore/history and WebCore/html

Qt failure is:

generated/JSHTMLLabelElement.cpp: In function 'JSC::JSValue WebCore::jsHTMLLabelElementControl(JSC::ExecState*, JSC::JSValue, const JSC::Identifier&)':
generated/JSHTMLLabelElement.cpp:182: error: no matching function for call to 'toJS(JSC::ExecState*&, WebCore::JSDOMGlobalObject*, WebCore::HTMLFormControlElement*)'
../../../WebCore/bindings/js/JSNodeCustom.h:48: note: candidates are: JSC::JSValue WebCore::toJS(JSC::ExecState*, WebCore::JSDOMGlobalObject*, WebCore::Node*)

[WebKit Review Bot]

Attachment 61277 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3529049

[WebKit Review Bot]

Attachment 61277 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/canvas/WebGLUniformLocation.cpp:31:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dumitru Daniliuc]

Created attachment 61295 [details]
patch #4: WebCore/editing, WebCore/history and WebCore/html

One more try.

[Early Warning System Bot]

Attachment 61295 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3471231

[WebKit Review Bot]

Attachment 61295 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3426258

[WebKit Review Bot]

Attachment 61295 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3473234

[Dumitru Daniliuc]

Created attachment 61327 [details]
patch #4: more clean-up in WebCore/accessibility and WebCore/css

[Dumitru Daniliuc]

Comment on attachment 61327 [details]
patch #4: more clean-up in WebCore/accessibility and WebCore/css

Not submitting patch #4, and closing bug.

From what I've seen so far, the number of unused includes is quite small. And after talking to eseidel, it looks like there's no benefit in minimizing the number of includes, since that doesn't really help with build times or library sizes.