Closed Bug 773296 Opened 12 years ago Closed 10 years ago

implement CSS3 variables

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox29 --- disabled
relnote-firefox --- 29+

People

(Reporter: ebassi, Assigned: heycam)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

(Keywords: dev-doc-needed, feature, Whiteboard: [enabled by default in 31][DocArea=CSS][qa-])

Attachments

(30 files, 30 obsolete files)

10.99 KB, patch
dbaron
: review-
fantasai.bugs
: review-
Details | Diff | Splinter Review
4.86 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
47.44 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.39 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
15.61 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.19 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.89 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
9.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.01 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
15.14 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.71 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.98 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.65 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.36 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
4.31 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
32.91 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
21.78 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
20.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.07 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.12 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.37 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
21.67 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.03 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
9.08 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
69.00 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.16 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
190.31 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
13.99 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.93 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
CSS Variables Module Level 1

latest draft: http://www.w3.org/TR/css-variables/

the naive implementation would require checking for tokens starting with the 'var-' literal and storing the literal value in a hash table, using the variable name as the key.

every time a value is expressed as a '$variable-name' or as a 'var(variable-name)', a preliminary pass would check for the existence of the 'variable-name' key inside the hash table and return the literal value - thus leaving the existing CSS parser machinery to determine the validity of the value; if the key is not found, the whole declaration is marked as invalid.
latest editor's draft is usually more relevant:
http://dev.w3.org/csswg/css-variables/
the latest draft kills the chances of a naive implementation - considering that it enforces cascading rules, as well as the ability to get to the value of the variable on the parent element.
I'm not sure what you mean by that.  Could you explain?
Blocks: unprefix
notes from the discussion with dbaron, roc, and bz:

 * the variable value should be stored as a token stream during parsing;
 * the token stream should be reinjected into the parser when resolving the variable;
 * the CSSOM part of the spec should be ignored until it is more formally defined
   * does not block the implementation;
   * should there be a way to programmatically set variables from JS?
 * support for '$' can be skipped;
 * the implementation should not be prefixed/should land un-prefixed;

there are still interesting issues re: invalidation of the style sheet in case new classes that only define variables are added/removed and should cause a re-evaluation of the whole style sheet; I think support for scoped style sheets has the same class of issues (see bug 508725).
Also, might be better to store the token stream as a string (which can always be re-converted to a token stream); see the code heycam is adding in bug 649740.
Assignee: nobody → ebassi
as a comparison point: Chromium/WebKit implement the working draft from April 2012, and they support only the (prefixed) -webkit-var-* and -webkit-var() notations for defining and using a variable, respectively.
Bug 442864 seems like an earlier duplicate of this.
Keywords: dev-doc-needed
OS: Linux → All
Hardware: x86_64 → All
I was hoping to see preliminary code & test patches posted today. Can you summarize he issues that you're still seeing in your local builds/tests?
Attached patch Initial test suite (obsolete) — Splinter Review
this is the initial test suite for CSS3 Variables.

currently, it only checks whether the substitution produces a correct result, as the access from JavaScript is not defined.

I used the CSS-WG reftest as the template.
adds a new CSS unit for nsCSSValue, used to store the contents of a 'var()' token.
simple function that is used to parse a 'var()' function token into a nsCSSValue.
caches the preference value, given that we're going to query it for every property later on.
variable declarations are parsed, stored in a temporary location until the declaration block is done, and then flattened into a PairList nsCSSValue associated to the eCSSProperty_variable id.

after looking at various approaches, this is the one that does not punch through too many layers of abstraction; I'm not sure it's the most correct, but given the constraints of:

  - variables can be declared in any style declaration block;
  - variables can be declared interspersed with property declaration;
  - selectors are only known when creating a StyleRule, but are not known from within the declaration block, so I cannot just add the variable declaration to an ancillary hashmap and attach the selectors list to that from within the declaration block

I'm a bit stumped after this, though: changing css::Declaration to allow getting a variable cascades into a bunch of changes in dom/* that I'm not entirely familiar with.
Attachment #668459 - Flags: feedback?(dbaron)
hooks attachment 668450 [details] [diff] [review] and attachment 668451 [details] [diff] [review] to parse 'var()' into nsCSSValues. it's missing handling the fallback value in case the variable is invalid.
the patchset is incomplete, mostly because after doing an implementation that just punched a hole through so many layers of abstractions in order to store a hashmap of (name,tokens) pairs, I realized that I got stuff mostly wrong. well, "first one written to be thrown away", so no harm done, and I got to learn how the pieces fit together.
[ugh, sorry, haven't finished the comment]

the lookup part is missing, though it's mostly a matter of matching the name stored inside the nsCSSValue token when computing the style; the actual issue is storage of the variables, so feedback is welcome.
Attachment #668450 - Flags: feedback?(dbaron)
Attachment #668451 - Flags: feedback?(dbaron)
Attachment #668454 - Flags: feedback?(dbaron)
Attachment #668463 - Flags: feedback?(dbaron)
Attachment #668450 - Attachment is patch: true
Attachment #668451 - Attachment is patch: true
Comment on attachment 668454 [details] [diff] [review]
patch 3: add nsLayoutUtils::AreCSSVariablesEnabled()

># HG changeset patch
># User Emmanuele Bassi <ebassi@mozilla.com>
>Bug 773296 - implement CSS3 variables/3: Add nsLayoutUtils::AreCSSVariablesEnabled()
>
>
>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>index d7b646b..ce01890 100644
>--- a/layout/base/nsLayoutUtils.cpp
>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -229,16 +229,30 @@ nsLayoutUtils::GPUImageScalingEnabled()
>   if (!sGPUImageScalingPrefInitialised) {
>     sGPUImageScalingPrefInitialised = true;
>     sGPUImageScalingEnabled = mozilla::Preferences::GetBool("layout.gpu-image-scaling.enabled", false);
>   }
> 
>   return sGPUImageScalingEnabled;
> }
> 
>+bool
>+nsLayoutUtils::AreCSSVariablesEnabled()
>+{
>+  static bool sCSSVariablesEnabled;
>+  static bool sCSSVariablesEnabledInitialised = false;
>+
>+  if (!sCSSVariablesEnabledInitialised) {
>+    sCSSVariablesEnabledInitialised = true;
>+    sCSSVariablesEnabled = mozilla::Preferences::GetBool("layout.css.variables.enabled", false);
>+  }
>+
>+  return sCSSVariablesEnabled;
>+}
>+
> void
> nsLayoutUtils::UnionChildOverflow(nsIFrame* aFrame,
>                                   nsOverflowAreas& aOverflowAreas)
> {
>   // Iterate over all children except pop-ups.
>   const nsIFrame::ChildListIDs skip(nsIFrame::kPopupList |
>                                     nsIFrame::kSelectPopupList);
>   for (nsIFrame::ChildListIterator childLists(aFrame);
>diff --git a/layout/base/nsLayoutUtils.h b/layout/base/nsLayoutUtils.h
>index 7e7e59b..b3679d0 100644
>--- a/layout/base/nsLayoutUtils.h
>+++ b/layout/base/nsLayoutUtils.h
>@@ -1549,16 +1549,21 @@ public:
> 
>   /**
>    * Checks whether we want to use the GPU to scale images when
>    * possible.
>    */
>   static bool GPUImageScalingEnabled();
> 
>   /**
>+   * Checks whether the CSS variables are currently enabled.
>+   */
>+  static bool AreCSSVariablesEnabled();
>+
>+  /**
>    * Unions the overflow areas of all non-popup children of aFrame with
>    * aOverflowAreas.
>    */
>   static void UnionChildOverflow(nsIFrame* aFrame,
>                                  nsOverflowAreas& aOverflowAreas);
> 
>   /**
>    * Return whether this is a frame whose width is used when computing
Attachment #668454 - Attachment is patch: true
Attachment #668459 - Attachment is patch: true
Attachment #668463 - Attachment is patch: true
Attachment #668459 - Flags: feedback?(bzbarsky)
Comment on attachment 668459 [details] [diff] [review]
patch 4: parse and store var-* declarations

"VariableDeclarationIdetifier" is missing an "n".

Would it make sense to move the check for variables to inside the eCSSProperty_UNKNOWN case?  That would keep it off the parsing fast path.

ParseVariableDeclaration's argument should be of type "const nsString&", I think.

Should EOF during parsing of a variable's value really be fatal?  Seems like this:

  <div style="var-x: foo">

should be ok, no?

I believe the current code in ParseVariableDeclaration will get this case wrong:

  div {
    var-x: ;
    color: green;
  }

and in particular it will get the ';' token, then call ExpectEndProperty(), which will test false, then proceed to make "; color: green" be the variable value.

ExpectEndProperty() already ungets any tokens it gets, so I don't think you need to UngetToken() after calling it.

I would call the accessors on nsCSSVariable "Name" and "Value" respectively.  And maybe call the class mozilla::css::Variable?
Attachment #668459 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 668450 [details] [diff] [review]
patch 1: add nsCSSValue for representing var()

Seems reasonable, but:

 * I think you're going to need some more code in nsCSSValue::AppendToString

 * you want an operator!= where you have an operator==

 * I'm not crazy about extending the overuse of namespaces here; we should probably get rid of mozilla::css and use names like mozilla::CSSVariableValue in the long term.  But I guess it's probably better to be consistent with the other things here.

 * the operator== needs to look at mDefault


Was there something specific you wanted feedback on?  (feedback? is usually associated with a particular question; review? is generally for the whole patch)
Attachment #668450 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 664489 [details] [diff] [review]
Initial test suite

You should add a test item to property_database.js for an example variable.  This will get exercised by a whole bunch of tests in interesting ways.
Comment on attachment 668451 [details] [diff] [review]
patch 2: add nsCSSParser::ParseVar()

While this code looks mostly ok on its own (except that I'd put the error-handling in early returns and leave the main unindented flow of the function for the normal case), it's hard for me to see how it fits in to a plan for implementing variables.  var() is essentially going to need to be handled on a different level from normal parsing, in terms of storing a token stream or a string each time you parse the value of a property.

I think you're going to need to start from CSSParserImpl::ParseProperty(nsCSSProperty) and work from there.  My suspicion is that when you do, you'll find the code you end up needing doesn't look like this.  I could be wrong, though.
Attachment #668451 - Flags: feedback?(dbaron) → feedback-
Comment on attachment 668454 [details] [diff] [review]
patch 3: add nsLayoutUtils::AreCSSVariablesEnabled()

At the very least you should copy one of the functions that uses Preferences::AddBoolVarCache (e.g., Are3DTransformsEnabled or IsAnimationLoggingEnabled).

However, I think lazily initializing all of these things that we're guaranteed to need during startup is probably silly.  (nsContentUtils::Init doesn't, and thus has inline getters for the preference values, which is probably superior.)  It might be worth simplifying all of these to work that way instead.
Attachment #668454 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 668463 [details] [diff] [review]
patch 5: parse (partial) var() notation

same comments as patch 2
Attachment #668463 - Flags: feedback?(dbaron) → feedback-
Comment on attachment 668459 [details] [diff] [review]
patch 4: parse and store var-* declarations

I agree with bzbarsky's comments above.

Also, in ParseVariableDeclaration, you're going to need to do more to
make sure you're matching the grammar of what's allowed in variables. 
You need to check that {}, (), and [] match, and you need to terminate
if you hit EOF, semicolon at top level, <!-- at top level, or --> at top
level (where "at top level" means not inside (), {}, or []).  (I tend to
think we should change the spec for the <!-- and --> token thing,
though.)

It's hard for me to tell how this particular storage mechanism is going
to work out as you propagate these changes to other parts of the system;
I wouldn't feel too tied to it if you think it needs to change.

(And, in the end, we will need to rewrite a bunch of declaration storage
anyway; see bug 649145.)
Attachment #668459 - Flags: feedback?(dbaron) → feedback+
updated test suite: differentiate between titles and meta.content
Attachment #664489 - Attachment is obsolete: true
Attachment #672815 - Flags: review?(fantasai.bugs)
Attachment #672815 - Flags: review?(dbaron)
Attachment #668450 - Attachment is obsolete: true
Attachment #668451 - Attachment is obsolete: true
Attachment #668454 - Attachment is obsolete: true
Attachment #668459 - Attachment is obsolete: true
Attachment #668463 - Attachment is obsolete: true
Attachment #672817 - Flags: review?(dbaron)
Attachment #672818 - Flags: review?(dbaron)
Attachment #672819 - Flags: review?(dbaron)
Attached patch parse var() into a nsCSSValue (obsolete) — Splinter Review
Attachment #672820 - Flags: review?(dbaron)
Comment on attachment 672819 [details] [diff] [review]
add nsCSSValue for representing var()

See comment 22; I don't think this is the right approach.
Attachment #672819 - Flags: review?(dbaron) → review-
Comment on attachment 672820 [details] [diff] [review]
parse var() into a nsCSSValue

See comment 22; I don't think this is the right approach.
Attachment #672820 - Flags: review?(dbaron) → review-
Comment on attachment 672818 [details] [diff] [review]
add layout.css.variables.enabled preference

>+bool
>+nsLayoutUtils::AreVariablesEnabled()
>+{
>+  return sAreVariablesEnabled;
>+}

This is probably better off inline in the header file.

r=dbaron with that
Attachment #672818 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #33)
> Comment on attachment 672818 [details] [diff] [review]
> add layout.css.variables.enabled preference
> 
> >+bool
> >+nsLayoutUtils::AreVariablesEnabled()
> >+{
> >+  return sAreVariablesEnabled;
> >+}
> 
> This is probably better off inline in the header file.
> 
> r=dbaron with that

fixed locally, but does that apply to attachment 672817 [details] [diff] [review] as well - i.e. should all boolean getters be moved inline to the header?
(In reply to David Baron [:dbaron] from comment #32)
> Comment on attachment 672820 [details] [diff] [review]
> parse var() into a nsCSSValue
> 
> See comment 22; I don't think this is the right approach.

I can see where you're going to with the ParseProperty() route and the variable declaration, but I'm more fuzzy about the var() usage. care to explain what your approach would be for parsing something like:

  margin: 10px var(right-margin, 2em) 20px var(left-margin, 2em);

for instance, this would go down to ParseValueList(), which would then reach ParseVariant() for each value. one way would be to start recording every single token, and if a 'var()' is found we just store everything into a special nsCSSValue - but that would mean going over every property twice.
(In reply to Emmanuele Bassi (:ebassi) from comment #35)
> (In reply to David Baron [:dbaron] from comment #32)
> > Comment on attachment 672820 [details] [diff] [review]
> > parse var() into a nsCSSValue
> > 
> > See comment 22; I don't think this is the right approach.
> 
> I can see where you're going to with the ParseProperty() route and the
> variable declaration, but I'm more fuzzy about the var() usage. care to
> explain what your approach would be for parsing something like:
> 
>   margin: 10px var(right-margin, 2em) 20px var(left-margin, 2em);
> 
> for instance, this would go down to ParseValueList(), which would then reach
> ParseVariant() for each value. one way would be to start recording every
> single token, and if a 'var()' is found we just store everything into a
> special nsCSSValue - but that would mean going over every property twice.

So the basic issue is that you need to invoke the parsing function for the 'margin' property *after* you've done the variable substitution.  This is important because authors are going to want to do both:
  html { var-header-indent: -2em }
  h1 { margin: 1em 0 0.5em var(header-indent); }
and to do:
  html { var-picture-margin: 1em } /* or 1em 2em */
  .gallery img { margin: var(picture-margin); }
So you can't assign a variable substitution to a particular value within the parsed data when you see the var() because you don't know how many values it's going to have.  (For example, with text-shadow, it might be a color, it might be a full shadow, or it might be a list of shadows.)

(I realize now that this is actually harder for shorthands than I'd thought.)

So, instead, you need to change property parsing so that it takes a different path when there's a var(), and stores the string to be reparsed later when the variable values are known.
Comment on attachment 672817 [details] [diff] [review]
initialize cached preferences in nsLayoutUtils::Initialize()

>@@ -4697,6 +4651,18 @@
>                                         "font.size.inflation.lineThreshold");
>   mozilla::Preferences::AddIntVarCache(&sFontSizeInflationMappingIntercept,
>                                        "font.size.inflation.mappingIntercept");
>+  mozilla::Preferences::AddBoolVarCache(&sAre3DTransformsEnabled,
>+                                        "layout.3d-transforms.enabled");
>+  mozilla::Preferences::AddBoolVarCache(&sAreOpacityAnimationsEnabled,
>+                                        "layers.offmainthreadcomposition.animate-opacity");
>+  mozilla::Preferences::AddBoolVarCache(&sAreTransformAnimationsEnabled,
>+                                        "layers.offmainthreadcomposition.animate-transform");
>+  mozilla::Preferences::AddBoolVarCache(&sIsAnimationLoggingEnabled,
>+                                        "layers.offmainthreadcomposition.log-animations");
>+  sUseBackgroundNearestFilteringEnabled =
>+    mozilla::Preferences::GetBool("gfx.filter.nearest.force-enabled", false);
>+  sGPUImageScalingEnabled =
>+    mozilla::Preferences::GetBool("layout.gpu-image-scaling.enabled", false);

These last two might be a problem.  Not using AddBoolVarCache might mean that we end up getting an incorrect result because we haven't actually initialized the profile by the first call.  I think these should also use AddBoolVarCache, though you should probably also ask the people who added them if the code in question is ok with them potentially changing over the lifetime of the application.

(I think we still have profile manager UI in some applications, though I might be wrong.)



Also, I think the functions whose contents are just "return <boolean>" should move to the header file so they get inlined.

r=dbaron with that
Attachment #672817 - Flags: review?(dbaron) → review+
Comment on attachment 672815 [details] [diff] [review]
updated test suite

>+
>+# Variables Level 1
>+skip-if(!prefs.getBoolPref("layout.css.variables.enabled")) include variables3/reftest.list

I tend to think you probaly want to call the directory variables rather than variables3.  But if fantasai disagrees, she should win.

>diff --git a/layout/reftests/w3c-css/submitted/variables3/css-variables-002.xht b/layout/reftests/w3c-css/submitted/variables3/css-variables-002.xht
>new file mode 100644
>index 0000000..54e55de
>--- /dev/null
>+++ b/layout/reftests/w3c-css/submitted/variables3/css-variables-002.xht
>@@ -0,0 +1,17 @@
>+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
>+<html xmlns="http://www.w3.org/1999/xhtml">
>+ <head>
>+  <title>CSS Test: Unknown variable</title>
>+  <link rel="author" title="Emmanuele Bassi" href="mailto:ebassi@gmail.com" />
>+  <link rel="help" href="http://www.w3.org/TR/css-variables/#using-invalid-variables" />
>+  <meta name="flags" content="invalid" />
>+  <meta name="assert" content="CSS Variables with invalid or unknown values should be ignored and the inherited or default value should be used instead"/>
>+  <link rel="match" href="variable/pass.xht" />
>+  <style type="text/css"><![CDATA[
>+    * { var-bg-color: rainbow; background-color: green; }}
>+    html { background-color: var(bg-color); }

So, first of all, tests 002 and 003 are identical except that 003 fixes the }} to be } as it should be.

