Bug 41941

Summary: Clean up as many unnecessary includes in header files as possible
Product: WebKit Reporter: Dumitru Daniliuc <dumi>
Component: New BugsAssignee: Dumitru Daniliuc <dumi>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, darin, dglazkov, eric, gustavo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch #1: WebCore/accessibility
darin: review-, dumi: commit-queue-
patch #1: WebCore/accessibility
none
patch #2: WebCore/css
dumi: commit-queue-
patch #2: WebCore/css
none
patch #3: WebCore/dom + fix to WebCore/css/StyleMedia.h
darin: review+, dumi: commit-queue-
patch #4: WebCore/editing, WebCore/history and WebCore/html
dumi: commit-queue-
patch #4: WebCore/editing, WebCore/history and WebCore/html
dumi: commit-queue-
patch #4: more clean-up in WebCore/accessibility and WebCore/css none

Description Dumitru Daniliuc 2010-07-09 01:38:34 PDT
We should use WebKitTools/Scripts/check-header-includes to identify as many unnecessary #includes in header files as possible and delete them.
Comment 1 Dumitru Daniliuc 2010-07-09 02:19:42 PDT
Created attachment 61021 [details]
patch #1: WebCore/accessibility
Comment 2 Eric Seidel (no email) 2010-07-09 02:21:32 PDT
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.
Comment 3 Eric Seidel (no email) 2010-07-09 02:27:22 PDT
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. :)
Comment 4 Dumitru Daniliuc 2010-07-09 02:32:33 PDT
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.
Comment 5 Darin Adler 2010-07-09 08:22:35 PDT
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.
Comment 6 Dumitru Daniliuc 2010-07-09 11:10:11 PDT
Created attachment 61062 [details]
patch #1: WebCore/accessibility
Comment 7 Dumitru Daniliuc 2010-07-09 11:15:40 PDT
> 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.
Comment 8 Dumitru Daniliuc 2010-07-09 12:05:31 PDT
Comment on attachment 61062 [details]
patch #1: WebCore/accessibility

patch #1 landed: r62978
Comment 9 Dumitru Daniliuc 2010-07-09 13:30:44 PDT
Created attachment 61082 [details]
patch #2: WebCore/css
Comment 10 Eric Seidel (no email) 2010-07-09 13:39:02 PDT
Attachment 61082 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3495069
Comment 11 Dumitru Daniliuc 2010-07-09 13:46:26 PDT
Created attachment 61083 [details]
patch #2: WebCore/css
Comment 12 Dumitru Daniliuc 2010-07-09 17:12:23 PDT
Comment on attachment 61083 [details]
patch #2: WebCore/css

patch #2 landed: r63014.
Comment 13 Darin Adler 2010-07-09 17:39:59 PDT
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.
Comment 14 Dumitru Daniliuc 2010-07-09 17:41:56 PDT
(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.
Comment 15 Dumitru Daniliuc 2010-07-09 18:00:40 PDT
(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.
Comment 16 Dumitru Daniliuc 2010-07-09 19:11:43 PDT
Created attachment 61140 [details]
patch #3: WebCore/dom + fix to WebCore/css/StyleMedia.h
Comment 17 Darin Adler 2010-07-09 22:36:00 PDT
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.
Comment 18 Darin Adler 2010-07-09 22:44:08 PDT
(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.
Comment 19 Dumitru Daniliuc 2010-07-10 00:25:15 PDT
patch #3 landed: r63042.
Comment 20 Dumitru Daniliuc 2010-07-12 15:50:26 PDT
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.
Comment 21 Early Warning System Bot 2010-07-12 16:02:40 PDT
Attachment 61277 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3426254
Comment 22 Darin Adler 2010-07-12 16:04:08 PDT
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*)
Comment 23 WebKit Review Bot 2010-07-12 16:26:43 PDT
Attachment 61277 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3529049
Comment 24 WebKit Review Bot 2010-07-12 16:28:11 PDT
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.
Comment 25 Dumitru Daniliuc 2010-07-12 17:08:48 PDT
Created attachment 61295 [details]
patch #4: WebCore/editing, WebCore/history and WebCore/html

One more try.
Comment 26 Early Warning System Bot 2010-07-12 17:17:52 PDT
Attachment 61295 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3471231
Comment 27 WebKit Review Bot 2010-07-12 18:06:57 PDT
Attachment 61295 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3426258
Comment 28 WebKit Review Bot 2010-07-12 19:20:15 PDT
Attachment 61295 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3473234
Comment 29 Dumitru Daniliuc 2010-07-12 22:34:12 PDT
Created attachment 61327 [details]
patch #4: more clean-up in WebCore/accessibility and WebCore/css
Comment 30 Dumitru Daniliuc 2010-07-13 01:07:59 PDT
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.