Bug 76381 – Use precompiled headers in Chromium port on Windows

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jói Sigurðsson]

There is a trick in GYP that can be used to enable precompiled headers for Windows builds without touching any of the source files.  This speeds up a build of the WebKit Chromium port by about 18% on a fast machine (8 cores, 12 GB memory).

[Jói Sigurðsson]

Created attachment 122645 [details]
Patch

[Tony Chang]

Comment on attachment 122645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122645&action=review

Seems fine, just some minor nits.

I'm not sold on the directory Source/gyp/* for these files because chromium currently doesn't use either file in Source/gyp, but maybe that's OK.  Maybe abarth has a better suggestion.

> Source/gyp/WinPrecompile.cpp:35
> + */

The svn:executable bit appears to be set on all these files.

> Source/gyp/WinPrecompile.gypi:37
> +  'conditions': [
> +    ['OS=="win" and chromium_win_pch==1', {
> +        'variables': {
> +            'conditions': [

Nit: This is a mix of 2 space and 4 space indents.  I would use 4 space indents since that's more consistent with the rest of WebKit.

> Source/gyp/WinPrecompile.gypi:41
> +                # We need to calculate the path to the gyp directory
> +                # differently depending on whether we are being built
> +                # stand-alone (via build-webkit --chromium) or as part
> +                # of the Chromium checkout.

80 column line limit doesn't exist in webKit so feel free to unwrap this comment into 2 lines or so.

[Tony Chang]

(In reply to comment #2)
> 
> I'm not sold on the directory Source/gyp/* for these files because chromium currently doesn't use either file in Source/gyp, but maybe that's OK.  Maybe abarth has a better suggestion.

On IRC abarth suggested putting the new files in Source/JavaScriptCore/wtf/win/*.

[Jói Sigurðsson]

Created attachment 122901 [details]
Patch

[WebKit Review Bot]

Attachment 122901 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/WebKit/chromium/WinPrecompile.h:60:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jói Sigurðsson]

I've uploaded a new patch, which:
- Removes the svn:executable bit that was accidentally added;
- Fixes the mixed 2/4 space indentation (uses 4-space as suggested);
- Unwraps the comment as suggested;
- Moves the new files to Source/WebKit/chromium.

That last bit is counter to abarth's suggestion (which was Source/JavaScriptCore/wtf/win).  I thought about it a bit and it seems weird to move the files to a top-level directory that they're not specific to (JavaScriptCore).  OTOH, these files are specific to the Chromium port, so it could make sense to keep them there.

Let me know what you think, I'm happy to move them back to the location suggested by abarth, or to a new location.

[Tony Chang]

Comment on attachment 122901 [details]
Patch

Putting the files in Source/WebKit/chromium seems fine to me. We can always move them later.

[Tony Chang]

Joi, do you want me to land this patch?

[Jói Sigurðsson]

Tony, that would be great, and thanks for the review.

[Tony Chang]

Committed r105428: <http://trac.webkit.org/changeset/105428>