Last Comment Bug 508942 - Make filters usable in defines
: Make filters usable in defines
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mike Hommey [:glandium]
: Gregory Szorc [:gps]
Depends on: 751865
Blocks: 508940
  Show dependency treegraph
Reported: 2009-08-06 18:12 PDT by Markus Stange [:mstange]
Modified: 2012-05-04 05:36 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (2.51 KB, patch)
2009-08-06 18:12 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
Use filters in #defines and #includes (7.28 KB, patch)
2012-04-23 06:02 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Use filters in #defines and #includes (6.18 KB, patch)
2012-04-23 07:58 PDT, Mike Hommey [:glandium]
ted: review+
l10n: review+
Details | Diff | Splinter Review

Description Markus Stange [:mstange] 2009-08-06 18:12:15 PDT
Created attachment 393091 [details] [diff] [review]

In bug 508940 I'd like to do things like this:

%filter substitution

%define SCOPE_BAR_BACKGROUND -moz-linear-gradient(top, bottom, from(@SCOPE_BAR_TOP_COLOR@), to(@SCOPE_BAR_BOTTOM_COLOR@)) repeat-x



findbar {
  background: @SCOPE_BAR_BACKGROUND@;

in order to minimize code repetition. The existing cascading mechanisms of CSS can't satisfy our needs, it seems :(

However, if I read the code in correctly, there's currently no way to reuse a #defined value when #defining a new value.

I'm attaching a patch that fixes that by applying filters to the #defined values at the time they're defined. Does that sound like a sane solution to the problem?
Comment 1 Axel Hecht [:Pike] 2009-08-06 23:33:58 PDT
I'm a bit concerned about regular values that have @ signs in them, like emails, contract id, @media rules. varsubst only matches non-whitespace, so this is probably OK in general, though.

Can you add some tests that make sure that we're not regressing some of those?

And this should get some testing, I guess we need to at least some fx builds with and without this patch and confirm that they're binary equal right now, same for probably fennec and thunderbird. Just running the build after the patch doesn't seem paranoid enough to me.

From a patch point-of-view, this looks OK, maybe more tests. I'd consider this a patch to need sr, though I don't have a suggestion on a good candidate.
Comment 2 Markus Stange [:mstange] 2009-08-17 21:29:54 PDT
Comment on attachment 393091 [details] [diff] [review]

Testing builds for binary equality will take me a while, so I'm canceling the review request until I've done that. Do you know what would be the fastest way to do it? I suppose I can't just do two try server pushes, since I suspect that tryserver somehow encodes the build time into the builds... is that correct?

I'm not so very concerned about this breaking anything. The "attemptSubstitution" filter doesn't seem to be used anywhere:
And the "substitution" filter will cause the build to fail if it encounters an undefined variable.
Comment 3 Mike Hommey [:glandium] 2012-04-23 06:02:49 PDT
Created attachment 617450 [details] [diff] [review]
Use filters in #defines and #includes

I'm testing this patch on Try, with the following additional change:

This effectively compares the output with the patch applied to the
output without the patch applied, and will fail the build when they
don't match.

TBPL link for that try:
Comment 4 Mike Hommey [:glandium] 2012-04-23 06:49:39 PDT
New attempt with more check glue (reading sys.stdin twice doesn't work)
Comment 5 Mike Hommey [:glandium] 2012-04-23 06:57:36 PDT
This is embarassing. Another try, with js/src/config in sync:
Comment 6 Mike Hommey [:glandium] 2012-04-23 07:44:06 PDT
... and avoid failing the testcase:
Comment 7 Mike Hommey [:glandium] 2012-04-23 07:58:13 PDT
Created attachment 617480 [details] [diff] [review]
Use filters in #defines and #includes

And yet another try (sorry for the noise). There was a problem with includesubst in, because doesn't enable the substitution filter.
Comment 8 Mike Hommey [:glandium] 2012-04-24 00:39:27 PDT
(In reply to Mike Hommey [:glandium] from comment #6)
> ... and avoid failing the testcase:

Finally all green.
Comment 9 Mike Hommey [:glandium] 2012-04-24 00:40:13 PDT
(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > ... and avoid failing the testcase:
> >
> >
> Finally all green.

Err wrong quote, was meant for comment #7.
Comment 10 Axel Hecht [:Pike] 2012-04-24 06:06:23 PDT
Comment on attachment 617480 [details] [diff] [review]
Use filters in #defines and #includes

Review of attachment 617480 [details] [diff] [review]:

Looks OK to me.

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