Bug 52612 - Apply feature flags to user-agent style sheets
Summary: Apply feature flags to user-agent style sheets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 52214
Blocks: 29358 51854
  Show dependency treegraph
 
Reported: 2011-01-17 23:53 PST by Kent Tamura
Modified: 2011-07-21 07:39 PDT (History)
17 users (show)

See Also:


Attachments
Patch (9.50 KB, patch)
2011-01-18 01:17 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (move preprocessor.pm to WebCore/bindings/scripts/) (12.94 KB, patch)
2011-01-18 18:00 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (13.57 KB, patch)
2011-01-18 18:41 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (22.06 KB, patch)
2011-01-18 23:20 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 5 (22.64 KB, patch)
2011-01-24 17:40 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 6 (22.95 KB, patch)
2011-04-12 00:58 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 7 (22.95 KB, patch)
2011-04-12 02:05 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 8 (21.00 KB, patch)
2011-05-16 22:18 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-01-17 23:53:28 PST
I saw some demand for this feature in several bugs.
Comment 1 Kent Tamura 2011-01-18 01:17:28 PST
Created attachment 79252 [details]
Patch
Comment 2 WebKit Review Bot 2011-01-18 01:33:22 PST
Attachment 79252 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7598194
Comment 3 Early Warning System Bot 2011-01-18 01:39:04 PST
Attachment 79252 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7507200
Comment 4 WebKit Review Bot 2011-01-18 01:44:41 PST
Attachment 79252 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7596157
Comment 5 Build Bot 2011-01-18 02:01:09 PST
Attachment 79252 [details] did not build on win:
Build output: http://queues.webkit.org/results/7499187
Comment 6 Kent Tamura 2011-01-18 02:01:44 PST
I know I need to update build files.  But I feel it might be irregular that WebCore depends on Tools/Scripts/webkitperl.
Comment 7 WebKit Review Bot 2011-01-18 02:29:23 PST
Attachment 79252 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7545192
Comment 8 Kent Tamura 2011-01-18 16:52:04 PST
I'll move preprocessor.pm to WebCore/binding/scripts/.  It would be simpler.
Comment 9 Kent Tamura 2011-01-18 18:00:28 PST
Created attachment 79373 [details]
Patch 2 (move preprocessor.pm to WebCore/bindings/scripts/)
Comment 10 Build Bot 2011-01-18 18:34:34 PST
Attachment 79373 [details] did not build on win:
Build output: http://queues.webkit.org/results/7536192
Comment 11 Kent Tamura 2011-01-18 18:41:52 PST
Created attachment 79381 [details]
Patch 3

Fix Windows build
Comment 12 Kent Tamura 2011-01-18 23:20:09 PST
Created attachment 79395 [details]
Patch 4

Passing feature flags on Chromium, GTK, CMake
Comment 13 Eric Seidel (no email) 2011-01-24 15:35:01 PST
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?
Comment 14 Kent Tamura 2011-01-24 17:39:19 PST
(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.
Comment 15 Kent Tamura 2011-01-24 17:40:04 PST
Created attachment 79997 [details]
Patch 5

Update ChangeLog
Comment 16 Eric Seidel (no email) 2011-04-06 10:22:30 PDT
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 <wildfox@kde.org>

Google too?
Comment 17 Kent Tamura 2011-04-12 00:58:21 PDT
Created attachment 89167 [details]
Patch 6

Add Google copyright, rebase
Comment 18 Early Warning System Bot 2011-04-12 01:06:41 PDT
Attachment 89167 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8391293
Comment 19 Andras Becsi 2011-04-12 01:36:52 PDT
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.
Comment 20 Kent Tamura 2011-04-12 02:04:13 PDT
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!
Comment 21 Kent Tamura 2011-04-12 02:05:37 PDT
Created attachment 89174 [details]
Patch 7

Fix Qt build
Comment 22 Hajime Morrita 2011-05-16 21:50:10 PDT
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?
Comment 23 Kent Tamura 2011-05-16 22:05:51 PDT
(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.
Comment 24 Kent Tamura 2011-05-16 22:18:45 PDT
Created attachment 93738 [details]
Patch 8

Rebase
Comment 25 Hajime Morrita 2011-05-16 22:23:00 PDT
(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.
Comment 26 Hajime Morrita 2011-05-20 01:37:24 PDT
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+.
Comment 27 WebKit Commit Bot 2011-05-20 03:46:21 PDT
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: cshu@webkit.org and kling@webkit.org)
http/tests/websocket/tests/multiple-connections.html bug 53825 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 28 WebKit Commit Bot 2011-05-20 03:48:08 PDT
Comment on attachment 93738 [details]
Patch 8

Clearing flags on attachment: 93738

Committed r86936: <http://trac.webkit.org/changeset/86936>
Comment 29 WebKit Commit Bot 2011-05-20 03:48:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 WebKit Commit Bot 2011-05-20 05:50:22 PDT
The commit-queue encountered the following flaky tests while processing attachment 93738 [details]:

java/lc3/JavaObject/JavaObjectToByte-006.html bug 60333 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 31 Evan Martin 2011-07-20 10:32:02 PDT
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.
Comment 32 Kent Tamura 2011-07-20 18:30:26 PDT
(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.
Comment 33 Evan Martin 2011-07-21 07:39:04 PDT
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.