Bug 52612 – Apply feature flags to user-agent style sheets

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kent Tamura]

I saw some demand for this feature in several bugs.

[Kent Tamura]

Created attachment 79252 [details]
Patch

[WebKit Review Bot]

Attachment 79252 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7598194

[Early Warning System Bot]

Attachment 79252 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7507200

[WebKit Review Bot]

Attachment 79252 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7596157

[Build Bot]

Attachment 79252 [details] did not build on win:
Build output: http://queues.webkit.org/results/7499187

[Kent Tamura]

I know I need to update build files.  But I feel it might be irregular that WebCore depends on Tools/Scripts/webkitperl.

[WebKit Review Bot]

Attachment 79252 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7545192

[Kent Tamura]

I'll move preprocessor.pm to WebCore/binding/scripts/.  It would be simpler.

[Kent Tamura]

Created attachment 79373 [details]
Patch 2 (move preprocessor.pm to WebCore/bindings/scripts/)

[Build Bot]

Attachment 79373 [details] did not build on win:
Build output: http://queues.webkit.org/results/7536192

[Kent Tamura]

Created attachment 79381 [details]
Patch 3

Fix Windows build

[Kent Tamura]

Created attachment 79395 [details]
Patch 4

Passing feature flags on Chromium, GTK, CMake

[Eric Seidel (no email)]

Comment on attachment 79395 [details]
Patch 4

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

I'm not sure why we need to do this?  Aren't these -webkit- rules going to be ignored if the features in question are not enabled anyway?

r- for lack of ChangeLog.

> Source/WebCore/bindings/scripts/preprocessor.pm:36
> +sub applyPreprocessor

Sad that this ended up as perl.  Why do we even need a pre-processor?  Can't we use GCC's?

[Kent Tamura]

(In reply to comment #13)
> I'm not sure why we need to do this?  Aren't these -webkit- rules going to be ignored if the features in question are not enabled anyway?

ok, I add more sentences to ChangeLog.
The main motivation is Bug 52214.  We need declare "datalist { display:none; }" only if ENABLE_DATALIST.

> > Source/WebCore/bindings/scripts/preprocessor.pm:36
> > +sub applyPreprocessor
> 
> Sad that this ended up as perl.  Why do we even need a pre-processor?  Can't we use GCC's?

It uses GCC if no $CC is set.  It's a common behavior with IDL processing.

[Kent Tamura]

Created attachment 79997 [details]
Patch 5

Update ChangeLog

[Eric Seidel (no email)]

Comment on attachment 79997 [details]
Patch 5

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

> Source/WebCore/bindings/scripts/preprocessor.pm:2
> +# Copyright (C) 2005 Nikolas Zimmermann <[e-mail]>

Google too?

[Kent Tamura]

Created attachment 89167 [details]
Patch 6

Add Google copyright, rebase

[Early Warning System Bot]

Attachment 89167 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8391293

[Andras Becsi]

Comment on attachment 89167 [details]
Patch 6

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

> Source/WebCore/CodeGenerators.pri:671
> +              $$PWD/bindings/scripts/preprocessor.pm

This line is missing a trailing \.
Adding this should fix the Qt build.

[Kent Tamura]

Comment on attachment 89167 [details]
Patch 6

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

>> Source/WebCore/CodeGenerators.pri:671
>> +              $$PWD/bindings/scripts/preprocessor.pm
> 
> This line is missing a trailing \.
> Adding this should fix the Qt build.

Thank you, Andras!

[Kent Tamura]

Created attachment 89174 [details]
Patch 7

Fix Qt build

[Hajime Morrita]

Looks sane. And we need this.
I'll r+ this within days if there is no objection.

Anyway, just for curious.... Is it possible to use #include to concat multiple files?

[Kent Tamura]

(In reply to comment #22)
> Anyway, just for curious.... Is it possible to use #include to concat multiple files?

Do you mean adding #include "mediaControls.css" and so on to html.css and processing only html.css?
I think It's possible.  It would be helpful if we pass not only ENABLE macros but also PLATFORM and OS macros.

[Kent Tamura]

Created attachment 93738 [details]
Patch 8

Rebase

[Hajime Morrita]

(In reply to comment #23)
> (In reply to comment #22)
> > Anyway, just for curious.... Is it possible to use #include to concat multiple files?
> 
> Do you mean adding #include "mediaControls.css" and so on to html.css and processing only html.css?
> I think It's possible.  It would be helpful if we pass not only ENABLE macros but also PLATFORM and OS macros.
Yes! And it possibly reduces the conversion script's adhocracy.
With #include, it would be clear which files are used, without digging into the perl land.

[Hajime Morrita]

Comment on attachment 93738 [details]
Patch 8

(In reply to comment #22)
> Looks sane. And we need this.
> I'll r+ this within days if there is no objection.
Seeing no objection, making r+.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 93738 [details]:

canvas/philip/tests/2d.text.draw.align.center.html bug 51028 (authors: [e-mail] and [e-mail])
http/tests/websocket/tests/multiple-connections.html bug 53825 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 93738 [details]
Patch 8

Clearing flags on attachment: 93738

Committed r86936: <http://trac.webkit.org/changeset/86936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 93738 [details]:

java/lc3/JavaObject/JavaObjectToByte-006.html bug 60333 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Evan Martin]

After this patch landed, some tools have regressed to start printing more output when building.  It looks like there are callers to applyPreprocessor() that aren't passing the last argument, which then defaults it to 0 which makes them verbose.

[Kent Tamura]

(In reply to comment #31)
> After this patch landed, some tools have regressed to start printing more output when building.  It looks like there are callers to applyPreprocessor() that aren't passing the last argument, which then defaults it to 0 which makes them verbose.

WebCore/css/makeprop.pl and WebCore/css/makevalues.pl started to use applyPreprocessor() after this patch.
Hmm, "the default is verbose" may be not good. We had better change the default.

[Evan Martin]

Another option is to remove the $beQuiet argument completely, and have the caller print somethign before calling applyPreprocessing.  I think it only prints one line and that line is printed before it does any real work.