Second, this test is really quite confusing given how it relies on the body background propagation rules.  It should at the very least have a comment saying that:
 (1) if the variables behavior it's testing is implemented correctly, then html will have background:transparent, body will have background: green, and the body's background will get propagated to the canvas
 (2) if the variables behavior is implemented incorrectly, then presumably the body background will no longer be propagated to the canvas.

You really want something that tests more directly that an invalid value in a variable falls back to the initial value of the property.  For example, you could do this with:
  <div id="outer"><div id="inner">Hello</div></div>
  #outer { background: green }
  #inner { var-bg: rainbow; background-color: red; background-color: var(bg); } /* transparent is the initial value */


In fact, I think given:
 * the complexity of html/body/canvas background propagation
 * the atypicality of setting * { background-color: green } (and the confusion that can result from backgrounds covering each other)
I think you should probably pick a different way to write pretty much all of these tests.  Understanding what each one is actually testing requires understanding too many things about how backgrounds work, especially on the root element and the body element.

>diff --git a/layout/reftests/w3c-css/submitted/variables3/variable/pass.xht b/layout/reftests/w3c-css/submitted/variables3/variable/pass.xht

This should be in a support/ subdirectory rather than a variable/ subdirectory.
Attachment #672815 - Flags: review?(dbaron) → review-
Attachment #672819 - Attachment is obsolete: true
Attachment #672820 - Attachment is obsolete: true
Attached file Store property values using var() (obsolete) —
this is the approach that dbaron and I discussed.

inside ParseProperty() we slurp the token stream from |mScanner|, and if it contains 'var()' we just save the it as a string so that we can later expand the variables when computing the style. if there is no variable to substitute we just roll back the scanner's |mOffset| and continue as usual.

I think we'll need a new eCSSUnit value, something like eCSSUnit_Variable, so that we can tell the nsRuleNode later on that the contents of the nsCSSValue have to be expanded and parsed again.
Comment on attachment 672815 [details] [diff] [review]
updated test suite

> I tend to think you probably want to call the directory variables rather than variables3.
> But if fantasai disagrees, she should win.

However, fantasai agrees. :) Also, I'd remove the 'css-' from the front; all the tests in this test suite are CSS tests.

Wrt tests, I don't understand why you're using the universal selector everywhere. It seems excessive to do so, and it hides the fact that variables inherit.

css-variables-001.xht
> "A single variable definition for all elements and its usage for the html element."

This isn't really asserting anything. What's the test trying to prove?

css-variables-002.xht
As dbaron points out, this should be discarded in favor of css-variables-003.xht

css-variables-004.xht
> "Undefined CSS variables should be ignored, and use the parent's value for the property
> instead."

I have no idea what you mean by "use the parent's value for the property". It sounds wrong.
I also don't see how this test can possibly fail the way it's written.
Also the 'var-bg-color: rainbow;' is extraneous and should be removed.

css-variables-005
> "Unknown CSS Variables should be ignored, and if a default value is defined then the
> default should be used."

This asserts multiple things, only one of which is covered here. You should assert
"Test checks that if an unknown variable is referenced and a default value is given, the given default is used."

css-variables-006's assertion is similarly over-broad:
> Unknown CSS Variables should be ignored, and if a default value is defined then the
> default should be used; if the default is invalid, the inherited or default value
> should be used instead.

"Test checks that if an unknown variable is referenced, and an invalid default is given, the property's initial value is used instead."

You need two tests here to cover what I think was the original intent of your test: one for non-inherited properties (which use the initial value) and one for inherited properties (which use the inherited value).

css-variables-007
> +  <meta name="assert" content="If a variable property is declared multiple times,
>          the standard cascade rules help resolve it. Variables always draw from the
>          computed value of the associated variable property on the same element."/>
> +  <link rel="match" href="variable/pass.xht" />
> +  <style type="text/css"><![CDATA[
> +    html { var-bg-color: green; }
> +    p { var-bg-color: red; }
> +    * { background-color: var(bg-color); }
> +]]></style>

The coding of this test is completely wrong given what you're trying to prove here.
The cascade is not operating at all; the red is being filtered out by selectors.
(Side-note to dbaron: this kind of error is why we have asserts in the template. :)

You're missing tests for
  - cascading of variable declarations (specificity and order checks should be sufficient)
  - inheritance of variable values through the document tree
  - negative scoping of variables: that they are not in-scope outside their actual scope
  - falling back to initial vs. inherit when there's an error, depending on property type
  - referencing a variable as a single component in a multi-component property,
    with and without fallback, and in various combinations of valid and invalid
  - referencing a variable as a single component in a multi-component shorthand,
    with and without fallback, and in various combinations of valid and invalid
  - variable computation: whether e.g. lengths are computed when declared or when referenced
  - syntax of variable names: non-ASCII characters, special ASCII characters, escaping

I would recommend using a testing method that allows having multiple tests in one file. Things like syntax and parsing, or cascading, are more efficient to test that way and equally clear. See e.g. http://www.w3.org/Style/CSS/Test/CSS3/Namespace/current/syntax-005.xml for an example. You can use properties like background, color, and borders to create clear and mostly self-contained tests; but as dbaron said, don't use background on HTML or BODY unless you're specifically testing the interactions there.
Attachment #672815 - Flags: review?(fantasai.bugs) → review-
Attachment #677071 - Attachment is obsolete: true
Attachment #683524 - Flags: review?(dbaron)
Attachment #677074 - Attachment is obsolete: true
Attachment #683526 - Flags: review?(dbaron)
Comment on attachment 683524 [details] [diff] [review]
Add offset saving to nsCSSScanner

>+  // Stores the current position in the input stream on a stack.
>+  void SaveOffset();
>+
>+  // Restores the current position in the input stream from the

current -> saved

>+  // stack.
>+  void RestoreOffset();


>+  InfallibleTArray<uint32_t> mOffsetStack;


This API seems likely to lead to mOffsetStack growing indefinitely if there are offsets we don't go back to.

I'd prefer that:
 * there be an assertion in ~nsCSSScanner that mOffsetStack is empty
 * if there were cases where you weren't restoring the offset (which I certainly hope there were), you add a DiscardOffset method to throw away a saved offset that you realize you don't need
with the idea that any SaveOffset is paired with either a RestoreOffset or a DiscardOffset.

r=dbaron with that
Attachment #683524 - Flags: review?(dbaron) → review+
Comment on attachment 683525 [details] [diff] [review]
Add eCSSUnit_Token for storing token streams inside nsCSSValue

>+  else if (eCSSUnit_Token == unit) {
>+    nsAutoString tmpStr;
>+    GetStringValue(tmpStr);
>+    nsStyleUtil::AppendEscapedCSSIdent(tmpStr, aResult);

This should just do aResult.Append(tmpStr), since you're storing raw data from the input stream, and thus you don't want to re-escape it.

r=dbaron with that
Attachment #683525 - Flags: review?(dbaron) → review+
Comment on attachment 683526 [details] [diff] [review]
Store properties using var() as token streams

> CSSParserImpl::ParseVariableDeclaration(const nsString& aName)

The changes to this function go in patch 2, not patch 4.  (They're compilation error fixes to errors introduced in patch 2.)
Comment on attachment 683526 [details] [diff] [review]
Store properties using var() as token streams

...continued from comment 50

I guess I'm afraid that the performance of this approach isn't going to
be acceptable, since it imposes a good bit of overhead
(double-tokenization) on all CSS property parses.

It looks usable as a foundation for testing other parts of the patch,
though, so fixing that isn't necessarily urgent, but it does need to
happen before landing.  And you'll still use a bunch of the code (see 3b
below).

I think there's a relatively simple way to fix that, though.  (It took
me a bit of time to remember this, though.)

In particular, instead of doing variable parsing first, we do variable
parsing last, by doing the following:

 (1) we *always* maintain a boolean flag in the tokenizer that gets set
 to true when we see a "var(" function token

 (2) when we start a property parse:
     (a) set that boolean seen-var-function flag to false
     (b) remember the current position using much like the
     StartRecording concept here

 (3) when we finish a property parse the normal way:

     (a) if it succeeds, assert that we didn't see a "var(" function

     (b) if it fails, call SkipDeclaration, and then check if we saw a
         "var(" token.

         If we *did*, then excute code much like what's in this patch to
         check and (if valid) store the syntax for the variable value.

         I realize, further, that the spec doesn't really say anything
         about what sort of checking we should do on a declaration that
         contains var() functions at parse time.  It needs to; that's an
         omission in the spec that I should have noticed earlier.

         Your approach isn't quite right; at the very least it's going
         to need to:
           - handle (), {}, and [] matching correctly (which includes
             matching function tokens with ')' tokens, since the
             function token includes the '('), which means only calling
             CheckEndProperty() when not nested inside such a structure
           - have better error handling for the error cases (in particular,
             it's going to need to do (), {}, and [] matching correctly
             rather than just returning straightaway).

Sorry for taking so long to get to this; bug 780692 was higher priority
given B2G pressure right now, and I was also having trouble putting my
finger on exactly what you ought to do instead (which I've now figured
out).

So one of us, probably me, needs to post to www-style explaining the
things that we need to do to get the spec straightened out.  You should
probably wait to clean up the details of the validation bits until that
happens, though there's nothing preventing you from inverting the
processing (normal path first, variables second) any time.
Attachment #683526 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] from comment #51)
> So one of us, probably me, needs to post to www-style explaining the
> things that we need to do to get the spec straightened out.  You should
> probably wait to clean up the details of the validation bits until that
> happens, though there's nothing preventing you from inverting the
> processing (normal path first, variables second) any time.

My proposal is http://lists.w3.org/Archives/Public/www-style/2012Dec/0092.html ; we'll see what others think of it.
Also, in general, when you start having to make up behavior like that, it's a sign that there's something missing in the spec.
(In reply to David Baron [:dbaron] from comment #51)
> Comment on attachment 683526 [details] [diff] [review]
> Store properties using var() as token streams
> 
> ...continued from comment 50
> 
> I guess I'm afraid that the performance of this approach isn't going to
> be acceptable, since it imposes a good bit of overhead
> (double-tokenization) on all CSS property parses.

yes, it's a very good point.

> It looks usable as a foundation for testing other parts of the patch,
> though, so fixing that isn't necessarily urgent, but it does need to
> happen before landing.  And you'll still use a bunch of the code (see 3b
> below).

I have also some performance-related issues with the variable resolution when computing the style; we have to build a map of the declarations that apply to a specific branch of the RuleNode tree, but I'm wary of the overhead imposed especially when dealing with allocating and freeing a bunch of data structures in a critical path; right now, WalkRuleTree() uses alloca() and nsRuleData to get around that; I think that the variable declaration map can be solved in a similar way.

> I think there's a relatively simple way to fix that, though.  (It took
> me a bit of time to remember this, though.)
> 
> In particular, instead of doing variable parsing first, we do variable
> parsing last, by doing the following:
> 
>  (1) we *always* maintain a boolean flag in the tokenizer that gets set
>  to true when we see a "var(" function token
> 
>  (2) when we start a property parse:
>      (a) set that boolean seen-var-function flag to false
>      (b) remember the current position using much like the
>      StartRecording concept here
> 
>  (3) when we finish a property parse the normal way:
> 
>      (a) if it succeeds, assert that we didn't see a "var(" function
> 
>      (b) if it fails, call SkipDeclaration, and then check if we saw a
>          "var(" token.
> 
>          If we *did*, then excute code much like what's in this patch to
>          check and (if valid) store the syntax for the variable value.

sounds good to me.

>          Your approach isn't quite right; at the very least it's going
>          to need to:
>            - handle (), {}, and [] matching correctly (which includes
>              matching function tokens with ')' tokens, since the
>              function token includes the '('), which means only calling
>              CheckEndProperty() when not nested inside such a structure

right.

>            - have better error handling for the error cases (in particular,
>              it's going to need to do (), {}, and [] matching correctly
>              rather than just returning straightaway).

okay.

> So one of us, probably me, needs to post to www-style explaining the
> things that we need to do to get the spec straightened out.  You should
> probably wait to clean up the details of the validation bits until that
> happens, though there's nothing preventing you from inverting the
> processing (normal path first, variables second) any time.

thanks for taking care of the email: I'll keep an eye out on the mailing list.
Comment on attachment 685621 [details] [diff] [review]
Parse variable declarations and store them into css::Declaration using an ancillary hash table

So, as far as namespaces go... we've basically decided that the nested
mozilla::css namespace was a mistake and we shouldn't be doing more of
that.  namespace mozilla is fine, but inside that, things should just
have names.  (And at some point we should probably unroll the
mozilla::css stuff.)

So I'd drop "namespace css" and just add "CSS" to the beginning of both
class names defined here.  No colons needed.

I'd also call the header CSSVariables.h rather than CSSVar.h, just to be
less cryptic.


VariableDeclarationMap should have a TODO that we ought not to store the
name of the variable twice (once in the VariableDeclaration, and once in
the hashtable's key).  (It's a pain to do that with our current
hashtable templates, though.)

VariableDeclarationMap should also have comments explaining the
ownership rules, since it depends on VariableDeclaration objects stored
elsewhere.

TODO: check callers


>+void
>+Declaration::AddVariableDeclaration(mozilla::css::VariableDeclaration *aDecl)
>+{
>+  if (!IsMutable()) {
>+    return;
>+  }

This shouldn't just return.  Instead you should call
AssertMutable().

TODO: check callers


In ParseVariableDeclaration, you need to honor the forward-compatible
parsing rules of CSS rather than calling CheckEndProperty() without
regard to matching braces, brackets, and parentheses.  You also need to
honor the other parsing rules in the spec, in particular, disallowing
<!-- and --> tokens (I *think*... unless the spec has changed) and maybe
one other thing.


>+  if (eCSSPropertyExtra_variable == propID) {
>+    nsDependentString varName(propertyName, 4); // remove 'var-'
>+    if (!ParseVariableDeclaration(aDeclaration, varName)) {
>+      REPORT_UNEXPECTED_P(PEUnknownProperty, propertyName);
>+      REPORT_UNEXPECTED(PEDeclDropped);
>+      OUTPUT_ERROR();
>+      return false;
>+    }
>+    return true;
>+  }
>   if (! ParseProperty(propID)) {
>     // XXX Much better to put stuff in the value parsers instead...
>     REPORT_UNEXPECTED_P(PEValueParsingError, propertyName);
>     REPORT_UNEXPECTED(PEDeclDropped);
>     OUTPUT_ERROR();

You should use the same REPORT_UNEXPECTED_P call as for a ParseProperty
failure.

>+  mScanner.StartRecording();

You should assert around the call to StartRecording that mHavePushBack
is false.  In fact, you should probably have an inline method on the
parser that wraps mScanner.StartRecording, and combines it with this
assertion.


>+  // Remove the trailing ';' if any
>+  if (variableValue.Length() > 0 &&
>+      variableValue[variableValue.Length() - 1] == ';') {
>+    variableValue.Truncate(variableValue.Length() - 1);
>+  }

You also need to do this for a possible } that triggered your test
that you're at the end.  But you can't trim all final }s since you'd
need to trim for:
  p {var-theme-color: blue}
