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
Benjamin Otte
2009-12-17 07:47:11 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).
style-queue ran check-webkit-style on attachment 45070 [details] without any errors.
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. 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 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 on attachment 45085 [details]
another patch
Looks good to me. I'll take care of landing these two patches.
Comment on attachment 45070 [details] patch Landed in r52275 Committed r52278: <http://trac.webkit.org/changeset/52278> (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. |