Bug 24653 – WebKit should be buildable without prefix header injection

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Mark Mentovai]

For the Chromium Mac build, I'm working on a system to speed along our sluggish buildbot-driven continuous builds.  This system involves using distcc in pump mode to distribute the compilation to a number of systems, some of which are not even Macs.  (They're running Linux and have a full Apple cross-toolchain.)

distcc's pump mode is not compatible with prefix header injection ("gcc -include" or the GCC_PREFIX_HEADER Xcode setting.)

The WebCore Mac build is currently dependent on prefix header injection.  It precompiles WebCorePrefix.h.  It seems that Mac-specific parts of WebCore are the only parts that actually rely on a prefix header; all of the platform-independent bits build without prefix header injection.

The Chromium Mac build still builds some Mac files from WebCore.

In order to get everything to work properly with my new distcc setup, I need to make WebCore buildable without prefix header injection, even on the Mac.  To be clear, I am not eliminating the ability to build with a [precompiled] prefix header.  I'm merely adding the #includes and forward declarations that are needed to make the Mac-specific portions of WebCore used by Chromium build when the prefix header is not in use.

Further reading on distcc's pump mode: http://google-opensource.blogspot.com/2008/08/distccs-pump-mode-new-design-for.html .

[Eric Seidel (no email)]

Hopefully Apple will chose to look at shipping distcc 3.0 in the next Xcode release anyway. :)  In which case the Apple WebKit folks will be incentivized to make pump-mode work too! :)

[Mark Mentovai]

Created attachment 28698 [details]
Adds missing #includes and forward declarations as needed

Hopefully indeed.

Eric or Dimitri, if you would be so kind...

[Eric Seidel (no email)]

Comment on attachment 28698 [details]
Adds missing #includes and forward declarations as needed

This is not something for Dimitri or I to review.  Mark Rowe, or someone who works with the Mac port at Apple should review this.

[Mark Rowe (bdash)]

Comment on attachment 28698 [details]
Adds missing #includes and forward declarations as needed

The includes need to be consistently sorted in case-sensitive alphabetical order.  In some places this was done, but in others they were not sorted or were added in a separate paragraph from existing includes.  #import or #include should be used consistently within FoundationExtras.h, and the includes should not be added in between the comment describing HardRetain/HardRelease and their definitions.

[Mark Mentovai]

Created attachment 28701 [details]
Sorted as requested

[Mark Rowe (bdash)]

Comment on attachment 28701 [details]
Sorted as requested

> +typedef unsigned int NSFontTraitMask;

In 10.5 and beyond NSFontTraitMask is declared by AppKit as a NSUInteger.  I suspect that this will lead to problems, possibly a build breakage, in 64-bit where NSUInteger is a typedef of unsigned long rather than unsigned int.

r=me if you've verified that this doesn't break the 64-bit build.

[Mark Mentovai]

Created attachment 28705 [details]
Without NSUInteger problems

Good catch.  This version just gets that typedef from the header to minimize conflicts.  NSFontManager.h needs to be #imported (no guards), so I've flipped the other #include in that file over to be consistent.

[Mark Mentovai]

Comment on attachment 28705 [details]
Without NSUInteger problems

One more coming

[Mark Rowe (bdash)]

Comment on attachment 28705 [details]
Without NSUInteger problems

Now that you're including NSFontManager.h, there's no need to forward declare NSPoint or NSRect.  NSFontManager.h includes NSGeometry.h, which provides the real declarations for NSPoint and NSRect.

[Mark Mentovai]

Created attachment 28708 [details]
v4

OK, in addition to the removal of the NSPoint/NSRect forward declarations, this version is more careful to use #import for the guard-less AppKit headers, and fixes the order in one file I missed the sort on.

I think this should be the final version.  It works with both the Chromium build (with prefix header injection disabled) and the stock WebKit Mac build (with the precompiled prefix header).

[Mark Mentovai]

Created attachment 28709 [details]
v5

Sorry for the noise.  This one moves a Foundation #include to #import for the same reason, Foundation is guard-less and expects to be #imported.

[Mark Mentovai]

Mark, I'm done making extra patches here now, this is ready for final review.

[Eric Seidel (no email)]

Comment on attachment 28709 [details]
v5

The include of objc/objc.h in PlatformString.h seems a bit strange.  PlatformString.h is included in almost every file in WebCore, so it seems like you're trying to use it as a prefix header here... unless there is some obj-c specific logic in that file?

[Mark Rowe (bdash)]

(In reply to comment #13)
> (From update of attachment 28709 [details] [review])
> The include of objc/objc.h in PlatformString.h seems a bit strange. 
> PlatformString.h is included in almost every file in WebCore, so it seems like
> you're trying to use it as a prefix header here... unless there is some obj-c
> specific logic in that file?
> 

Presumably it needs this for the definition of "nil" which is used later on in the file.

[Mark Mentovai]

That's correct, objc.h is needed for http://trac.webkit.org/browser/trunk/WebCore/platform/text/PlatformString.h#L282 .

[Mark Mentovai]

Created attachment 28797 [details]
v6

Needed one more change, ColorMac.mm uses NSApp and was apparently getting it via some other #import until recently.

[Mark Mentovai]

Comment on attachment 28797 [details]
v6

Does this need another r+ or are the earlier r+s enough to get this checked in?

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/41939.

[Darin Adler]

It’s highly likely this will break over and over again. I’m not sure this is the right solution long term.

[Mark Mentovai]

I'm aware, the long-term solution is actually for Chromium Mac to continue reducing dependencies on WebKit Mac platform code, except where it really makes sense to share.  We'll* just need to keep up with this for the time being, since some of the Chromium builds depend on no precompilation (making the prefix header a loss) and no prefix headers at all.

* as the de facto Chromium Mac build czar, I know what "we" means.

[Eric Seidel (no email)]

(In reply to comment #20)
> We'll* just need to keep up with this for the time being,
> since some of the Chromium builds depend on no precompilation (making the
> prefix header a loss) and no prefix headers at all.

We have a Chromium Mac buildbot in mountain view wired up to build.webkit.org.  Currently it's turned off because build-webkit --chromium is broken.  Perhaps we should have it build some other version of webkit with no precomp?  If there would be an easy way to set up such a build, it's certainly possible to add a webkit.org builder for it.