but not trim for
  p {var-foo: {bar}; color: blue}
The CheckEndProperty changes that we made for @supports also mean that you
can't use CheckEndProperty here.  So you should just roll your own, and
preferably find a way to record the scanner offset *before* you do the
equivalent of the CheckEndProperty so you don't need to do any of this magic
anyway.

In terms of all aspects of property mutation, variable properties need
to behave like other properties, supporting things like
CSSStyleDeclaration.setProperty, removeProperty, getPropertyValue, etc.,
with the same semantics as other CSS properties.  (And with each
variable property treated as a separate property!)  Given the current
storage model you're using, those are all going to require special cases
to handle eCSSPropertyExtra_variable (and in many cases additional
parameters to pass the actual name given, since currently the property
is passed through only as an enum value, but we need to distinguish
between different variables).  The code as written, given your
modifications to LookupProperty, will for many of these cases cause
assertions, out-of-bounds reads/writes, or other crashes.
Attachment #685621 - Flags: review?(dbaron) → review-
these are the notes for the current (incomplete) implementation.

- the basic design for the variables support is split in three parts:
  1. parsing and storing the variable declaration inside a block;
  2. parsing and storing the properties containing a var() function token;
  3. resolving the variables when computing the style.

part 1: variable declarations can happen in any block. the current approach is to take the tokens that follow the 'var-name' and store them into a separate <variable name, tokens> map that is attached into the css::Declaration. a minor optimization would be to check whether the variable declaration contains a 'var()' token. this part is probably the most straightforward, given that nothing should happen except identifying the 'var-' fragment in the CSS property name; the nsCSSParser should still check if the token stream conforms to the CSS grammar.

part 2: this is a bit trickier, especially for performance. the current patch in the bug is a naïve implementation that just pre-processes the token stream and, if the 'var()' token is found, stores the whole stream inside a specially crafted nsCSSValue, along with the property id. the correct implementation, as dbaron outlined in his review, is to have a flag inside the CSS scanner so that, if the 'var()' token is found (and expected) we can check it in case the parser returns an error because of the 'var()' token, and store the whole property only on that case, instead of doing the work twice. I store the nsPropID of the property inside the nsCSSValue because we need it later on, when re-parsing the resolved token stream, given that there is no way to go from the property offsets into the nsRuleData to the CSS property id or name; it should be possible to extend the nsCSSProps::* functions to have a reverse mapping there.

part 3, which is currently incomplete: once we build the nsRuleData for a specific branch of the style tree we need to resolve the variables in order to have either a valid nsCSSValue for the Compute*Data methods, or to have the value just inherit from the parent node. while walking the nsRuleNode tree, we check for the variable declarations stored inside the StyleRule, and merge them together. the resolution can be recursive, in case of variables defined to use other variables. the way I've written the code is by adding a new method to nsCSSParser that wraps ParseProperty(). the main issue is that once the buffer containing the tokens stream is owned by the nsCSSScanner it becomes harder to expand it to replace the 'var(name)' tokens with the payload of the variable, or in case of 'var(name, fallback value)' with the fallback value if the variable is invalid. this could be either solved by replacing tokens using string operations before parsing the property value, or by modifying nsCSSScanner with a push/pop API to store the tokenizer state and buffer as a stack.
Assignee: ebassi → nobody
Attachment #672818 - Attachment is obsolete: true
Attachment #685621 - Attachment is obsolete: true
Attachment #683524 - Attachment is obsolete: true
Attachment #683525 - Attachment is obsolete: true
Attachment #683526 - Attachment is obsolete: true
Attachment #704942 - Attachment is obsolete: true
For what it's worth -- I think that part of what's in part 3 -- figuring out the computed style of the variable properties -- probably ought to be its own part (living on either side of part 2).  And it probably should involve a new nsStyleVariables style struct that holds the computed values for the variable properties so that we don't need to compute them every time (or any time we're asked for the computed value of a variable property via getComputedStyle()).
Blocks: 864562
Status: NEW → ASSIGNED
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #55)
> In terms of all aspects of property mutation, variable properties need
> to behave like other properties, supporting things like
> CSSStyleDeclaration.setProperty, removeProperty, getPropertyValue, etc.,
> with the same semantics as other CSS properties.  (And with each
> variable property treated as a separate property!)  Given the current
> storage model you're using, those are all going to require special cases
> to handle eCSSPropertyExtra_variable (and in many cases additional
> parameters to pass the actual name given, since currently the property
> is passed through only as an enum value, but we need to distinguish
> between different variables).  The code as written, given your
> modifications to LookupProperty, will for many of these cases cause
> assertions, out-of-bounds reads/writes, or other crashes.

I was just thinking about this.  Another approach would be to dynamically allocate property IDs for variables.  This would mean that we can store them in the nsCSSExpandedDataBlock while parsing, we can copy them into a Declaration's nsCSSCompressedDataBlocks without special handling, although it probably makes sense to keep nsRuleData storing variables outside of the normal array of nsCSSValues.  We then shouldn't need to modify setProperty, removeProperty, etc.

I suppose we would then make nsCSSProps::PropertyCountInStruct(eStyleStruct_Variables), by not having the internal property.

Would it be a concern if this were a global mapping of variables names to IDs?  It'd be hard to know when items in that mapping could go away.  Could we store the mapping of variable names to property IDs per pres context or something, rather than globally?  If so, then many places where we call nsCSSProps::LookupProperty we'd need to be able to get to the pres context, and I don't think we always have that.

WDYT?
Flags: needinfo?(dbaron)
(In reply to Cameron McCormack (:heycam) from comment #65)
> I was just thinking about this.  Another approach would be to dynamically
> allocate property IDs for variables.  This would mean that we can store them
> in the nsCSSExpandedDataBlock while parsing, we can copy them into a
> Declaration's nsCSSCompressedDataBlocks without special handling, although
> it probably makes sense to keep nsRuleData storing variables outside of the
> normal array of nsCSSValues.  We then shouldn't need to modify setProperty,
> removeProperty, etc.
> 
> I suppose we would then make
> nsCSSProps::PropertyCountInStruct(eStyleStruct_Variables), by not having the
> internal property.
> 
> Would it be a concern if this were a global mapping of variables names to
> IDs?  It'd be hard to know when items in that mapping could go away.  Could
> we store the mapping of variable names to property IDs per pres context or
> something, rather than globally?  If so, then many places where we call
> nsCSSProps::LookupProperty we'd need to be able to get to the pres context,
> and I don't think we always have that.
> 
> WDYT?

Hmmm.  I have mixed feelings about this in that I'm not crazy about this sort of approach, but it does seem like it might simplify a bunch of things, so maybe it's the right thing to do.

I'd probably be inclined to keep the mapping per-presentation rather than keep a global mapping (and then have to worry about how what we're doing will continue working for e10s, etc.).  Though that has the disadvantage of having to pass the pres context or style set around in a bunch of places -- though I'm not really sure how many.

I'd also be inclined to keep the mapping on the style set rather than the pres context, just because that's the containing object for style sheets for the presentation.  It's 1:1 with pres contexts, but it just helps keep the code more organized (see also RestyleManager).

It's also worth thinking about how many special cases variables would need despite that unification.
Flags: needinfo?(dbaron)
Thanks for your thoughts on that approach.  In the meantime, I continued with having variables stored separately on Declaration, nsRuleData, etc. and it has ended up not being too bad.  So I think for now I will stick with that.
Depends on: 909170
I'll be uploading patches soon (in the next couple of days; still some
edge cases to iron out and more tests to write), but I want to give an
overview of my patch queue in advance.  Up to date WIP patches still at
http://mcc.id.au/temp/bug-773296/.  I might still split some patches up
further.

Part 1: Add a preference for CSS variables.
  As it says.

Part 2: Parse CSS variable declarations and store them on Declaration
        objects.
  This defines a CSSVariableDeclarations class that holds a set of
  variable declarations.  This is at the specified value stage, so
  values can either be 'initial', 'inherit' or a token stream (which is
  what you normally have).  That variable declarations do interpret
  'initial' and 'inherit' is a recent change in the spec.  The
  variables are stored in a hash table.  Although it's a bit of a hack,
  I store 'initial' and 'inherit' using special string values that
  can't be valid token streams (I use "!" and ";").

  Declaration objects now can have two CSSVariableDeclarations objects
  on them, to store normal and !important variable declarations.  So
  that we keep preserving the order of declarations on the object,
  I inflated mOrder to store uint32_ts, where values from
  eCSSProperty_COUNT onwards represent custom properties.
  mVariableOrder stores the names of the variables corresponding to
  those entries in mOrder.

  The changes to nsCSSParser are straightforward.  Custom properties
  are parsed and checked for syntactic validity (e.g. "var(a,)" being
  invalid) and stored on the Declaration.

Part 3: Allow more than 27 style structs.
  This bumps up nsStyleContext::mBits to a uint64_t so that it can
  fit another style struct.  If we're going to need nsStyleContext
  to be bigger, it might be better to split mBits up into two
  uint32_ts: one for the flags and one for the style struct bits.

Part 4: Add style struct to store CSS variables.
  This adds an nsStyleVariables on which computed variable values will
  be stored.  We don't actually have any properties assigned to
  nsStyleVariables; eCSSPropertyExtra_Variables which we add in Part 2
  isn't a real property.

Part 5: Map variables on a Declaration to nsRuleData.
  This adds a CSSVariableDeclarations object to nsRuleData and adds
  a MapRuleInfoInto function to CSSVariableDeclarations so it can
  copy variable declarations into a nsRuleData's object.  We call
  that from Declaration::Map{Normal,Important}RuleInfoInto.

Part 6: Add a field to nsStyleVariables to store computed variable
        values.
  This defines a class CSSVariableValues which is used to store
  computed variable values.  We store them a bit differently from
  CSSVariableDeclarations -- here we have a hash table of variable
  names to integer IDs, and then an array of variables where the
  array index is the ID.  This is because later on we'll want a stable
  order for the variables to return from DOM APIs.

  We add a CSSVariableValues member to nsStyleVariables.

Part 7: Resolve and compute CSS variables.
  We add a new class CSSVariableResolver whose job is to take the
  inherited computed variables and the specified variable declarations
  and to perform cycle removal and resolution of the variables, storing
  the result in the CSSVariableValues object on an nsStyleVariables.
  We use CSSVariableResolver in nsRuleNode::ComputeVariablesData.

  The variable resolver does this:

    1. Asks the CSSVariableValues and CSSVariableDeclarations objects
       to add their variables to it.
    2. Calls in to a new nsCSSParser function
       EnumerateVariableReferences that informs the resolver which
       other variables a given variable references, and by doing so,
       builds a graph of variable dependencies.
    3. Removes variables involved in cyclic references using Tarjan's
       strongly connected component algorithm, setting those variables
       to have an invalid value.
    4. Calls in to a new nsCSSParser function ResolveVariableValue
       to resolve the remaining valid variables by substituting variable
       references.

  We could do step 2, recording the variable depenencies, as part of the
  original variable declaration parsing that happens in Part 2.

  I also add a couple of helper functions to nsCSSScanner to handle
  correctly stitching together token streams (by inserting "/**/").
  Currently this is done too eagerly; I'll have to make some changes
  to match the new Serialization section in css-syntax that requires
  "/**/" only between specific tokens.

Part 8: Give nsCSSScanner the ability to remember when it encounters a
        "var(" token.
  This is the first part of handling variable references in regular
  properties.  We have the scanner set a flag whenever it returns a
  "var(" token, so that when we come to the end of parsing a property,
  which failed, we know that it is because of a variable reference.

Part 9: Add a new eCSSUnit_TokenStream type for storing unparsed CSS
        values.
  This adds a new nsCSSValue unit type to represent an unparsed stream
  of CSS tokens as a specified value.  This is what properties that
  have a variable reference get as their specified value.  On the
  nsCSSValueTokenStream object that is used when mUnit ==
  eCSSUnit_TokenStream, we store two property IDs: first, the property
  ID for the longhand that this token stream value is the value for.
  We pass this back in to nsCSSParser::ParseProperty at computed
  value time, when we need to re-parse the property.  Second is
  the shorthand property ID, if we used a variable reference in
  a shorthand.  In such a case, we store the token stream value for
  each of the corresponding longhand properties.  This is because
  shorthands don't actually get any storage in an nsRuleData,
  and because any of the longhands might be overwritten by subsequent
  declarations, we need to keep the token stream somewhere.

Part 10: Give nsCSSScanner the ability to save/restore its current
         position.
  This adds functions to nsCSSScanner that lets us save the current input
  position (and corresponding information like line/column number) and
  be able to restore it later.  We'll use this when rewinding the scanner
  after we first encounter a property with a variable reference and
  go back to reparse it as a token stream.

Part 11: Record whether we are parsing an @supports condition.
  This stores on the nsCSSParser whether we are somewhere in the middle
  of parsing an @supports condition.  Because @supports condition parsing
  uses the scanner recording function (to save its conditionText), and
  variable-reference-containing values also need it, we can't do both
  at once.  Luckily, if we're parsing an @supports condition, we don't
  really need to store the token stream text; we only need to know if
  it was valid, and we don't need its actual value later.  So we use
  this flag later to see if we can skip turning on scanner recording
  while parsing variable-reference-containing values.

Part 12: Parse properties that use variable references and store them as
         eCSSUnit_TokenStream values.
  This adds functionality to nsCSSParser to recognise properties with
  variable references and store their recorded token stream as an
  eCSSUnit_TokenStream nsCSSValue.

Part 13: Resolve property values that have variable references at
         computed value time.
  This re-parses property values at computed value time if they had a
  specified value that was a token stream.  We add a function
  nsRuleNode::ResolveVariableReferences that looks at all the values
  in the nsRuleData and calls in to a new
  nsCSSParser::ParsePropertyWithVariableReferences function if they
  have a token stream value.

  We add a nsCSSExpandedDataBlock::MapRuleInfoInto function that will
  take the re-parsed property values and copy them back into the
  nsRuleData.  In here, we check to make sure we only overwrite values
  in the nsRuleData if they currently have a token stream value.  This
  lets us handle cases like:

    font: var(a);
    font-size: 16px;

  where we don't want the re-parsing of 'font' to overwrite the value
  of font-size we've got.

  There is some handling in this patch at the moment for when a
  re-parsed property gets an 'inherit' value.  I think this is now
  impossible since 'inherit' is interpreted at the specified value
  stage.  So I think I can remove this.

Part 14: Compare style structs even for the same rule node when
         variables have changed.
  This makes updates work correctly when variable values change.  Rather
  than handling nsStyleVariables with a DO_STRUCT_DIFFERENCE, we
  explicitly compare the two nsStyleVariables objects in
  nsStyleContext::CalcStyleDifference before looking at the other
  style structs.  This is because we need to force those other style
  structs to be compared if variable values are changing.
  nsStyleVariables::CalcDifference always returns 0, since the change
  in variable values themselves doesn't require any updates.

Part 15: Return variables from
         getComputedStyle().getProperty{,CSS}Value().
  As it says, this makes getPropertyValue and getPropertyCSSValue work
  with "var-blah" property names on computed style objects.

Part 16: Factor out style context getting from
         nsComputedDOMStyle::GetCSSPropertyValue.
  This just factors out some code from
  nsComputedDOMStyle::GetCSSPropertyValue that will be needed in the
  updated indexed getter for the object in part 17.

Part 17: Expose variables through the nsComputedDOMStyle indexed getter.
  This makes variables on a computed style object be exposed as indexed
  properties that appear after all the regular properties.

Part 18: Support variables in CSSStyleDelcaration methods.
  This adds support for "var-blah" properties in the CSSStyleDeclaration
  methods -- getPropertyValue, removeProperty, etc.

Part 19: Recognize variables in CSS.supports("var-blah", "whatever")
         calls.
   As it says.  This is where we check mInSupportsCondition from Part
   11 to make sure we don't turn the scanner recording on when parsing
   a variable value.

Part 20: Parse variable declarations in @supports conditions.
  Also as it says.
