Bug 52612

Summary:

Apply feature flags to user-agent style sheets

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Tools / Tests

Assignee:

Kent Tamura <tkent>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abecsi, buildbot, commit-queue, dbates, dglazkov, eric, evan, gustavo, hausmann, keishi, laszlo.gombos, morrita, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

52214

Bug Blocks:

29358, 51854

Attachments:

Description	Flags
Patch	none
Patch 2 (move preprocessor.pm to WebCore/bindings/scripts/)	none
Patch 3	none
Patch 4	none
Patch 5	none
Patch 6	none
Patch 7	none
Patch 8	none

Kent Tamura

Reported 2011-01-17 23:53:28 PST

I saw some demand for this feature in several bugs.

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Kent Tamura

Comment 1 2011-01-18 01:17:28 PST

Created attachment 79252 [details] Patch

WebKit Review Bot

Comment 2 2011-01-18 01:33:22 PST

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

Early Warning System Bot

Comment 3 2011-01-18 01:39:04 PST

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

WebKit Review Bot

Comment 4 2011-01-18 01:44:41 PST

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

Build Bot

Comment 5 2011-01-18 02:01:09 PST

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

Kent Tamura

Comment 6 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.

WebKit Review Bot

Comment 7 2011-01-18 02:29:23 PST

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

Kent Tamura

Comment 8 2011-01-18 16:52:04 PST

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

Kent Tamura

Comment 9 2011-01-18 18:00:28 PST

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

Build Bot

Comment 10 2011-01-18 18:34:34 PST

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

Kent Tamura

Comment 11 2011-01-18 18:41:52 PST

Created attachment 79381 [details] Patch 3 Fix Windows build

Kent Tamura

Comment 12 2011-01-18 23:20:09 PST

Created attachment 79395 [details] Patch 4 Passing feature flags on Chromium, GTK, CMake

Eric Seidel (no email)

Comment 13 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?

Kent Tamura

Comment 14 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.

Kent Tamura

Comment 15 2011-01-24 17:40:04 PST

Created attachment 79997 [details] Patch 5 Update ChangeLog

Eric Seidel (no email)

Comment 16 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?

Kent Tamura

Comment 17 2011-04-12 00:58:21 PDT

Created attachment 89167 [details] Patch 6 Add Google copyright, rebase

Early Warning System Bot

Comment 18 2011-04-12 01:06:41 PDT

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

Andras Becsi

Comment 19 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.

Kent Tamura

Comment 20 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!

Kent Tamura

Comment 21 2011-04-12 02:05:37 PDT

Created attachment 89174 [details] Patch 7 Fix Qt build

Hajime Morrita

Comment 22 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?

Kent Tamura

Comment 23 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.

Kent Tamura

Comment 24 2011-05-16 22:18:45 PDT

Created attachment 93738 [details] Patch 8 Rebase

Hajime Morrita

Comment 25 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.

Hajime Morrita

Comment 26 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+.

WebKit Commit Bot

Comment 27 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.

WebKit Commit Bot

Comment 28 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>

WebKit Commit Bot

Comment 29 2011-05-20 03:48:17 PDT

All reviewed patches have been landed. Closing bug.

WebKit Commit Bot

Comment 30 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.

Evan Martin

Comment 31 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.

Kent Tamura

Comment 32 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.

Evan Martin

Comment 33 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.

Note You need to log in before you can comment on or make changes to this bug.