Bug 112638 – [GTK] Add support for building the WebCore bindings to the gyp build

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The WebCore bindings is perhaps the most complex part of building WebCore. We can separate it out and add it to the WebCoreGTK build.

[Martin Robinson]

Created attachment 193698 [details]
Patch

[Martin Robinson]

Created attachment 193886 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193886 [details]
Patch

Attachment 193886 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17244035

[WebKit Review Bot]

Comment on attachment 193886 [details]
Patch

Attachment 193886 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17015824

[Peter Beverloo (cr-android ews)]

Comment on attachment 193886 [details]
Patch

Attachment 193886 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17238284

[Nico Weber]

Comment on attachment 193886 [details]
Patch

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

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:42
> +    'WebCore': '..',

Why this?

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:122
> +            # The relative path here is to work around a quirk of gyp that removes duplicate command-line arguments.

Is this filed somewhere?

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:222
> +          'message': 'Generating InjectedScriptSource.h from InjectedScriptSource.js',

These all look pretty similar. Could you instead have a single target with a 'rule' that has all these .js files in its sources section?

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:578
> +        {

This too is pretty repetitive. If a rule can't express this (maybe due to this having many outputs?), one thing we do in chromium is to have gypi files for actions like so: https://code.google.com/p/chromium/codesearch#chromium/src/build/jar_file_jni_generator.gypi&q=jar_file_jni_generator.gypi&sq=package:chromium&type=cs&l=16

[Nico Weber]

Comment on attachment 193886 [details]
Patch

(I'll mark this r- for now so others don't feel the need to comment.)

[Martin Robinson]

Comment on attachment 193886 [details]
Patch

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

>> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:42
>> +    'WebCore': '..',
> 
> Why this?

I've established this pattern in JavaScriptCoreGTK.gyp and WTFGTK.gyp. Originally it was because Dirk suggested avoiding relative paths in gypfiles. I think it does make things a little easier to read.

>> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:122
>> +            # The relative path here is to work around a quirk of gyp that removes duplicate command-line arguments.
> 
> Is this filed somewhere?

I found https://code.google.com/p/chromium/issues/detail?id=146682, but it seems that the Chromium WebCore.gyp also goes through a lot of acrobatics to avoid this. I confirmed that it still happens.

>> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:222
>> +          'message': 'Generating InjectedScriptSource.h from InjectedScriptSource.js',
> 
> These all look pretty similar. Could you instead have a single target with a 'rule' that has all these .js files in its sources section?

I gave this a shot, but was stymied by INPUT_RULE_EXT maintaining the '.' part of the path [1], rules not supporting multiple extensions (thus needing to duplicate the target for InspectorOverlayPage.html), and realizing that DebuggerScriptSource can just be removed.

1. This means that the first argument looks like InjectedScriptSource_.js instead of InjectedScriptSource_js.

>> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:578
>> +        {
> 
> This too is pretty repetitive. If a rule can't express this (maybe due to this having many outputs?), one thing we do in chromium is to have gypi files for actions like so: https://code.google.com/p/chromium/codesearch#chromium/src/build/jar_file_jni_generator.gypi&q=jar_file_jni_generator.gypi&sq=package:chromium&type=cs&l=16

Neat idea. This worked out well. Perhaps I can reuse this in WebCore.gyp for Chromium in a followup patch.

[Martin Robinson]

Created attachment 194118 [details]
Patch

[Nico Weber]

Comment on attachment 194118 [details]
Patch

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

Only one real question (about the chromium build), and a few small nits.

> Source/WebCore/WebCore.gyp/MakeNames.gypi:1
> +{

Can you add a "how to use" comment at the top? Also, does this need a copyright notice?

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:124
> +            # The relative path here is to work around a quirk of gyp that removes duplicate command-line arguments.

Maybe mention http://crbug.com/146682 in this comment.

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:166
> +            '<(WebCore)/inspector/xxd.pl',

Is it worth having a gypi file for xxd.pl actions too? It's used in a few places (and slightly inconsistently: The last two uses don't list xxd.pl as an input, the remaining do.)

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:569
> +            '<(WebCore)/dom/make_event_factory.pl',

Is this right? Does the python script shell out to the pl script? Or is this a copy-pasto?

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:588
> +            '<(WebCore)/dom/make_event_factory.pl',

(same question)

> Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:606
> +            '<(WebCore)/dom/make_dom_exceptions.pl',

(same question. also in at least one more spot below, so I'll stop saying this.)

> Source/WebCore/WebCore.gypi:-5774
> -            '<(PRODUCT_DIR)/DerivedSources/WebCore/CSSGrammar.cpp',

Isn't this file used by chromium too? Won't this change break the chromium build?

[Martin Robinson]

Created attachment 194636 [details]
Address review comments and add Source/Platform includes

[Martin Robinson]

(In reply to comment #10)
> (From update of attachment 194118 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194118&action=review
> 
> Only one real question (about the chromium build), and a few small nits.
> 
> > Source/WebCore/WebCore.gyp/MakeNames.gypi:1
> > +{
> 
> Can you add a "how to use" comment at the top? Also, does this need a copyright notice?

Sure. I've added it. In the GTK+ port we don't traditionally put license headers in our build files, but it's a good idea to start, so I've added that as well. WebCoreGTK.gyp always had a license because so much of it was cribbed from WebCore.gyp.

> 
> > Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:124
> > +            # The relative path here is to work around a quirk of gyp that removes duplicate command-line arguments.
> 
> Maybe mention http://crbug.com/146682 in this comment.

Done.

> 
> > Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:166
> > +            '<(WebCore)/inspector/xxd.pl',
> 
> Is it worth having a gypi file for xxd.pl actions too? It's used in a few places (and slightly inconsistently: The last two uses don't list xxd.pl as an input, the remaining do.)

Another great suggestion! This made things a lot more compact. I need to remember this pattern. Wish it was possible to do it without the include.
 
> > Source/WebCore/WebCore.gyp/WebCoreGTK.gyp:569
> > +            '<(WebCore)/dom/make_event_factory.pl',
> 
> Is this right? Does the python script shell out to the pl script? Or is this a copy-pasto?

The python script shells out. We share this with the Chromium port.

> > Source/WebCore/WebCore.gypi:-5774
> > -            '<(PRODUCT_DIR)/DerivedSources/WebCore/CSSGrammar.cpp',
> 
> Isn't this file used by chromium too? Won't this change break the chromium build?

This particular variable was only used by the (now deleted) Mac port gyp build, so there should be no risk of changing it. The Chromium port uses another path altogether.

[Nico Weber]

Comment on attachment 194636 [details]
Address review comments and add Source/Platform includes

Thanks for the changes!

bdash was playing with gyp for the mac port in bug 109248, but that seems to have died down. We can worry about that if it gets revived.

[Martin Robinson]

Committed r146677: <http://trac.webkit.org/changeset/146677>