Bug 32663 – Don't include all JSC headers everywhere

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Benjamin Otte]

Currently PlatformString.h #include's almost all of JSC. As that is a pretty central header for WebCore, it means JSC headers get included almost everywhere inside a WebCore build.

[Benjamin Otte]

Created attachment 45070 [details]
patch

PlatformString.h included almost all of JSC via runtime/Identifier.h. This patch gets rid of this include by forward-declaring the required classes instead.
This reduces the build size of the object files on a Gtk debug build by 10%. The resulting libwebkit.so gets 5% smaller.

Somebody should probably make sure it still compiles on mac and qt as PlatformString.h pulled a lot of other headers that must now be included manually (like the one math.h fix for Gtk I have in the patch).

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45070 [details] without any errors.

[Adam Roben (:aroben)]

Comment on attachment 45070 [details]
patch

> -#if USE(JSC)
> -#include <runtime/Identifier.h>
> -#else
> -// runtime/Identifier.h brings in a variety of wtf headers.  We explicitly
> -// include them in the case of non-JSC builds to keep things consistent.
>  #include <wtf/HashMap.h>
>  #include <wtf/HashSet.h>
>  #include <wtf/OwnPtr.h>
> -#endif

I think it's worth adding a FIXME here saying that we shouldn't #include these headers, either, unless absolutely necessary.

r=me, but commit-queue- so you can consider adding the FIXME.

[Benjamin Otte]

Created attachment 45085 [details]
another patch

Here's a patch that removes all the headers you mentioned. It compiled without a complaint here.

I'm filing it as a separate patch so the potential fallout of missing includes can be detected easier.

[WebKit Commit Bot]

Comment on attachment 45070 [details]
patch

Rejecting patch 45070 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

- If you have committer rights please correct the error in WebKitTools/Scripts/modules/committers.py by adding yourself to the file (no review needed) and then set the committer flag again.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

[Adam Roben (:aroben)]

Comment on attachment 45085 [details]
another patch

Looks good to me. I'll take care of landing these two patches.

[Adam Roben (:aroben)]

Comment on attachment 45070 [details]
patch

Landed in r52275

[Adam Roben (:aroben)]

Committed r52278: <http://trac.webkit.org/changeset/52278>

[Eric Seidel (no email)]

(In reply to comment #5)
> (From update of attachment 45070 [details])
> Rejecting patch 45070 from commit-queue.
> 
> [e-mail] does not have committer permissions according to
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
> 
> - If you have committer rights please correct the error in
> WebKitTools/Scripts/modules/committers.py by adding yourself to the file (no
> review needed) and then set the committer flag again.
> 
> - If you do not have committer rights please read
> http://webkit.org/coding/contributing.html for instructions on how to use
> bugzilla flags.


Sorry about the rejection.  The bot currently requires a restart after mods to committres.py.  I need to update the failure message to note that.  I just restarted the bot, so you should be able to set cq+ just fine now.