Bug 32663

Summary: Don't include all JSC headers everywhere
Product: WebKit Reporter: Benjamin Otte <otte>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, eric, evan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch
commit-queue: commit-queue-
another patch aroben: review+, aroben: commit-queue-

Description Benjamin Otte 2009-12-17 07:47:11 PST
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.
Comment 1 Benjamin Otte 2009-12-17 08:02:42 PST
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).
Comment 2 WebKit Review Bot 2009-12-17 08:04:46 PST
style-queue ran check-webkit-style on attachment 45070 [details] without any errors.
Comment 3 Adam Roben (:aroben) 2009-12-17 08:17:43 PST
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.
Comment 4 Benjamin Otte 2009-12-17 10:31:30 PST
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.
Comment 5 WebKit Commit Bot 2009-12-17 10:32:14 PST
Comment on attachment 45070 [details]
patch

Rejecting patch 45070 from commit-queue.

otte@gnome.org 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.
Comment 6 Adam Roben (:aroben) 2009-12-17 11:07:21 PST
Comment on attachment 45085 [details]
another patch

Looks good to me. I'll take care of landing these two patches.
Comment 7 Adam Roben (:aroben) 2009-12-17 12:02:16 PST
Comment on attachment 45070 [details]
patch

Landed in r52275
Comment 8 Adam Roben (:aroben) 2009-12-17 12:39:49 PST
Committed r52278: <http://trac.webkit.org/changeset/52278>
Comment 9 Eric Seidel (no email) 2009-12-17 14:32:23 PST
(In reply to comment #5)
> (From update of attachment 45070 [details])
> Rejecting patch 45070 from commit-queue.
> 
> otte@gnome.org 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.