(In reply to Cameron McCormack (:heycam) from comment #69)
> Part 13: Resolve property values that have variable references at
>          computed value time.
...
>   There is some handling in this patch at the moment for when a
>   re-parsed property gets an 'inherit' value.  I think this is now
>   impossible since 'inherit' is interpreted at the specified value
>   stage.  So I think I can remove this.

Actually, this is still possible: when re-parsing of the property fails, and the property is an inherited property, it is treated as 'inherit'.  So we still do need to massage the rule detail after re-parsing a property if a property became 'inherit'.
A problem that has arisen while I've been writing tests is that I crash when using variable references in 'transform'.  For example with this document:

  <!DOCTYPE html>
  <div style="display: none; var-a: scaleX(1); transform: var(a);"></div>
  <script>
  var div = document.querySelector("div");
  getComputedStyle(div, "").getPropertyValue("transform");
  </script>

The problem is that nsStyleDisplay::mSpecifiedTransform is a weak pointer to an nsCSSValueList that is owned by the style rule.  Normally, with a specified value that has a transform list but no variable references, a strong reference to the nsCSSValueList will be held on to by the nsCSSValue in the rule's Declaration's data block.  With a variable reference, the nsCSSValue in the data block is an eCSSUnit_TokenStream instead.  In nsRuleNode::ResolveVariableReferences, called under nsRuleNode::WalkRuleTree, we replace the eCSSUnit_TokenStream nsCSSValue that is in the nsRuleData with the eCSSUnit_List after re-parsing it as a 'transform' property value.  When the nsRuleData goes out of scope, the reference to the nsCSSValueList inside the nsCSSValue is lost, and nsStyleDisplay::mSpecifiedTransform ends up pointing to freed memory.

Currently, nsCSSValueList_heap is an implementation detail of nsCSSValue.  Would it be OK to just move its reference counting methods up to nsCSSValue, get rid of nsCSSValueList_heap, and make nsStyleDisplay::mSpecifiedTransform an nsRefPtr?
Flags: needinfo?(dbaron)
I guess we'd need to keep separate structs for eCSSUnit_List and eCSSUnit_ListDep types, so maybe merging nsCSSValueList_heap up into nsCSSValue is not the right solution, and instead we should add a separate GetListHeapValue accessor that returns the value as an nsCSSValueList_heap, which consumers could then AddRef/Release or stick in an nsRefPtr.
Making the transform list be reference counted is reasonable.  (I suggested recently to someone else that they use a similar solution to what the transform list is doing, though, but I don't remember where.)  I guess the idea that computed style data can depend on things in the specified style data isn't going to stick.

Does it make sense to have a third list type in nsCSSValue that's for a reference-counted value list (i.e., a third value alongside eCSSUnit_List and eCSSUnit_ListDep), and switch transform to use that?
Flags: needinfo?(dbaron)
Depends on: 914072
(In reply to David Baron [:dbaron] (needinfo? me) (busy through Sept. 14) from comment #73)
> Does it make sense to have a third list type in nsCSSValue that's for a
> reference-counted value list (i.e., a third value alongside eCSSUnit_List
> and eCSSUnit_ListDep), and switch transform to use that?

I went with that approach in the end, rather than exposing nsCSSValueList_heap, and it worked well.
Attachment #704928 - Attachment is obsolete: true
Attachment #704937 - Attachment is obsolete: true
Attachment #704938 - Attachment is obsolete: true
Attachment #704939 - Attachment is obsolete: true
Attachment #704940 - Attachment is obsolete: true
Attachment #704941 - Attachment is obsolete: true
Attachment #704943 - Attachment is obsolete: true
Attachment #804209 - Attachment description: Part 1: Add a preference for CSS variables. r? → Part 1: Add a preference for CSS variables.
This defines a CSSVariableDeclarations class that holds a set of
variable declarations.  This is at the specified value stage, so values
can either be 'initial', 'inherit' or a token stream (which is what you
normally have).  The variables are stored in a hash table.  Although
it's a bit of a hack, we store 'initial' and 'inherit' using special
string values that can't be valid token streams (we use "!" and ";").

Declaration objects now can have two CSSVariableDeclarations objects
on them, to store normal and !important variable declarations.  So that
we keep preserving the order of declarations on the object, we inflate
mOrder to store uint32_ts, where values from eCSSProperty_COUNT onwards
represent custom properties.  mVariableOrder stores the names of the
variables corresponding to those entries in mOrder.

We also add a new nsCSSProperty value, eCSSPropertyExtra_variable, which
is used to represent any custom property name.
nsCSSProps::LookupProperty can return this value.

The changes to nsCSSParser are straightforward.  Custom properties
are parsed and checked for syntactic validity (e.g. "var(a,)" being
invalid) and stored on the Declaration.  We use nsCSSScanner's
recording ability to grab the unparsed CSS string corresponding to
the variable's value.
Attachment #804213 - Flags: review?(dbaron)
This bumps up nsStyleContext::mBits to a uint64_t so that it can fit
another style struct.  If we're going to need to keep at least 27 style
structs, it might be better to split mBits up into two uint32_ts: one
for the flags and one for the style struct bits.
Attachment #804214 - Flags: review?(dbaron)
This adds an nsStyleVariables on which computed variable values
will be stored.  We don't actually have any properties assigned to
nsStyleVariables; eCSSPropertyExtra_Variables which we added earlier
isn't a real property.  To avoid compiler errors for gVariableFlags
being a zero length array, we stick a dummy entry in there.

nsRuleNode::ComputeVariablesData does nothing for the moment.

nsStyleVariable nsChangeHint calculations always return 0, as later
we will compare the actual properties that reference variables to
see what changes are required for them.
Attachment #804215 - Flags: review?(dbaron)
This adds a CSSVariableDeclarations object to nsRuleData and adds a
MapRuleInfoInto function to CSSVariableDeclarations so the can copy
variable declarations into a nsRuleData's object.  We call that from
Declaration::Map{Normal,Important}RuleInfoInto.

We make HasImportantData return true if we have important variables
but no important non-custom properties on a declaration, since that
is used to determine whether we have a rule node for important
declarations.  This means MapImportantRuleInfoInto can no longer
assume that mImportantData is non-null.
Attachment #804216 - Flags: review?(dbaron)
This adds an enum to nsCSSScanner.h that represents the types of tokens
we need to consider when pasting together two adjacent tokens during
serialization or variable resolution.  For example with:

  var-a:orange;
  var-b:red;
  color:var(a)var(b);

we need to generate the string "orange/**/red" to parse for the value of
'color'.

https://dvcs.w3.org/hg/csswg/raw-file/3479cdefc59a/css-syntax/Overview.html#serialization
Attachment #804217 - Flags: review?(dbaron)
This defines a class CSSVariableValues which is used to store
computed variable values.  We store them a bit differently from
CSSVariableDeclarations -- here we have a hash table of variable names
to integer IDs, and then an array of variables where the array index
is the ID.  This is because later on we'll want a stable order for the
variables to return from DOM APIs.

In addition to the string value of the variable, we store the type
of the first and last token of the variable value.  This information
will be used when resolving entire variable reference containing
values, to determine when to insert "/**/" before and after a resolved
var(blah) token.

We add a CSSVariableValues member to nsStyleVariables.
Attachment #804218 - Flags: review?(dbaron)
We add a new class CSSVariableResolver whose job is to take the
inherited computed variables and the specified variable declarations and
to perform cycle removal and resolution of the variables, storing the
result in the CSSVariableValues object on an nsStyleVariables.  We use
CSSVariableResolver in nsRuleNode::ComputeVariablesData.

The variable resolver does this:

  1. Asks the CSSVariableValues and CSSVariableDeclarations objects
     to add their variables to it.
  2. Calls in to a new nsCSSParser function
     EnumerateVariableReferences that informs the resolver which
     other variables a given variable references, and by doing so,
     builds a graph of variable dependencies.
  3. Removes variables involved in cyclic references using Tarjan's
     strongly connected component algorithm, setting those variables
     to have an invalid value.
  4. Calls in to a new nsCSSParser function ResolveVariableValue
     to resolve the remaining valid variables by substituting variable
     references.

We extend nsCSSParser::ParseValueWithVariables to take a callback
function to be invoked when encountering a variable reference.  This
lets EnumerateVariableReferences re-use ParseValueWithVariables.

CSSParserImpl::ResolveValueWithVariableReferences needs different
error handling behaviour from ParseValueWithVariables, so we don't
re-use it.

CSSParserImpl::AppendImpliedEOFCharacters is used to take the
value returned from nsCSSScanner::GetImpliedEOFCharacters while
resolving variable references that were declared using custom
properties that encountered EOF before being closed properly.

The SeparatorRequiredBetweenTokens helper function in nsCSSParser.cpp
implements the serialization rules in CSS Syntax Module Level 3:

https://dvcs.w3.org/hg/csswg/raw-file/3479cdefc59a/css-syntax/Overview.html#serialization
Attachment #804219 - Flags: review?(dbaron)
This is the first part of handling variable references in regular
properties.  We have the scanner set a flag whenever it returns a "var("
token, so that when we come to the end of parsing a property that
failed, we know that it is because of a variable reference.
Attachment #804220 - Flags: review?(dbaron)
This adds a new nsCSSValue unit type to represent an unparsed
stream of CSS tokens as a specified value.  This is what properties
that have a variable reference get as their specified value.

On the nsCSSValueTokenStream object that is used when mUnit ==
eCSSUnit_TokenStream, we store two property IDs: first, the property
ID for the longhand that this token stream value is the value for.  We
pass this back in to nsCSSParser::ParseProperty at computed value time,
when we need to re-parse the property.  Second is the shorthand property
ID, if we used a variable reference in a shorthand.  In such a case, we
store the token stream value for each of the corresponding longhand
properties.  This is because shorthands don't actually get any storage
in an nsRuleData, and because any of the longhands might be overwritten
by subsequent declarations, we need to keep the token stream somewhere.

We also store other information on the nsCSSValueTokenStream required by
the CSS parser (base URI, etc.).
Attachment #804221 - Flags: review?(dbaron)
This adds functions to nsCSSParser and nsCSSScanner that let us save the
current input position (and corresponding information like line/column
number) and parser pushback, and be able to restore it later.  We'll use
this when rewinding the scanner after we first encounter a property with
a variable reference and go back to reparse it as a token stream.
Attachment #804222 - Flags: review?(dbaron)
This stores on the nsCSSParser whether we are somewhere in the middle
of parsing an @supports condition.  Because @supports condition parsing
uses the scanner recording function (to save its conditionText), and
variable reference containing values also need it, we can't do both at
once.  Luckily, if we're parsing an @supports condition, we don't really
need to store the token stream text; we only need to know if it was
valid, and we don't need its actual value later.  So we use this flag
later to see if we can skip turning on scanner recording while parsing
variable reference containing values.
Attachment #804223 - Flags: review?(dbaron)
This adds functionality to nsCSSParser to recognise properties with
variable references and store their recorded token stream as an
eCSSUnit_TokenStream nsCSSValue.
Attachment #804224 - Flags: review?(dbaron)
Add a helper function to nsCSSProps to look up whether a given
nsCSSProperty lives in a style struct for inherited properties.
Attachment #804225 - Flags: review?(dbaron)
This factors out the part of nsCSSCompressedDataBlock::MapRuleInfoInto
that starts image loading and maybe-copies an nsCSSValue into
an nsRuleData.  We will need this functionality for mapping re-parsed
properties that had variable references into an nsRuleData, which
will be done from an nsCSSExpandedDataBlock.
Attachment #804226 - Flags: review?(dbaron)
This adds a new eCSSUnit_SharedList type for nsCSSValue, which is a
reference counted object that contains an nsCSSValueList.  We need this
so that nsStyleDisplay::mSpecifiedTransform can hold a strong reference
to a specified transform list value.  When 'transform' is specified
using a variable reference, the resulting nsCSSValue does not stick
around in the Declaration object, so we wouldn't be guaranteed that
it lives long enough for nsStyleDisplay to keep referencing it.
Attachment #804227 - Flags: review?(dbaron)
This re-parses property values at computed value time if
they had a specified value that was a token stream.  We add
a function nsRuleNode::ResolveVariableReferences that looks
at all the values in the nsRuleData and calls in to a new
nsCSSParser::ParsePropertyWithVariableReferences function if they have a
token stream value.

We add a nsCSSExpandedDataBlock::MapRuleInfoInto function that will
take the re-parsed property values and copy them back into the
nsRuleData.  In here, we check to make sure we only overwrite values
in the nsRuleData if they currently have a token stream value.  This
lets us handle cases like:

  font: var(a);
  font-size: 16px;

where we don't want the re-parsing of 'font' to overwrite the value
of 'font-size' we've got.

nsRuleNode::ResolveVariableReferences returns whether any resolved
properties values became 'inherit', so that nsRuleNode::WalkRuleTree
can fix up the rule detail as appropriate.
Attachment #804228 - Flags: review?(dbaron)
This adds CSS parser error reporting for parsing of custom properties
and normal properties that have variable references.

When re-parsing a normal property that had a variable reference, we
report any parse error to be at the beginning of the property value.
This is because it is difficult to keep track of where exactly each
variable substitution came from to point to the particular value
that would have caused the parse error.  For example, with this:

  :root {
    var-a: 1px 2px;
    var-b: 3px var(a);
  }
  p {
    margin: var(a) var(b);
  }

we would end up resolving the value of 'margin' to:

  "  1px 2px  3px  1px 2px"

In this string, the parse error occurs when we encounter the final
"2px", but by this point we don't know where that value came from.
So instead we just point to the line on which 'margin' was declared.

We extend ErrorReporter with an OutputError overload that takes the
specific line and column number to use in the error report to get this
right, and we store the line and column number for each token stream
we parse on the nsCSSValueTokenStream object.
Attachment #804229 - Flags: review?(dbaron)
This makes updates work correctly when variable values change.
Rather than handling nsStyleVariables with a DO_STRUCT_DIFFERENCE,
we explicitly compare the two nsStyleVariables objects in
nsStyleContext::CalcStyleDifference before looking at the other
style structs.  This is because we need to force those other
style structs to be compared if variable values are changing.
nsStyleVariables::CalcDifference still returns 0, since the change in
variable values themselves doesn't require any updates.
Attachment #804230 - Flags: review?(dbaron)
This just factors out some code from
nsComputedDOMStyle::GetCSSPropertyValue into a
UpdateCurrentStyleInformation and ClearCurrentStyleInformation function.
These will will be needed in nsComputedDOMStyle::IndexGetter so that it
can have up to date information on how many custom properties there are
on the node.
Attachment #804232 - Flags: review?(dbaron)
This exposes the names of the custom properties on a computed style
object through its indexed property getter, just after all of the
built-in properties.
Attachment #804233 - Flags: review?(dbaron)
This adds support for custom properties on the remainder of the
CSSStyleDeclaration methods.

A shorthand that was specified using a variable reference is serialized
to its originally specified value, rather than by concatenating the
serializations of its longhand components.

A shorthand that was not specified using a variable references but which
has a component longhand that was is serialized to the empty string.
Attachment #804234 - Flags: review?(dbaron)
Since we cannot parse and discard a white space token after the ':' in a
custom property declaration, we instead explicitly skip over it when
parsing a non-custom property's value.
Attachment #804236 - Flags: review?(dbaron)
Attached patch Part 26: Tests. (obsolete) — Splinter Review
Attachment #804238 - Flags: review?(dbaron)
Attachment #804238 - Attachment description: try: -b do -p all -u all -t none - Part 26: Tests. r? → Part 26: Tests.
Add a missing return after setting an exception on an ErrorResult.
Attachment #804232 - Attachment is obsolete: true
Attachment #804232 - Flags: review?(dbaron)
Attachment #815800 - Flags: review?(dbaron)
Reworked on top of new bug 914072 patch.
Attachment #816309 - Flags: review?(dbaron)
Attachment #804219 - Attachment is obsolete: true
Attachment #804219 - Flags: review?(dbaron)
Reworked on top of new bug 914072 patch.
Attachment #804235 - Attachment is obsolete: true
Attachment #804235 - Flags: review?(dbaron)
Attachment #816311 - Flags: review?(dbaron)
Reworked on top of new bug 914072 patch.
Attachment #804238 - Attachment is obsolete: true
Attachment #804238 - Flags: review?(dbaron)
Attachment #816312 - Flags: review?(dbaron)
Handle 'unset' like 'inherit' when used as the value of a variable.
Attachment #816313 - Flags: review?(dbaron)
Patch queue rebased on top of a mozilla-central from a couple of days ago updated here again: http://mcc.id.au/temp/bug-773296/
Try run based on that patch queue: https://tbpl.mozilla.org/?tree=Try&rev=211f50818618
Comment on attachment 804213 [details] [diff] [review]
Part 2: Parse CSS variable declarations and store them on Declaration objects. p=ebassi,heycam

Review of attachment 804213 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/CSSVariableDeclarations.cpp
@@ +71,5 @@
> +    return false;
> +  }
> +  if (value.EqualsLiteral(INITIAL_VALUE)) {
> +    aType = eInitial;
> +    aTokenStream.Truncate(0);

Truncate() without an argument, please.

@@ +74,5 @@
> +    aType = eInitial;
> +    aTokenStream.Truncate(0);
> +  } else if (value.EqualsLiteral(INHERIT_VALUE)) {
> +    aType = eInitial;
> +    aTokenStream.Truncate(0);

Ditto.

@@ +112,5 @@
> +
> +static size_t
> +SizeOfTableEntry(const nsAString& aKey,
> +                 const nsString& aValue,
> +                 mozilla::MallocSizeOf aMallocSizeOf,

No need for any of the mozilla::'s around here.

::: layout/style/CSSVariableDeclarations.h
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* CSS Custom Property assignments for a Declaration at a given priority */
> +
> +#ifndef mozilla_Variables_h

mozilla_CSSVariableDeclarations_h

@@ +7,5 @@
> +
> +#ifndef mozilla_Variables_h
> +#define mozilla_Variables_h
> +
> +#include "mozilla/DebugOnly.h"

I don't think you need this.

@@ +94,5 @@
> +
> +  /**
> +   * Returns the number of entries in this set of variable declarations.
> +   */
> +  bool Count() const { return mVariables.Count(); }

bool?

::: layout/style/Declaration.cpp
@@ +866,5 @@
>    if (propID == eCSSProperty_UNKNOWN) {
>      return false;
>    }
> +  if (propID == eCSSPropertyExtra_variable) {
> +    return GetVariableValueIsImportant(Substring(aProperty, 4));

Magic number :/

@@ +921,5 @@
>  void
> +Declaration::AppendVariableAndValueToString(const nsAString& aName,
> +                                            nsAString& aResult) const
> +{
> +  aResult.Append(NS_LITERAL_STRING("var-"));

AppendLiteral

@@ +1094,5 @@
> +    nsCSSProperty property = GetPropertyAt(aIndex);
> +    if (property == eCSSPropertyExtra_variable) {
> +      GetCustomPropertyNameAt(aIndex, aReturn);
> +      return true;
> +    } else if (0 <= property) {

No else after return

@@ +1128,5 @@
>    n += mOrder.SizeOfExcludingThis(aMallocSizeOf);
>    n += mData          ? mData         ->SizeOfIncludingThis(aMallocSizeOf) : 0;
>    n += mImportantData ? mImportantData->SizeOfIncludingThis(aMallocSizeOf) : 0;
> +  if (mVariables) {
> +    mVariables->SizeOfIncludingThis(aMallocSizeOf);

Did you mean to use the result of this function?

@@ +1149,5 @@
> +                                    nsAString& aValue) const
> +{
> +  CSSVariableDeclarations::Type type;
> +  nsString value;
> +  aValue.Truncate();

The Truncate() is usually done first

@@ +1180,5 @@
> +                                    bool aIsImportant)
> +{
> +  if (!IsMutable()) {
> +    return;
> +  }

Nit: this function could use a few more empty lines.

@@ +1188,5 @@
> +    index = mVariableOrder.Length();
> +    mVariableOrder.AppendElement(aName);
> +  }
> +  CSSVariableDeclarations* variables;
> +  propertyIndex = index + eCSSProperty_COUNT;

Declare propertyIndex here.

::: layout/style/Declaration.h
@@ +281,5 @@
> +   * Returns the property at the given index in the ordered list of
> +   * declarations.  For custom properties, eCSSPropertyExtra_variable
> +   * is returned.
> +   */
> +  nsCSSProperty GetPropertyAt(uint32_t aIndex) const {

{ goes on the next line

::: layout/style/moz.build
@@ +10,5 @@
>  MODULE = 'layout'
>  
>  EXPORTS += [
>      'AnimationCommon.h',
> +    'CSSVariableDeclarations.h',

Put it under EXPORTS.mozilla.

::: layout/style/nsCSSParser.cpp
@@ +11125,5 @@
> +
> +  // Look for 'initial' or 'inherit' as the first non-white space token.
> +  CSSVariableDeclarations::Type type = CSSVariableDeclarations::eTokenStream;
> +  if (mToken.mType == eCSSToken_Ident) {
> +    if (mToken.mIdent.LowerCaseEqualsLiteral("initial")) {

I think this is correct now, but please add tests for the Turkish dotted I/dotless i.

@@ +11252,5 @@
> +                                        const nsAString& aName,
> +                                        uint32_t aFlags)
> +{
> +  CSSVariableDeclarations::Type type;
> +  nsString variableValue;

This one can be declared a little later, I think.

::: layout/style/nsCSSProps.cpp
@@ +349,5 @@
>  
> +/* static */ bool
> +nsCSSProps::IsCustomPropertyName(const nsACString& aProperty)
> +{
> +  return aProperty.Length() >= 5 &&

Magic number. Also, seems like this checks for "var-" and at least one more character? Worth commenting, IMO.
Comment on attachment 804216 [details] [diff] [review]
Part 5: Map variables on a Declaration to nsRuleData.

Review of attachment 804216 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/CSSVariableDeclarations.cpp
@@ +120,5 @@
> +{
> +  nsDataHashtable<nsStringHashKey, nsString>* variables =
> +    static_cast<nsDataHashtable<nsStringHashKey, nsString>*>(aData);
> +  nsString oldValue;
> +  if (!variables->Get(aName, &oldValue)) {

I think you can use Contains().

::: layout/style/Declaration.h
@@ +63,5 @@
>    bool HasProperty(nsCSSProperty aProperty) const;
>  
>    void GetValue(nsCSSProperty aProperty, nsAString& aValue) const;
>  
> +  bool HasImportantData() const {

{ on the next line
Comment on attachment 804218 [details] [diff] [review]
Part 7: Add a field to nsStyleVariables to store computed variable values.

Review of attachment 804218 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/moz.build
@@ +11,5 @@
>  
>  EXPORTS += [
>      'AnimationCommon.h',
>      'CSSVariableDeclarations.h',
> +    'CSSVariableValues.h',

Again, EXPORTS.mozilla
Comment on attachment 804226 [details] [diff] [review]
Part 15: Factor out mapping of a single property from an nsCSSCompresseDataBlock to an nsRuleData.

Review of attachment 804226 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSDataBlock.cpp
@@ +108,5 @@
> +                      const nsCSSValue* aValue,
> +                      nsCSSValue* aTarget,
> +                      nsRuleData* aRuleData)
> +{
> +    NS_ABORT_IF_FALSE(aValue->GetUnit() != eCSSUnit_Null, "oops");

This file seems to be inconsistent in 2/4-space indentation, so match the style guide, I guess.
Comment on attachment 804227 [details] [diff] [review]
Part 16: Add a ref-counted list nsCSSValue unit and use it for tranform lists; hold a strong reference to one on nsStyleDisplay.

Review of attachment 804227 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSValue.h
@@ +731,5 @@
> +  void AppendToString(nsCSSProperty aProperty, nsAString& aResult) const;
> +
> +  bool operator==(nsCSSValueSharedList const& aOther) const;
> +  bool operator!=(const nsCSSValueSharedList& aOther) const
> +  { return !(*this == aOther); }

On three lines, please

@@ +735,5 @@
> +  { return !(*this == aOther); }
> +
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
> +
> +  nsCSSValueList* mHead;

Not sure how I feel about making this public.
Comment on attachment 804229 [details] [diff] [review]
Part 18: Add error reporting for invalid variable references.

Review of attachment 804229 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSScanner.h
@@ +142,5 @@
>    friend class nsCSSScanner;
>  public:
>    nsCSSScannerPosition() : mInitialized(false) { }
>  
> +  uint32_t LineNumber() {

{ on the next line
Comment on attachment 804230 [details] [diff] [review]
Part 19: Compare style structs even for the same rule node when variables have changed.

Review of attachment 804230 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/CSSVariableValues.h
@@ +29,5 @@
>    CSSVariableValues& operator=(const CSSVariableValues& aOther);
>  
> +  bool operator==(const CSSVariableValues& aOther) const;
> +  bool operator!=(const CSSVariableValues& aOther) const
> +  { return !(*this == aOther); }

Three lines
Comment on attachment 816309 [details] [diff] [review]
Part 8: Resolve and compute CSS variables. (v1.1)

Review of attachment 816309 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/CSSVariableDeclarations.cpp
@@ +153,5 @@
> +  CSSVariableResolver* resolver = static_cast<CSSVariableResolver*>(aData);
> +  if (aValue.EqualsLiteral(INITIAL_VALUE)) {
> +    // Values of 'initial' are treated the same as an invalid value in the
> +    // variable resolver.
> +    resolver->Put(aName, nsString(),

EmptyString()?

::: layout/style/CSSVariableResolver.h
@@ +22,5 @@
> +class CSSVariableValues;
> +class EnumerateVariableReferencesData;
> +}
> +
> +namespace mozilla {

No need to close and open the namespace

::: layout/style/CSSVariableValues.h
@@ +15,5 @@
>  namespace mozilla {
> +class CSSVariableResolver;
> +}
> +
> +namespace mozilla {

Ditto

::: layout/style/moz.build
@@ +11,5 @@
>  
>  EXPORTS += [
>      'AnimationCommon.h',
>      'CSSVariableDeclarations.h',
> +    'CSSVariableResolver.h',

Again, .mozilla

::: layout/style/nsCSSParser.cpp
@@ +164,5 @@
> +   */
> +  bool EnumerateVariableReferences(const nsAString& aPropertyValue,
> +                                   void (*aFunc)(const nsAString&, void*),
> +                                   void* aData);
> +  

Trailing ws

@@ +3509,5 @@
> +        if (stackTopIndex == 0) {
> +          return true;
> +        }
> +
> +      // Just handle out-of-memory by parsing incorrectly.  It's

(Where's the oom?)
Comment on attachment 816312 [details] [diff] [review]
Part 26: Tests. (v1.1)

Review of attachment 816312 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_css_supports_variables.html
@@ +4,5 @@
> +https://bugzilla.mozilla.org/show_bug.cgi?id=773296
> +-->
> +<head>
> +  <title>Test for Bug 773296</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>

testharness.js? ;)
(In reply to :Ms2ger from comment #112)
> ::: layout/style/nsCSSDataBlock.cpp

> This file seems to be inconsistent in 2/4-space indentation, so match the
> style guide, I guess.

That file uses 4 space indent, though maybe we should consider converting it.  The chunks that slipped in with 2-space indent were mistakes.

(In reply to :Ms2ger from comment #115)
> Comment on attachment 804230 [details] [diff] [review]
> Part 19: Compare style structs even for the same rule node when variables
> have changed.
> > +  bool operator==(const CSSVariableValues& aOther) const;
> > +  bool operator!=(const CSSVariableValues& aOther) const
> > +  { return !(*this == aOther); }
> 
> Three lines

I think the style there is ideal, except that I'd have indented the { two more spaces.
(In reply to :Ms2ger from comment #114)
> Comment on attachment 804229 [details] [diff] [review]
> Part 18: Add error reporting for invalid variable references.
> > +  uint32_t LineNumber() {
> 
> { on the next line

Disagree here as well; out-of-line functions have their opening { on its own line; inline functions do not.  (Local style in layout has both of those as strictly-applied rules, I think.)
Comment on attachment 804209 [details] [diff] [review]
Part 1: Add a preference for CSS variables.

Given that the nearby booleans don't begin with "Is" or "Are", I'd rather this one didn't as well.  How about s/AreVariablesEnabled/CSSVariablesEnabled/g, which is also less ambiguous?

r=dbaron with that
Attachment #804209 - Flags: review?(dbaron) → review+
Comment on attachment 804213 [details] [diff] [review]
Part 2: Parse CSS variable declarations and store them on Declaration objects. p=ebassi,heycam

You should also handle the 'unset' keyword specially (need to come
up with another character!) throughout this patch.

Declaration::Declaration needs to copy mImportantVariables as well, and
you should add a test that fails because it doesn't.

Declaration::AddVariableDeclaration should probably make assertions
about aValue inside the switch(aType), for all the cases other than
eTokenStream.  (Should it be empty?)

In Declaration::ValueAppended:

>-  NS_ABORT_IF_FALSE(!nsCSSProps::IsShorthand(aProperty),
>+  NS_ABORT_IF_FALSE(0 <= aProperty && aProperty < eCSSProperty_COUNT &&
>+                    !nsCSSProps::IsShorthand(aProperty),
>                     "shorthands forbidden");

I think this change is unneeded since IsShorthand has, inside of it,
another NS_ABORT_IF_FALSE that its parameter is a valid property.

IN Declaration::GetNthProperty:

>+    nsCSSProperty property = GetPropertyAt(aIndex);
>+    if (property == eCSSPropertyExtra_variable) {
>+      GetCustomPropertyNameAt(aIndex, aReturn);
>+      return true;
>+    } else if (0 <= property) {

Avoid else-after-return.


In Declaration::AddVariableDeclaration, you shouldn't be checking
IsMutable and bailing.  It's probably reasonable to AssertMutable,
although I don't recall if it's meaningfully necessary.  I guess you may
as well.

Same method, declare propertyIndex at first use rather than above, and
swap that first use with the declaration of variables.

Same method:  you're being inconsistent about what to do with the
important boolean.  (There's a long-running debate about that on
www-style.)  But we should be consistent with the equivalent code.  The
problem is that the equivalent code is different between the CSSOM and
normal parsing (see aOverrideImportant in
nsCSSExpandedDataBlock::DoTransferFromBlock).  You'll need to audit the
callers with the whole patch stack applied to see if you need a similar
boolean.  In any case, your current behavior in aIsImportant==false case
doesn't match *either* of the desired behaviors (for the case where
mImportantVariables already has an occurrence of that variable).  We
either want to no-op, or we want to remove from mImportantVariables
along with adding to mVariables.

>+bool
>+Declaration::GetVariableValueIsImportant(const nsAString& aName) const
>+{
>+  return mImportantVariables && mVariables->Has(aName);
>+}

Looks like the second bit should be mVariables rather than
mImportantVariables.  Please fix and add a test.

nsCSSParser.cpp:

>+  // calls SkipUntil with each character from aStopSymbolChars from the end
>+  // of the string (indicated by aCount) to the start
>+  void SkipUntilAllOf(const PRUnichar* aStopSymbolChars, uint32_t aCount);

This comment is incomprehensible.  Please fix.

I think you should also just change the function to take
  const nsAutoTArray<PRUnichar, 16>& aStack
(probably with the help of a typedef) since that's what all the callers
have.

>+  /**
>+   * Parses a CSS variable value.  This could be 'initial', 'inherit'
>+   * or a token stream, which may or may not include variable references.
>+   *
>+   * @param aType Out parameter into which the type of the variable value
>+   *   will be stored.
>+   * @param aClosingBrackets Out parameter appended to which will be any
>+   *   closing brackets that were implied when encountering EOF.
>+   * @return Whether parsing succeeded.
>+   */
>+  bool ParseValueWithVariables(CSSVariableDeclarations::Type* aType,
>+                               nsString& aClosingBrackets);

I think we've run into one of the annoying differences between varieties
of English.  In US English, "brackets" are the square ones, [], braces
are curly {}, and parentheses are round (). (Perhaps there's a little
leeway between brackets and braces, but we'd never call () brackets or
braces.)

So could you just call it aClosingChars instead?

>+      REPORT_UNEXPECTED_P(PEUnknownProperty, propertyName);

Reporting failure in ParseVariableDeclaration as an unknown property
seems wrong.  I think you should either:
 (a) make a new error message describing the things that could have
 gone wrong, or
 (b) make multiple messages, and ensure that all the paths that lead to
 ParseVariableDeclaration return one of them (and leave a comment at the
 caller that there's already a specific message).
Which is better depends on how varied the cases are.  (Or maybe you've
already done all of (b) except for the removing the PEUnknownProperty
message.)

Could you put ParseVariableDeclaration and ParseValueWithVariables
in the opposite order?  The parser tends to have the functions for
parsing larger structures before those for parsing their subparts.
(Both .cpp and .h, please.)

>+  // Indexes into ')' characters in |stack| that correspond to "var(".  This
>+  // is used to stop parsing when we encounter a '!' or ';' at the top level
>+  // of a variable reference's fallback.
>+  nsAutoTArray<uint32_t, 16> references;

The idea of what this (and the code that uses it) is doing doesn't seem
to be supported by the spec, or at least I can't find it.  It might be
worth raising, though.  But either way, it should match the spec.

And the same holds for the code that checks that the thing after var( is
an ident.

(I think it's really an ambiguity in the spec -- it seems like the spec
author hasn't considered whether these errors should be invalid at parse
time or computed value time.  So I wouldn't say the spec calls for one
or the other -- but it should.)


nsCSSProps.h:

Please comment that all callers of IsCustomPropertyName need to first
check if variables are enabled.



Did you audit that none of the paths to the callers of
IsCustomPropertyName do case transformation?  If not, please do.

r=dbaron with those things fixed
Attachment #804213 - Flags: review?(dbaron) → review+
Comment on attachment 804214 [details] [diff] [review]
Part 3: Allow more than 27 style structs.

r=dbaron
Attachment #804214 - Flags: review?(dbaron) → review+
Comment on attachment 804215 [details] [diff] [review]
Part 4: Add style struct to store CSS variables.

nsCSSPropList.h:
  I have mixed feelings about adding the CSS_PROP_VARIABLES macro, but I
  guess it's ok.

nsRuleNode.cpp:
  Please add to the end of nsRuleNode::SetDefaultOnRoot rather than the 
  start.

  Need to make nsStyleVariables::CalcDifference actually do something, 
  for bug 931668.  This probably means giving
  nsChangeHint_BorderStyleNoneChange a more general name.  This should 
  be a separate patch, though, on which you should request review.  Make
  sure to update the MaxDifference method.  (This is perhaps more
  related to patch 22 than this patch.)

r=dbaron with that
Attachment #804215 - Flags: review?(dbaron) → review+
Comment on attachment 804216 [details] [diff] [review]
Part 5: Map variables on a Declaration to nsRuleData.

nsRuleData.h:

  Please put a blank line before mVariables, or maybe even put it
  after mValueStorage and mValueOffsets, since it's more semantically 
  related to mValueStorage and mValueOffsets than what you grouped it 
  with.

CSSVariableDeclarations.cpp:

  (I'm annoyed that nsDataHashtable doesn't have a Has or Contains
  method, but I'm not going to ask you to fix that here.)

  >+  if (!(aRuleData->mSIDs & (1 << eStyleStruct_Variables))) {

  Please use NS_STYLE_INHERIT_BIT(Variables) rather than writing
  (1 << eStyleStruct_Variables) explicitly.

r=dbaron
Attachment #804216 - Flags: review?(dbaron) → review+
Comment on attachment 804217 [details] [diff] [review]
Part 6: Add enum to represent types of CSS tokens involved in serialization.

Maybe put the spec link in the code comment rather than the commit message?

For now I'm going to assume the spec's logic is sound rather than review it, though I may yet choose to review it when I review the patch that uses these...

r=dbaron
Attachment #804217 - Flags: review?(dbaron) → review+
Comment on attachment 804216 [details] [diff] [review]
Part 5: Map variables on a Declaration to nsRuleData.

Also, please put a single-line comment at the top of CSSVariableDeclarations.h and .cpp, before the first #ifdef, that says something like /* specified values of CSS variables */ .  This will show up in http://mxr.mozilla.org/mozilla-central/source/layout/style/
Comment on attachment 804218 [details] [diff] [review]
Part 7: Add a field to nsStyleVariables to store computed variable values.

CSSVariableValues.h/cpp:

  Please put the single line comment at the start of the .h in both the
  .h and the .cpp, and have it before the first #ifdef, so it shows up
  in http://mxr.mozilla.org/mozilla-central/source/layout/style/

CSSVariableValues.cpp:

  operator= should bail at the start if this == &aOther.

  (And, oops, the same comment applies to patch 5, 
  CSSVariableDeclarations!)

CSSVariableValues.h:

  >+  struct Variable
  >+  {
  >+    Variable()
  >+      : mFirstToken(eCSSTokenSerialization_Nothing)
  >+      , mLastToken(eCSSTokenSerialization_Nothing) { }
  >+
  >+    Variable(const nsAString& aVariableName,
  >+             nsString aValue,
  >+             nsCSSTokenSerializationType aFirstToken,
  >+             nsCSSTokenSerializationType aLastToken)
  >+      : mVariableName(aVariableName)
  >+      , mValue(aValue)
  >+      , mFirstToken(aFirstToken)
  >+      , mLastToken(aLastToken) { }

  Please put the "{ }" on their own line, and probably without
  the space in between.

moz.build:
  The new header should be in EXPORTS.mozilla, and should be
  included as mozilla/CSSVariableValues.h.

  (And, oops, the same comment applies to patch 5,
  CSSVariableDeclarations!)

r=dbaron with that
Attachment #804218 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (needinfo? me) from comment #125)
> For now I'm going to assume the spec's logic is sound rather than review it,
> though I may yet choose to review it when I review the patch that uses
> these...

Looks like css-syntax-3 has recently changed the set of tokens between which "/**/" needs to be inserted, so I might need to revise this.
Blocks: 931668
Comment on attachment 816309 [details] [diff] [review]
Part 8: Resolve and compute CSS variables. (v1.1)

CSSVariableDeclarations.cpp:
>+  } else if (aValue.EqualsLiteral(INHERIT_VALUE)) {
>+    // Values of 'inherit' don't need any handling, since it means we just need
>+    // to keep whatever value is currently in the resolver.

  Could you comment that this is the case because the specified variable
  declarations already have only the winning declaration for the variable
  and no longer have any of the others?

CSSVariableResolver.h:

>+  /**
>+   * Creates a new CSSVariableValues that will output a set of resolved,
>+   * computed variables into aOutput.
>+   */
>+  CSSVariableResolver(CSSVariableValues* aOutput)
>+    : mOutput(aOutput)
>+    , mResolved(false)
>+  {
>+    MOZ_ASSERT(aOutput);
>+  }

  "Creates a new" doesn't seem accurate if it's passed in.

>+  void RemoveCycles(size_t v);

  Please give the argument a more descriptive name.

>+  size_t mIndex;

  Maybe mNextIndex?

CSSVariableResolver.cpp:
  Put the single-line comment at the top of the .cpp in addition to
  the .h so it shows up in MXR.  Perhaps make it a little shorter too?

>+#include "CSSVariableResolver.h"
>+
>+#include "CSSVariableDeclarations.h"
>+#include "CSSVariableValues.h"
>+#include "mozilla/PodOperations.h"

  Here, and elsewhere in the patch series, any headers that are declare
  a class with a namespace and are exported should be exported via
  EXPORTS.mozilla, and then included only as "mozilla/Class.h" and never
  as "Class.h".

>+  EnumerateVariableReferencesData(CSSVariableResolver& aResolver)
>+    : mResolver(aResolver)
>+    , mReferences(new bool[aResolver.mVariables.Length()])

  Maybe comment that Reset() needs to be called before first use?


>+void
>+CSSVariableResolver::Resolve(const CSSVariableValues* aInherited,
>+                             const CSSVariableDeclarations* aSpecified)
>+{
>+  MOZ_ASSERT(!mResolved);
>+  MOZ_ASSERT(aInherited);
>+
>+  aInherited->AddVariablesToResolver(this);
>+  if (aSpecified) {
>+    aSpecified->AddVariablesToResolver(this);
>+  }

Why would we bother calling this if aSpecified is null?  In other words,
if there aren't any specified variable declarations, wouldn't we just
copy the results from the parent?

Also, how do we ensure aInherited is never null?  What happens at the
root?  Or does the way ComputeVariablesData works lead to the root
style context passing aInherited==this?

I didn't read the SCC finding algorithm too closely; let me know if you
want me to.  (I've implemented Kosaraju's algorithm a few times, but
not Tarjan's.)

moz.build:
  Put the .h in EXPORTS.mozilla, and fix all #includes to match.

nsCSSParser.h:
  Could you name the callback function type, and use the name,
  rather than writing the signature every time?

nsCSSParser.cpp:

  SeparatorRequiredBetweenTokens should be updated to match the new
  table in http://dev.w3.org/csswg/css-syntax/#serialization

>+      case eCSSToken_Bad_String:
>+        return false;

  You should also return false for eCSSToken_Bad_URL.

>+          UngetToken();
>+          mScanner->StartRecording();

  For clarity, and in case we fix StartRecording to deal with or assert
  on pushback, could you swap the order of these two lines?

  You'll probably also need adjustment of the typse here to match
  updates to SeparatorRequiredBetweenTokens.


>+          bool valid = aVariables->Get(variableName, variableValue,
>+                                       varFirstToken, varLastToken) &&
>+                       !variableValue.IsEmpty();

  I haven't looked at the spec on this topic, but the IsEmpty check
  seems weird.  Could you at least add a comment explaining it (and
  why aVariables->Get()'s return value isn't sufficient)?

  The need for so many StartRecording calls in different places
  seems a bit awkward, but I don't immediately see a better solution.

>+              // Now we're either at the pushed back ')' that finished the
>+              // fallback or at EOF.
>+              GetToken(false);

  An assertion probably wouldn't be a bad idea (DebugOnly<bool> would
  be needed for the result of GetToken).

>+      default:
>+        UPDATE_RECORDING_TOKENS(eCSSTokenSerialization_Other);

  An assertion about what token types are left would probably be a good
  idea.

  I don't understand how things work when |value| is appended so late.
  It seems to me like it needs to be appended much closer to the
  var() function that uses it.  (And what if there are two var()
  functions inside a variable?)  If I'm right that this is wrong, please
  add tests that cover it.

  You should add comments to SkipBalancedContentUntil saying that it's
  not safe to use outside of variable parsing since it doesn't handle
  skipping of mismatched content correctly.

I didn't go into the EOF character business in much detail; let
me know if you want me to look at it more closely.

r=dbaron with the above
Attachment #816309 - Flags: review?(dbaron) → review+
Comment on attachment 804220 [details] [diff] [review]
Part 9: Give nsCSSScanner the ability to remember when it encounters a "var(" token.

r=dbaron
Attachment #804220 - Flags: review?(dbaron) → review+
Comment on attachment 804221 [details] [diff] [review]
Part 10: Add a new eCSSUnit_TokenStream type for storing unparsed CSS values. p=ebassi,heycam

># User Cameron McCormack <cam@mcc.id.au>

This field is free form, so you can put two users in it separated
by commas (which is what I generally do when a patch has multiple
authors).

>+  } else if (eCSSUnit_TokenStream == unit) {
>+    nsCSSProperty shorthand = mValue.mTokenStream->mShorthandPropertyID;
>+    if (shorthand == eCSSProperty_UNKNOWN ||
>+        shorthand == eCSSProperty__moz_transform) {
>+      // We treat serialization of 'transform' set through '-moz-transform' as a
>+      // special case, since it really wants to be handled like an alias, but is
>+      // implemented as a shorthand.
>+      aResult.Append(mValue.mTokenStream->mTokenStream);
>+    }

Instead of checking for __moz_transform, please use
nsCSSProps::PropHasFlags(shorthand, CSS_PROPERTY_IS_ALIAS)


But what's supposed to happen if this is a sub-part of a shorthand?
Does the spec say?  It seems like it ought to.  Do you want to raise
the spec issue, or should I?

>+  bool operator==(const nsCSSValueTokenStream& aOther) const
>+  {
>+    return mPropertyID == aOther.mPropertyID &&
>+           mShorthandPropertyID == aOther.mShorthandPropertyID &&
>+           mTokenStream.Equals(aOther.mTokenStream);
>+  }

Seems like this should check the 2 URIs and the principal as well.
(See the EqualURIs function in nsStyleStruct.cpp... and I'm not sure
about the right way to compare nsIPrincipals.)

I'm also curious why you need to store the longhand property ID.

r=dbaron
Attachment #804221 - Flags: review?(dbaron) → review+
Comment on attachment 804222 [details] [diff] [review]
Part 11: Give nsCSSParser and nsCSSScanner the ability to save/restore their current input state.

It seems a little odd to initialize mHavePushBack to false; I don't see why one value is better than the other or why it would ever be valid to use the initial value.

r=dbaron, possibly with that changed
Attachment #804222 - Flags: review?(dbaron) → review+
Comment on attachment 804223 [details] [diff] [review]
Part 12: Record whether we are parsing an @supports condition.

I'd slightly prefer AutoRestore (or using the AutoToggle patch in bug 518756).

r=dbaron either way
Attachment #804223 - Flags: review?(dbaron) → review+
Comment on attachment 804224 [details] [diff] [review]
Part 13: Parse properties that use variable references and store them as eCSSUnit_TokenStream values.

>+      const PRUnichar stopChars[] = { ';', '!', '}', ')', 0 };
>+      SkipUntilOneOf(stopChars);
>+      UngetToken();
>+      parseAsTokenStream = mScanner->SeenVariableReference();

This is a bit suspicious, though maybe it's ok.

At the very least, we never actually stay at the position resulting
from this SkipUntil, so it's only used for variable checks.


>+      if (nsCSSProps::IsShorthand(aPropID)) {
>+        // If this is a shorthand property, we store the token stream on each
>+        // of its corresponding longhand properties.
>+        CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aPropID) {
>+          nsCSSValueTokenStream* tokenStream = new nsCSSValueTokenStream;
>+          tokenStream->mPropertyID = *p;
>+          tokenStream->mShorthandPropertyID = aPropID;
>+          tokenStream->mTokenStream = propertyValue;
>+          tokenStream->mBaseURI = mBaseURI;
>+          tokenStream->mSheetURI = mSheetURI;
>+          tokenStream->mSheetPrincipal = mSheetPrincipal;
>+          value.SetTokenStreamValue(tokenStream);
>+          AppendValue(*p, value);
>+        }
>+      } else {
>+        nsCSSValueTokenStream* tokenStream = new nsCSSValueTokenStream;
>+        tokenStream->mPropertyID = aPropID;
>+        tokenStream->mTokenStream = propertyValue;
>+        tokenStream->mBaseURI = mBaseURI;
>+        tokenStream->mSheetURI = mSheetURI;
>+        tokenStream->mSheetPrincipal = mSheetPrincipal;
>+        value.SetTokenStreamValue(tokenStream);
>+        AppendValue(aPropID, value);
>+      }

Given that you're already checking mInSupportsCondition nearby,
and given that mInSupportsCondition implies that propertyValue is
the empty string, it seems like you may as well conditionalize this
bit on !mInSupportsCondition as well.

r=dbaron
Attachment #804224 - Flags: review?(dbaron) → review+
Comment on attachment 804225 [details] [diff] [review]
Part 14: Add an nsCSSProps::IsInherited function.

While I *think* it's currently true that all shorthands other than 'all' don't mix inherited and non-inherited properties, there's no guarantee that will remain true in the future; the CSS WG has definitely discussed shorthands that would mix them.

Given that it's relatively simple for the caller to handle, I'd prefer if you changed this so:
 * IsInherited asserts that it's been given a longhand property
 * you fix the caller (in part 17) to deal with that, and also to deal with 'all'.

Also, please use nsRuleNode::IsReset rather than reimplementing it, even though it is just one line.  (Actually, maybe we should have nsRuleNode::IsInherited in addition to IsReset so that we have both... and can perhaps migrate to using the IsInherited name rather than IsReset.)

r=dbaron with that
Attachment #804225 - Flags: review?(dbaron) → review+
Comment on attachment 804226 [details] [diff] [review]
Part 15: Factor out mapping of a single property from an nsCSSCompresseDataBlock to an nsRuleData.

r=dbaron
Attachment #804226 - Flags: review?(dbaron) → review+
Depends on: 945580
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #135)
> Also, please use nsRuleNode::IsReset rather than reimplementing it, even
> though it is just one line.  (Actually, maybe we should have
> nsRuleNode::IsInherited in addition to IsReset so that we have both... and
> can perhaps migrate to using the IsInherited name rather than IsReset.)

Added IsInherited in bug 945580.
Comment on attachment 804227 [details] [diff] [review]
Part 16: Add a ref-counted list nsCSSValue unit and use it for tranform lists; hold a strong reference to one on nsStyleDisplay.

I'm a little unhappy about the naming here.  At the very list, it's
inconsistent with ListDep/ListDependent, which put the ownership after
the fundamental type.  Then again, there is a difference in that
the dependent lists aren't actually a type distinction; they're
only a distinction in nsCSSValue itself.  And, another distinction
that I didn't expect when I started looking at the patch, that the
shared list object doesn't include the first item in the list; it's
just a wrapper object.  So I guess it's ok.

Could you at least add a comment in nsCSSValue.h, above the new type,
explaining that it's a reference-counted list, and pointing out
that the class itself is just a reference count and a pointer to the
actual list?

>+    // SharedList
>+    case eCSSUnit_SharedList:
>+      // n += mValue.mList->SizeOfIncludingThis(aMallocSizeOf);
>+      // XXX what here?
>+      break;

Maybe comment that it makes more sense not to measure since it most
cases it will actually be shared?


>+bool
>+nsCSSValueSharedList::operator==(const nsCSSValueSharedList& aOther) const
>+{
>+  if (!mHead || !aOther.mHead) {
>+    return false;
>+  }
>+  return *mHead == *aOther.mHead;
>+}

This is slightly wrong (returns false if both are null).  You want
  return !mHead == !aOther.mHead &&
         (!mHead || *mHead == *aOther.mHead);
or similar.

>+  nsCSSValueSharedList(nsCSSValueList* aList)
>+    : mHead(aList)
>+  {
>+    MOZ_COUNT_CTOR(nsCSSValueSharedList);
>+  }

Maybe comment that this takes ownership of aList?  Though if it's
unused I might be inclined to drop it.

I'd also be inclined to drop CloneInto; it looks unused, and it's an odd
API that corresponds to the SetListValue() API that returns a new list
head, which you also didn't add.  (Clone looks unused too, actually, but
it's at least a vaguely coherent API.  Maybe drop it too?  We don't have
Clone on the other things that could have it, but where it isn't used.)
(Were you initially planning to have the SharedList object be the first
item?)

>+  nsCSSValueSharedList(const nsCSSValueSharedList& aCopy) // makes a shallow copy
>+    : mHead(nullptr)
>+  {
>+    MOZ_COUNT_CTOR(nsCSSValueSharedList);
>+  }

This doesn't seem to make any sort of copy at all.  Is it used?

> void
>+nsStyleAnimation::Value::SetCSSValueSharedListValue(nsCSSValueSharedList* aList)
>+{
>+  FreeValue();
>+  mUnit = eUnit_Transform;
>+  mValue.mCSSValueSharedList = aList;
>+  mValue.mCSSValueSharedList->AddRef();
>+}

You should call this SetTransformValue given what it does to mUnit,
or you should require the unit to be passed as an argument (and
assert about it).


It's maybe worth double-checking with mattwoodrow or roc that
FrameTransformProperties isn't something we expect to be usable on the
compositor thread.  (If so, adding reference counting to it would be
bad... and the change also seems unnecessary, at least right now.)

r=dbaron with that
Attachment #804227 - Flags: review?(dbaron) → review+
Comment on attachment 804228 [details] [diff] [review]
Part 17: Resolve property values that have variable references at computed value time.

Rather than these big chunks of code:

>+    bool shorthand = true;

>+      shorthand = false;

>+    // Check to see if reparsing the property failed and its value was
>+    // treated as 'inherit'.
>+    if (shorthand) {
>+      CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, prop) {
>+        // We might look at a corresponding longhand that wasn't overwritten
>+        // by the ParsePropertyWithVariableReferences call above.  This
>+        // doesn't matter, since WalkRuleTree will have already taken that
>+        // 'inherit' value into account when computing the rule detail.
>+        if ((nsCachedStyleData::GetBitForSID(nsCSSProps::kSIDTable[*p]) &
>+             aRuleData->mSIDs) &&
>+            aRuleData->ValueFor(*p)->GetUnit() == eCSSUnit_Inherit) {
>+          anyInherit = true;
>+          break;
>+        }
>+      }
>+    } else {
>+      if (aRuleData->ValueFor(prop)->GetUnit() == eCSSUnit_Inherit) {
>+        anyInherit = true;
>+      }
>+    }

>+    if (anyInherit) {
>+      // One property's value might have became 'inherit' after resolving
>+      // variable references.  (This happens when an inherited property
>+      // fails to parse its resolved value.)  Adjust |detail| appropriately.
>+      switch (detail) {
>+        case eRuleNone:
>+          MOZ_ASSERT("unexpected rule detail value");
>+          break;
>+        case eRulePartialReset:
>+          detail = eRulePartialMixed;
>+          break;
>+        case eRuleFullReset:
>+          detail = eRuleFullMixed;
>+          break;
>+        default:
>+          break;
>+      }
>+    }

maybe you should just have nsRuleNode::ResolveVariableReferences return
whether it resolved any variables, and use that boolean to set a
bool recheckSpecifiedProperties in WalkRuleTree, and then consolidate
that with the code that reruns CheckSpecifiedProperties when we
have a nonzero pseudoRestriction?


>+  if (!(aRuleData->mSIDs & (1 << aSID))) {

Replace (1 << aSID) with nsCachedStyleData::GetBitForSID(aSID).



I think ResolveVariableReferences doesn't work correctly in the presence
of overlapping shorthands.  Given that it iterates the longhands in
a particular order, and then every time it parses a shorthand, it seems
to fill in every longhand whose value is a token stream, I think it
will get the following case wrong, or the following case with the last 2
lines swapped (depending on the order of the longhands in the array we're
reading from):
  var-style: solid;
  var-left: green dashed;
  border-style: var(style);
  border-left: var(left);

Given that you run the parsing for every single longhand anyway, I think
it probably makes sense to:
 - remove the dance you do around TokenStream properties in
   nsCSSExpandedDataBlock::MapRuleInfoInto
 - pass both the shorthand ID and the longhand ID to
   ParsePropertyWithVariableReferences, parse the shorthand, but only
   map the longhand
 - change nsCSSExpandedDataBlock::MapRuleInfoInto into something that's
   a thinner wrapper around MapSinglePropertyInto, and takes only
   longhands

In nsRuleNode::SetGenericFont, you may as well call
ResolveVariableReferences after the code that conditionally resets the
font-family, since it'll save a little bit of work.

nsCSSParser.cpp:

>+  mTempData.AssertInitialState();
>+  bool valid;
>+  nsString expandedValue;

Blank line between the assertion and the declarations, please.

>+      REPORT_UNEXPECTED(PEDeclDropped);

You should use a custom error message here that, instead of saying
the declaration was dropped, says that we fell back to initial or
inherit.

Do the error messages here give useful location information?  If not,
it might be worth improving, but probably in a followup patch.

And see my comments on IsInherited from comment 135.

r=dbaron with those fixes

(And I wrote a whole bunch of other comments that I then realized were
wrong, too.  Oh well.)
Attachment #804228 - Flags: review?(dbaron) → review+
Also, could you add tests testing:

 (1) the overlapping border shorthands case in the previous comment (both ways around), and check that one fails with the current code

 (2) the following two cases, and check that both fail if you remove the aRuleData->mCanStoreInRuleTree = false; in nsRuleNode::ResolveVariableReferences:

    <style>
      p { var-foo: purple; background-color: var(foo) }
      p#myid { var-foo: blue }
    </style>
    <p>This should be purple, and might cache background in the rule tree.</p>
    <p id="myid">This should be blue, and can't used that cached struct,
    which really shouldn't be cached anyway.</p>

    <style>
      div#a { var-foo: purple }
      div#b { var-foo: blue }
      p { background-color: var(foo) }
    </style>
    <div id="a"><p>This should be purple.</p></div>
    <div id="b"><p>This should be blue.</p></div>
Comment on attachment 804229 [details] [diff] [review]
Part 18: Add error reporting for invalid variable references.

Ah, I guess you can ignore my "probably in a followup patch" advice from
comment 139!  (But still fix the use of the PEDeclDropped message, both
in the one introduced in that patch and the one introduced here.)

>+PEExpectedVariableNameEOF=Expected identifier for variable name but found EOF.

change this to just "identifier for variable name"

and then instead of REPORT_UNEXPECTED here:

>+            REPORT_UNEXPECTED(PEExpectedVariableNameEOF);

use REPORT_UNEXPECTED_EOF.

(Probably double-check the output to make sure I got it right, though.)

r=dbaron with that
Attachment #804229 - Flags: review?(dbaron) → review+
Comment on attachment 804230 [details] [diff] [review]
Part 19: Compare style structs even for the same rule node when variables have changed.

r=dbaron, although I wouldn't mind index variables that have more descriptive names than i and j (or prefix ++ rather than postfix).
Attachment #804230 - Flags: review?(dbaron) → review+
Comment on attachment 804231 [details] [diff] [review]
Part 20: Return variables from getComputedStyle().get{,CSS}Property().

r=dbaron

Maybe leave a comment around the SetString -- though basically all uses of SetString are suspicious (see the FIXME above the method declaration).  Though, given that we already have that FIXME, probably the existing FIXME is good enough.

(I won't ask about enumeration.)
Attachment #804231 - Flags: review?(dbaron) → review+
Comment on attachment 815800 [details] [diff] [review]
Part 21: Factor out style context getting from nsComputedDOMStyle::GetCSSPropertyValue. (v1.1)

Maybe leave a comment above 
nsComputedDOMStyle::UpdateCurrentStyleInformation saying that it reports
failure by leaving mStyleContextHolder null?

And maybe change the names with s/StyleInformation/StyleSources/ ??

r=dbaron
Attachment #815800 - Flags: review?(dbaron) → review+
Comment on attachment 804233 [details] [diff] [review]
Part 22: Expose variables through the nsComputedDOMStyle indexed getter.

Could you put the bits that are corrections to the previous patch (patch
21) there instead of here?  (The FlushPendingLinkUpdates move, and the
added return in GetPropertyCSSValue.)

And, now that you're moving FlushPendingLinkUpdates (in the previous 
patch, now), could you also (again in the previous patch) move the 
|document| variable into UpdateCurrentStyleInformation, and remove the
aDocument argument from it?  

And then, in this patch, you don't need to add the second
UpdateCurrentStyleInformation method.

r=dbaron with that
Attachment #804233 - Flags: review?(dbaron) → review+
Reduced test case for Inspector crashes when inspecting following page:
<style>:root {var-foo:purple;} p {background-color: var(foo);}</style><p>This should be purple</p>

when using win32 try build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cmccormack@mozilla.com-08eaec472d7e/

crash report:
https://crash-stats.mozilla.com/report/index/10c02499-36d8-42d7-99cc-dbefb2131206
While reviewing patch 23 I think I noticed an additional problem in part 2:

ParseVariableDeclaration mutates the declaration before it's certain that the declaration **in its entirety** parsed successfully.  (That's the function of the mData.TransferFromBlock(mTempData,...) for normal property parsing.)

For example, the following:

  var-foo: green;
  var-foo: red);

should result in var-foo having a computed value of 'green', because the second declaration is a syntax error.  But CSSParserImpl::ParseVariableDeclaration will call AddVariableDeclaration as long as ExpectEndProperty is true -- that is, as long as we're at a condition that *might* be a valid endpoint for a property.  (I'd really like to get rid of ExpectEndProperty in the long run; it's fundamentally the wrong way to parse.)

(The RemoveVariableDeclaration in patch 23, in CSSParserImpl::ParseVariable, led me to this.)

To fix this, either ParseVariableDeclaration needs to do more of the checking, or its caller in part 2, ParseDeclaration, needs to do more checking, matching the bits of ParseDeclaration after the ParseProperty call.  So there are calls to ParseVariableDeclaration in patches 2, 23, 24, and 25.  The callers in parts 23 and 24 check that there are no remaining tokens after the variable (i.e., EOF), and the caller in part 25 checks for ')' or EOF.  So this makes me think that the fix belongs in the caller in part 2.

In particular, I think part 2 should be modified so that:
 - ParseVariableDeclaration doesn't modify aDeclaration, but instead returns a (type, value, importance) triplet in some form
 - ParseVariableDeclaration no longer checks ExpectEndProperty() || mToken.IsSymbol('!')
 - ParseDeclaration doesn't return quickly when ParseDeclaration succeds, but instead branches around the remaining code up to the ParseProperty call, and shares the rest (which will involve some branches)

and there should be a test for the above case (which I'm assuming currently fails) and perhaps other similar ones
Comment on attachment 804234 [details] [diff] [review]
Part 23: Support variables in CSSStyleDelcaration methods.

>Bug 773296 - Part 23: Support variables in CSSStyleDelcaration methods.

Typo in the name of the patch, change Delcaration -> Declaration.


>+  // Additionally, if a shorthand property was set using a value with a
>+  // variable reference and at least one of its component longhand properties
>+  // was not then overridden on the declaration, we return the token stream
>+  // assigned to the shorthand.

I think this approach is too lenient; it will return values such that
setting those values back again will cause a change.  We try not to do
that.

I think you should change the code such that if any subproperty is a
token stream, then if all subproperties have a token stream for
aProperty, return that token stream, and otherwise return the empty
string.

nsCSSParser.cpp:

Please leave a blank line between ParseVariable and the #ifdef _MSC_VER
following it.

And CSSParserImpl::ParseVariable needs to be modified to match the
changes from comment 147, which means not having a
RemoveVariableDeclaration call in the failure case, but adding an
AddVariableDeclaration call in the success case.

And, er, leaving status uninitialized in ParseVariableDeclaration
(nsCSSParser.cpp) should probably be fixed in the patch earlier in the
patch stack that created it.


nsDOMCSSDeclaration::GetCustomPropertyValue should perhaps assert about
the "var-" prefix before stripping it?  That is,
+  MOZ_ASSERT(nsCSSProps::IsCustomPropertyName(aPropertyName));

Same for nsDOMCSSDeclaration::RemoveCustomProperty.

r=dbaron with that
Attachment #804234 - Flags: review?(dbaron) → review+
Comment on attachment 816311 [details] [diff] [review]
Part 24: Recognize variables in CSS.supports("var-blah", "whatever") calls. (v1.1)

>+    const nsDependentSubstring varName = Substring(aProperty, 4);  // remove 'var-'

Again, should probably assert that you're removing what you think you 
are.

Also, that line and the next both have 80th-column violations.

And, again, this will need adjusting for comment 147.

r=dbaron
Attachment #816311 - Flags: review?(dbaron) → review+
Oh, and one more thing I meant to say on part 22:  it seems like a spec ought to define the enumeration order.  Could you email www-style about that?
Comment on attachment 804236 [details] [diff] [review]
Part 25: Parse variable declarations in @supports conditions.

>-      if (ExpectSymbol(')', true)) {
>-        UngetToken();
>-        return false;
>-      }

So the problem with removing this is that the @supports grammar requires
a nonempty value here, so an empty value is supposed to be a syntax
error rather than a false condition.  I suppose that's a problem for
variables, though, since they require empty values?  Could you send a
comment to www-style to that effect if that's correct?  And probably
include its URL in the commit message (but not a comment).  I think we
need to loosen the @supports grammar.  Or I could send the comment if
you want; let me know.

At least, assuming that was your intent here; the commit message is a
bit unclear.

>+        RequireWhitespace();  // skip white space before property value

This, on the other hand, is entirely unnecessary.  ParseProperty can
deal with initial whitespace.

>+    const nsDependentSubstring varName = Substring(aProperty, 4);  // remove 'var-'

Again, should probably assert that you're removing what you think you
are.

Also, that line and the next both have 80th-column violations.

And, again, this will need adjusting for comment 147.

>+        if (!aConditionMet) {
>+          SkipUntil(')');
>+          UngetToken();
>+          return false;

This should not return false.  Returning false says that the @supports
condition is syntactically invalid, which it's not.  (For example, it
could have an ! in it.)

Please add a test for this as well.


There's also no need for the CheckEndSupportsCondition call.  If there's
invalid syntax around, something else will lead to a failure.  Please
drop it.

r=dbaron with that
Attachment #804236 - Flags: review?(dbaron) → review+
Comment on attachment 816312 [details] [diff] [review]
Part 26: Tests. (v1.1)

It probably would have been nice if the ones that are mochitests were testharness.js tests, but at this point, don't worry about it.  (Although I guess pref changes would be a problem...)

You'll need to update the layout/style/test/Makefile.in changes to be mochitest.ini changes.

Either remind me to sync the tests to the csswg test repository after you land, or perhaps even do it yourself if you have access.

r=dbaron
Attachment #816312 - Flags: review?(dbaron) → review+
Comment on attachment 816313 [details] [diff] [review]
Part 27: Support 'unset' in CSS variables.

r=dbaron
Attachment #816313 - Flags: review?(dbaron) → review+
(In reply to :Ms2ger from comment #109)
> Comment on attachment 804213 [details] [diff] [review]
> Part 2: Parse CSS variable declarations and store them on Declaration
> objects. p=ebassi,heycam
> 
> Review of attachment 804213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsCSSParser.cpp
> @@ +11252,5 @@
> > +                                        const nsAString& aName,
> > +                                        uint32_t aFlags)
> > +{
> > +  CSSVariableDeclarations::Type type;
> > +  nsString variableValue;
> 
> This one can be declared a little later, I think.

Disagree; these three variables are the things that are computed and passed in to the function call right at the end, so I think it make sense to declare them all together.
(In reply to :Ms2ger from comment #110)
> Comment on attachment 804216 [details] [diff] [review]
> Part 5: Map variables on a Declaration to nsRuleData.
> 
> Review of attachment 804216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/CSSVariableDeclarations.cpp
> @@ +120,5 @@
> > +{
> > +  nsDataHashtable<nsStringHashKey, nsString>* variables =
> > +    static_cast<nsDataHashtable<nsStringHashKey, nsString>*>(aData);
> > +  nsString oldValue;
> > +  if (!variables->Get(aName, &oldValue)) {
> 
> I think you can use Contains().

Can't, but filed bug 947578 so I can.
Depends on: 947578
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #121)
> Comment on attachment 804213 [details] [diff] [review]
> Part 2: Parse CSS variable declarations and store them on Declaration
> objects. p=ebassi,heycam
> 
> >+  // Indexes into ')' characters in |stack| that correspond to "var(".  This
> >+  // is used to stop parsing when we encounter a '!' or ';' at the top level
> >+  // of a variable reference's fallback.
> >+  nsAutoTArray<uint32_t, 16> references;
> 
> The idea of what this (and the code that uses it) is doing doesn't seem
> to be supported by the spec, or at least I can't find it.  It might be
> worth raising, though.  But either way, it should match the spec.
> 
> And the same holds for the code that checks that the thing after var( is
> an ident.
> 
> (I think it's really an ambiguity in the spec -- it seems like the spec
> author hasn't considered whether these errors should be invalid at parse
> time or computed value time.  So I wouldn't say the spec calls for one
> or the other -- but it should.)

I've emailed www-style about this: http://lists.w3.org/Archives/Public/www-style/2013Dec/0148.html
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #129)
> Comment on attachment 816309 [details] [diff] [review]
> Part 8: Resolve and compute CSS variables. (v1.1)
> 
> >+void
> >+CSSVariableResolver::Resolve(const CSSVariableValues* aInherited,
> >+                             const CSSVariableDeclarations* aSpecified)
> >+{
> >+  MOZ_ASSERT(!mResolved);
> >+  MOZ_ASSERT(aInherited);
> >+
> >+  aInherited->AddVariablesToResolver(this);
> >+  if (aSpecified) {
> >+    aSpecified->AddVariablesToResolver(this);
> >+  }
> 
> Why would we bother calling this if aSpecified is null?  In other words,
> if there aren't any specified variable declarations, wouldn't we just
> copy the results from the parent?

Yes and I'm already asserting that in nsRuleNode::ComputeVariablesData, so I can get rid of the null check there.

> Also, how do we ensure aInherited is never null?  What happens at the
> root?  Or does the way ComputeVariablesData works lead to the root
> style context passing aInherited==this?

We probably don't ensure that, but I didn't really consider how easy or not it would be to have variable styles specified on the root.  I guess I should null check it in case we ever do that.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #129)
> Comment on attachment 816309 [details] [diff] [review]
> Part 8: Resolve and compute CSS variables. (v1.1)
> 
> >+          bool valid = aVariables->Get(variableName, variableValue,
> >+                                       varFirstToken, varLastToken) &&
> >+                       !variableValue.IsEmpty();
> 
>   I haven't looked at the spec on this topic, but the IsEmpty check
>   seems weird.  Could you at least add a comment explaining it (and
>   why aVariables->Get()'s return value isn't sufficient)?

Invalid variables due to cycles or references to undefined variables get stored explicitly as empty strings by the resolver.  Declarations like |var-a:;| get thrown out earlier, during parsing.  I'll add a comment.

>   The need for so many StartRecording calls in different places
>   seems a bit awkward, but I don't immediately see a better solution.
> 
> >+              // Now we're either at the pushed back ')' that finished the
> >+              // fallback or at EOF.
> >+              GetToken(false);
> 
>   An assertion probably wouldn't be a bad idea (DebugOnly<bool> would
>   be needed for the result of GetToken).
> 
> >+      default:
> >+        UPDATE_RECORDING_TOKENS(eCSSTokenSerialization_Other);
> 
>   An assertion about what token types are left would probably be a good
>   idea.
> 
>   I don't understand how things work when |value| is appended so late.
>   It seems to me like it needs to be appended much closer to the
>   var() function that uses it.  (And what if there are two var()
>   functions inside a variable?)  If I'm right that this is wrong, please
>   add tests that cover it.

I'm not sure what you mean.  When we encounter a var() function, we either successfully get its value and append it to |value|, or we use and resolve the fallback, in which case we append that resolved fallback to |value|.  By the end of the function, when we append |value| to |aResult|, it should contain the entire resolved value.
(In reply to Cameron McCormack (:heycam) from comment #158)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #129)
> > Comment on attachment 816309 [details] [diff] [review]
> > Part 8: Resolve and compute CSS variables. (v1.1)

> >   I don't understand how things work when |value| is appended so late.
> >   It seems to me like it needs to be appended much closer to the
> >   var() function that uses it.  (And what if there are two var()
> >   functions inside a variable?)  If I'm right that this is wrong, please
> >   add tests that cover it.
> 
> I'm not sure what you mean.  When we encounter a var() function, we either
> successfully get its value and append it to |value|, or we use and resolve
> the fallback, in which case we append that resolved fallback to |value|.  By
> the end of the function, when we append |value| to |aResult|, it should
> contain the entire resolved value.

I think I was confused, and thinking that you were interleaving things that appended to aResult with things that appended to value; looking now, I don't see that.  That said, now I'm wondering why value needs to be a separate variable, distinct from aResult.
(In reply to Cameron McCormack (:heycam) from comment #157)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #129)
> > Comment on attachment 816309 [details] [diff] [review]
> > Part 8: Resolve and compute CSS variables. (v1.1)

> > Also, how do we ensure aInherited is never null?  What happens at the
> > root?  Or does the way ComputeVariablesData works lead to the root
> > style context passing aInherited==this?
> 
> We probably don't ensure that, but I didn't really consider how easy or not
> it would be to have variable styles specified on the root.  I guess I should
> null check it in case we ever do that.

Also, having variables set on the root element seems like it'll be pretty common, and is easy to test.  I think the null-check probably isn't needed because the caller passes parentVariables->mVariables (see COMPUTE_START_INHERITED), which on the root will end up meaning that both parameters to Resolve are the same.  It's probably worth adding a comment pointing that out, though.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #159)
> I think I was confused, and thinking that you were interleaving things that
> appended to aResult with things that appended to value; looking now, I don't
> see that.  That said, now I'm wondering why value needs to be a separate
> variable, distinct from aResult.

I think I was trying to avoid writing in to aResult unless the function was going to succeed and return true.  That's probably not really necessary, though.

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #160)
> Also, having variables set on the root element seems like it'll be pretty
> common, and is easy to test.  I think the null-check probably isn't needed
> because the caller passes parentVariables->mVariables (see
> COMPUTE_START_INHERITED), which on the root will end up meaning that both
> parameters to Resolve are the same.  It's probably worth adding a comment
> pointing that out, though.

Isn't the case we're worried about when styles are being computed for the root of the rule tree, not the root element?  Good point about parentVariables == variables in this case, though.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #131)
> Comment on attachment 804221 [details] [diff] [review]
> Part 10: Add a new eCSSUnit_TokenStream type for storing unparsed CSS
> values. p=ebassi,heycam
> 
> I'm also curious why you need to store the longhand property ID.

I had thought that it wasn't possible to know what property you would be re-parsing at computed value time, but I guess in the nsRuleNode::ResolveVariableReferences loop you can converts those offsets into aRuleData->mValueStorage into property IDs.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #133)
> Comment on attachment 804223 [details] [diff] [review]
> Part 12: Record whether we are parsing an @supports condition.
> 
> I'd slightly prefer AutoRestore (or using the AutoToggle patch in bug
> 518756).

I think I considered that but due to it being a bitfield (and thus not being able to use one of the existing auto restorer classes), writing a whole separate class just for saving/restoring the value within the one function seemed like overkill.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #134)
> Comment on attachment 804224 [details] [diff] [review]
> Part 13: Parse properties that use variable references and store them as
> eCSSUnit_TokenStream values.
> 
> >+      const PRUnichar stopChars[] = { ';', '!', '}', ')', 0 };
> >+      SkipUntilOneOf(stopChars);
> >+      UngetToken();
> >+      parseAsTokenStream = mScanner->SeenVariableReference();
> 
> This is a bit suspicious, though maybe it's ok.
> 
> At the very least, we never actually stay at the position resulting
> from this SkipUntil, so it's only used for variable checks.

I don't understand what your suspicion is, can you be more specific?
(In reply to Cameron McCormack (:heycam) from comment #161)
> I think I was trying to avoid writing in to aResult unless the function was
> going to succeed and return true.  That's probably not really necessary,
> though.

I guess it's also probably harmless given string buffer sharing.

(In reply to Cameron McCormack (:heycam) from comment #161)
> Isn't the case we're worried about when styles are being computed for the
> root of the rule tree, not the root element?  Good point about
> parentVariables == variables in this case, though.

No, it should be the root element, or any other element that doesn't inherit from some other element (i.e., its style context has no parent).

(In reply to Cameron McCormack (:heycam) from comment #164)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #134)
> > Comment on attachment 804224 [details] [diff] [review]
> > Part 13: Parse properties that use variable references and store them as
> > eCSSUnit_TokenStream values.
> > 
> > >+      const PRUnichar stopChars[] = { ';', '!', '}', ')', 0 };
> > >+      SkipUntilOneOf(stopChars);
> > >+      UngetToken();
> > >+      parseAsTokenStream = mScanner->SeenVariableReference();
> > 
> > This is a bit suspicious, though maybe it's ok.
> > 
> > At the very least, we never actually stay at the position resulting
> > from this SkipUntil, so it's only used for variable checks.
> 
> I don't understand what your suspicion is, can you be more specific?

Considering what might be after a construct, rather than considering what is valid or not in the construct, is generally suspicious in a recursive descent parser, in my experience.  It's certainly led to a bunch of bugs in our CSS parser.  For example, see the problem I describe in my review comment in comment 147, and my general distaste for any calls to ExpectEndProperty().  (Implementing things using the approach in css3-syntax, where there are multiple levels of parsing, is probably the easiest way to avoid the need for most of these calls, though I think they could all be eliminated even without that.)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #150)
> Oh, and one more thing I meant to say on part 22:  it seems like a spec
> ought to define the enumeration order.  Could you email www-style about that?

I asked about that here: http://lists.w3.org/Archives/Public/www-style/2013Aug/0591.html
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #151)
> Comment on attachment 804236 [details] [diff] [review]
> Part 25: Parse variable declarations in @supports conditions.
> 
> >-      if (ExpectSymbol(')', true)) {
> >-        UngetToken();
> >-        return false;
> >-      }
> 
> So the problem with removing this is that the @supports grammar requires
> a nonempty value here, so an empty value is supposed to be a syntax
> error rather than a false condition.  I suppose that's a problem for
> variables, though, since they require empty values?

White space only values are allowed, but completely empty values are disallowed (and would be a syntax error).  I'll replicate the ExpectSymbol call into the three branches in here, passing false for aSkipWS for the variable case and true for the two other cases.

I think the problem lies in http://dev.w3.org/csswg/css-syntax/#parse-a-declaration which doesn't allow only white space after the colon.  Sent mail: 

  http://lists.w3.org/Archives/Public/www-style/2013Dec/0156.html
Attachment #8344456 - Attachment description: a.patch → Part 28: Avoid some crashes in the inspector when variables are present.
Attachment #8344456 - Flags: review?(dbaron)
Comment on attachment 8344456 [details] [diff] [review]
Part 28: Avoid some crashes in the inspector when variables are present.

Since roc is awake..
Attachment #8344456 - Flags: review?(dbaron) → review?(roc)
test_alerts_noobserve.html needs some tweaking to avoid some random failures in test_multiple_alerts.html, which my patch queue seems to surface.
Depends on: 948782
Missed a case where nsCSSValues for transforms in OMTA handling which needs to switch from nsCSSValueList to using nsCSSValueSharedList.  Also missed an initialization in nsCSSValueSharedList.
Attachment #8345673 - Flags: review?(roc)
One more try run with this fix and the bug 948782 fix: https://tbpl.mozilla.org/?tree=Try&rev=b2e1649ffd0d
And another one with an updated bug 948782 fix (looking better this time): https://tbpl.mozilla.org/?tree=Try&rev=402b688c3e47
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ceff347849d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e4e9b6cf6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7069d52b1e60
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1b73fffa77
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eae39ddcb77
https://hg.mozilla.org/integration/mozilla-inbound/rev/65d7e8f84978
https://hg.mozilla.org/integration/mozilla-inbound/rev/407ca304dcda
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08b6b8c6eca
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c381791e1a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/adaae40774cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/246d623b5259
https://hg.mozilla.org/integration/mozilla-inbound/rev/88fd50578a76
https://hg.mozilla.org/integration/mozilla-inbound/rev/27256a46adbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/39833de60ed7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec31b9795a5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5acf7d3cd8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c576c10f4a17
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aab83be781e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e44cc02cb44d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89737efe95f
https://hg.mozilla.org/integration/mozilla-inbound/rev/71760ec285cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8348447eea67
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d59284ef0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dcc56329eb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c650b9f764
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7460cd4b12f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f79faac8a087
https://hg.mozilla.org/integration/mozilla-inbound/rev/da1f0d2737ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/1390f263b8ce
https://hg.mozilla.org/mozilla-central/rev/6ceff347849d
https://hg.mozilla.org/mozilla-central/rev/e6e4e9b6cf6a
https://hg.mozilla.org/mozilla-central/rev/7069d52b1e60
https://hg.mozilla.org/mozilla-central/rev/6d1b73fffa77
https://hg.mozilla.org/mozilla-central/rev/8eae39ddcb77
https://hg.mozilla.org/mozilla-central/rev/65d7e8f84978
https://hg.mozilla.org/mozilla-central/rev/407ca304dcda
https://hg.mozilla.org/mozilla-central/rev/d08b6b8c6eca
https://hg.mozilla.org/mozilla-central/rev/6c381791e1a1
https://hg.mozilla.org/mozilla-central/rev/adaae40774cb
https://hg.mozilla.org/mozilla-central/rev/246d623b5259
https://hg.mozilla.org/mozilla-central/rev/88fd50578a76
https://hg.mozilla.org/mozilla-central/rev/27256a46adbb
https://hg.mozilla.org/mozilla-central/rev/39833de60ed7
https://hg.mozilla.org/mozilla-central/rev/ec31b9795a5d
https://hg.mozilla.org/mozilla-central/rev/f5acf7d3cd8f
https://hg.mozilla.org/mozilla-central/rev/c576c10f4a17
https://hg.mozilla.org/mozilla-central/rev/0aab83be781e
https://hg.mozilla.org/mozilla-central/rev/e44cc02cb44d
https://hg.mozilla.org/mozilla-central/rev/e89737efe95f
https://hg.mozilla.org/mozilla-central/rev/71760ec285cf
https://hg.mozilla.org/mozilla-central/rev/8348447eea67
https://hg.mozilla.org/mozilla-central/rev/67d59284ef0c
https://hg.mozilla.org/mozilla-central/rev/2dcc56329eb4
https://hg.mozilla.org/mozilla-central/rev/d7c650b9f764
https://hg.mozilla.org/mozilla-central/rev/d7460cd4b12f
https://hg.mozilla.org/mozilla-central/rev/f79faac8a087
https://hg.mozilla.org/mozilla-central/rev/da1f0d2737ea
https://hg.mozilla.org/mozilla-central/rev/1390f263b8ce
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Blocks: 947242
I'll try to fix this performance regression before backing out the patches: https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/Kt6dp07ZA3w
Looks like part 17 caused the tcanvasmark regression, and I'm thinking it's because of the nsRuleNode::ResolveVariableReferences calls we now do for each style struct we compute.
I suppose a relnote will be added?
relnote-firefox: --- → ?
Depends on: 950264
Keywords: feature
Whiteboard: [enabled for non-release builds only]
(In reply to Cameron McCormack (:heycam) from comment #178)
> Looks like part 17 caused the tcanvasmark regression, and I'm thinking it's
> because of the nsRuleNode::ResolveVariableReferences calls we now do for
> each style struct we compute.

Seems worth testing that theory; it seems a little odd to me given that ResolveVariableReferences should, when there are no variable references, be cheaper than CheckSpecifiedProperties, which we call for each rule node we walk past.
Yes, after some more try runs I don't think it is the ResolveVariableReferences call (or at least, it isn't where most of the regression comes from).  I'll investigate further in bug 950264.
Depends on: 950436
Depends on: 950381
Depends on: 950497
Depends on: 951255
Depends on: 952338
Depends on: 955913
Whiteboard: [enabled for non-release builds only] → [enabled for non-release builds only][DocArea=CSS]
Depends on: 969961
>[enabled for non-release builds only]
So when will this make it into stable builds?
Bug 957833 is for making this available in Release/Beta.  I think bug 950436 needs to be fixed first though.
Blocks: 957833
Flags: in-testsuite+
Whiteboard: [enabled for non-release builds only][DocArea=CSS] → [enabled for non-release builds only][DocArea=CSS][qa-]
Depends on: 985838
It seems that there will be some changes in spec:

http://dev.w3.org/csswg/css-variables/#changes

A variable is now declared `--foo: bar` and used as `var(--foo)`
(In reply to vincent from comment #184)
> A variable is now declared `--foo: bar` and used as `var(--foo)`

That's being done in bug 985838.
Whiteboard: [enabled for non-release builds only][DocArea=CSS][qa-] → [enabled by default in 31][DocArea=CSS][qa-]
Sylvestre, this is a feature that is only enabled in release builds (beta/release channel) and so it's preffed off in Beta. Could you update the release notes to indicate that it's behind the layout.css.variables.enabled preference or remove the mention altogether for Beta?

http://www.mozilla.org/en-US/firefox/29.0beta/releasenotes/
Flags: needinfo?(sledru)
Matthew, I just removed it from the release note. However, if you want to highlight this feature, we can write a better release note for this item
Flags: needinfo?(sledru)
Depends on: 993264
Depends on: 1031344
Blocks: css3test
Version: unspecified → Trunk
Depends on: 1046140
Depends on: 1105938
No longer depends on: 950381
Depends on: 1300237
You need to log in before you can comment on or make changes to this bug.