1363349 - stylo: Gecko forbids calc() in -webkit-gradient, stylo doesn't

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P5)

Description

[Anthony Ramine [:nox]]

The code for legacy -webkit-gradient() in Gecko forbid calc() where <number> or <percentage> is expected, this shouldn't be the case and neither Safari nor Chrome do that.

It is trivial to support calc() there in Stylo, so the tests will need to be fixed when it gets support for this legacy syntax.

Comment 2

[Bobby Holley (:bholley)]

Is this something we can easily ship in Gecko before 57?

If not, we probably still want to disable it for stylo (for now) to reduce risk, and then turn it on in 58. Even something trivial like this has the small possibility to break websites, and we don't want to have to turn off stylo because of that.

How easy would it be to make it conditional on feature = "servo"?

NI xidorn for the first question, nox for the second.

Comment 3

[Anthony Ramine [:nox]]

It is a PITA to disable it in Servo, because in Servo our parsing code is not all over the place and Number::parse, NumberOrPercentage::parse and friends all just support calc() transparently as they should.

Btw the new ticket title makes it sound like stylo currently forbid calc() in -webkit-gradient.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

I don't think it would be particular hard. We probably just need to add something to the parser, but I'm not completely sure without actually trying to implement it.

dholbert knows our current impl of -webkit-gradient better and may have idea whether adding that support is easy.

Comment 5

[Daniel Holbert [:dholbert]]

I don't expect it'd be hard to add calc() support to our -webkit-gradient impl, and I don't expect there'd be much compat risk from that change, given that Chrome/Safari implement that syntax.

Comment 6

[Daniel Holbert [:dholbert]]

So I think there are two parts here:

(A) Update ParseWebkitGradientPointComponent [1] to accept calc() expressions. I don't recall exactly how we parse calc() functions, but we'd presumably want to call out to that code from ParseWebkitGradientPointComponent.  Probably just in a new clause for "} else if (mToken.mType == eCSSToken_Function) {" in the existing mToken.mType cascade in that function.

(B) Adjust the code that shoehorns our parsed -webkit-gradient syntax into a modern/unprefixed linear-gradient or radial-gradient expression, so that it can handle calc() values and do *something* useful with them.  That code is called FinalizeLinearWebkitGradient()[2] and FinalizeRadialWebkitGradient() [3].  (We only support -webkit-graident at parse time, and we encode it into an approximately-equivalent modern unprefixed gradient under the hood, so that we don't have to bother messing with our rendering pipeline for expressiveness of a nonstandard feature that sites may not really care about that much. See bug 1241623 comment 12 for more on that.)


I probably won't be able to get to this for a few weeks (on PTO right now, on a work week next week, & will likely be recovering from review backlog the week after that).  So if anyone wants to give this a shot, feel free! In the meantime, as noted in comment 5, I don't think we need Servo to be bug-compatible with Gecko on what bits of -webkit-gradient it doesn't support.

[1] https://dxr.mozilla.org/mozilla-central/rev/b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/layout/style/nsCSSParser.cpp#10587
[2] https://dxr.mozilla.org/mozilla-central/rev/b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/layout/style/nsCSSParser.cpp#10851
[3] https://dxr.mozilla.org/mozilla-central/rev/b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/layout/style/nsCSSParser.cpp#10924

Comment 7

[Bobby Holley (:bholley)]

(In reply to Daniel Holbert [:dholbert] (AFK May 3-8, 13-21) from comment #6)
> 
> I probably won't be able to get to this for a few weeks (on PTO right now,
> on a work week next week, & will likely be recovering from review backlog
> the week after that).  So if anyone wants to give this a shot, feel free! In
> the meantime, as noted in comment 5, I don't think we need Servo to be
> bug-compatible with Gecko on what bits of -webkit-gradient it doesn't
> support.

Can you clarify whether you mean you think it's fine to ship this behavior change alongside stylo, or whether you just mean it's fine to take some weeks of delay on aligning Gecko ("in the mean time")?

So far, we've managed to avoid any intention behavior changes in stylo AFAIK, so I'd like to preserve that if it isn't too much trouble. But I'm open to the argument that it's a low-enough risk that the extra time investment isn't worth it. NI dbaron for his opinion as well.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> Can you clarify whether you mean you think it's fine to ship this behavior
> change alongside stylo, or whether you just mean it's fine to take some
> weeks of delay on aligning Gecko ("in the mean time")?

I was saying that, IMO, it'd be fine to ship this as a behavior-change (i.e. an improvement) with Stylo, as long as it works reasonably.

I agree that it'd be good to keep Stylo/Gecko behaving the same. But this particular case is an edge case, within a not-intended-to-be-perfect-in-the-first-place compatibility feature (-webkit-gradient). And it's an edge case that we're not aware of any real-world web content actually exercising.  (I expect usage of this combination to be rare, in part because this is combining a 2008-era[1] prefixed legacy WebKit feature with an *unprefixed* feature that wasn't available as such until 2013 [2].)

So it'd be nice to fix this in Gecko up-front, but it also doesn't seem especially high-priority to me (and realistically the earliest I personally could get to this wouldn't be for a few weeks, given vacation/travel, & I don't know if anyone else has cycles/interest to take it).

And the compat risk of a Gecko/Stylo difference here doesn't seem great enough to bother crippling Stylo to match Gecko here, particularly if that's hard to do (per comment 3).

[1] https://webkit.org/blog/175/introducing-css-gradients/
[2] https://bugs.webkit.org/show_bug.cgi?id=91951

Comment 9

[Daniel Holbert [:dholbert]]

AFAIK we don't yet have a testcase here. I came up with one to make things a bit more concrete:
  https://jsfiddle.net/tbsz7rvj/

From my testing, it seems Chrome does not universally support calc() within -webkit-gradient. It only supports it as follows:
 - In the <point> syntax, it supports number+number and percent+percent (but not mixed)
 - In the <radius> syntax, it supports number+number (no percentages allowed there)
 - In the <stop> syntax, it does not support calc() at all (despite the fact that <stop> takes a percent or a number as the stop offset).

:nox, does Servo match Chrome on which syntaxes it accepts/rejects from that testcase?

Comment 10

[Anthony Ramine [:nox]]

- <point> uses <number-or-percentage>, that never allows mixed numbers and percentages.
- <radius> uses <number>, which never allows percentages at all.
- <stop> currently uses <number-or-percentage>, which transparently allows calc().

I guess I need to fix the third case.

Comment 11

[Daniel Holbert [:dholbert]]

(As one other data point: Edge 15 rejects all of the calc() syntaxes in my jsfiddle from comment 9, just like Firefox/Gecko does right now.)

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> So far, we've managed to avoid any intention behavior changes in stylo
> AFAIK, so I'd like to preserve that if it isn't too much trouble. But I'm
> open to the argument that it's a low-enough risk that the extra time
> investment isn't worth it. NI dbaron for his opinion as well.

It seems reasonably low-risk, but it's not zero.  It's probably worth gathering a list of known behavior changes somewhere, probably in a bug -- since the behavior changes should end up documented on MDN (via dev-doc-needed keyword).

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

It doesn't seem to me any failures in test_value_storage.html is related to this. Removing it from blocks.

Comment 14

[Daniel Holbert [:dholbert]]

Per discussion with nox yesterday, I'm going to steal this bug and assign it to intern Kevin Hsieh (who starts today), once Kevin's got a bugzilla account set up.


CODE CHANGES:
=============
I think there are two parts here:

 (1) You'll need to modify "ParseWebkitGradientPointComponent" in nsCSSParser.cpp -- specifically, you probably want to rip out the current Number + Percent checks and replace them with a broader call to "ParseVariant()".  You'll want to pass in VARIANT_NP | VARIANT_CALC for ParseVariant's second parameter (aVariantMask), to allow numbers + percentages + calc; and you'll want to pass in nullptr for the third "keyword-table" argument, since you won't be using this call to handle keyword-parsing.  The code in question is her:
https://dxr.mozilla.org/mozilla-central/rev/95543bdc59bd038a3d5d084b85a4fec493c349ee/layout/style/nsCSSParser.cpp#10649-10654

 (2) You'll need to modify the code that *processes* the parsed -webkit-gradient value (and produces an approximately equivalent modern gradient) so that it can do something useful with the parsed calc() expression. The functions here are "FinalizeLinearWebkitGradient" and "FinalizeRadialWebkitGradient".  We can discuss this after you've dived into this a bit -- the exact behavior doesn't matter a lot as long as it's somewhat reasonable -- see the comments in those files.

 (3) You should be able to add some "valid" and "invalid" examples that involve calc(), alongside the existing "-webkit-gradient(...)" listings near the top of this file:
https://dxr.mozilla.org/mozilla-central/rev/95543bdc59bd038a3d5d084b85a4fec493c349ee/layout/style/test/property_database.js

That provides a basic level of verification that our parser accepts/rejects values that we expect it to.

You prooooobably should also add some tests here:
https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_specified_value_serialization.html
https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_value_computation.html
...alongside (or as a copy of) some of the existing -webkit gradient tests, to test *what* we serialize these values as. (since we convert -webkit-gradient(...) into simpler modern gradients when parsing)

Comment 15

[Kevin Hsieh]

Attachment: bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review commit: https://reviewboard.mozilla.org/r/151826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151826/

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

https://reviewboard.mozilla.org/r/151826/#review156880

Here's a partial review (I still need to look at the CSSCalc.h changes).  Mostly small stuff -- this is pretty close!

::: layout/style/nsCSSParser.cpp:10515
(Diff revision 1)
> +  // Attempts to use ParseVariant to process the token. Failing that, it may be
> +  // a keyword or unknown token, in which case we execute the rest of the
> +  // function.
> +  if (ParseVariant(aComponent, VARIANT_PN | VARIANT_CALC, nullptr) ==
> +      CSSParseResult::Ok) {

If ParseVariant returns CSSParseResult::Error here, we need to bail out and return false, rather than proceeding with the rest of the function (where we try to do some more parsing) -- because CSSParseResult::Error means we've already stepped past some badness that we failed at parsing, and we don't want to just pick up and keep parsing more pieces of this same value after we've skipped over some badness.

So -- please capture the result of this "ParseVariant" call in a local variable (of type CSSParseResult), and add an early "return false" if it's ::Error, and then after that early-return, you can move onto this ::OK comparison (now comparing against the new local variable) and the rest of the function.

::: layout/style/nsCSSParser.cpp:10542
(Diff revision 1)
> +        MOZ_ASSERT(false, "Position could not be parsed as Number, Calc, "
> +                          "or Percent");

This assertion-message isn't quite describing its real justification.  (It sounds like it's just complaining that the web page had some unexpected tokens here -- and that's a fine and not-assert-worthy thing.)

There *is something* assert-worthy here, though -- it's that ParseVariant did something unexpected.  It "succeeded" but returned a parsed thing of an unexpected type.

So, how about we change this message to more clearly describe that scenario -- maybe:
  "ParseVariant returned value with unexpected unit"

::: layout/style/nsCSSParser.cpp:10638
(Diff revision 1)
> -    return false;
> -  }
> +  if (ParseVariant(parseResult, VARIANT_NUMBER | VARIANT_CALC, nullptr) ==
> +      CSSParseResult::Ok) {

Could you invert the logic of this first if-check here, so that we handle parse-failure with an early-return? (This lets us de-indent the rest of the function, which helps readability a bit -- we tend to prefer handling failure via early-returns for this reason, as long as it doesn't make the code clumsier/longer.)


So, I think this should look like:
  if (!ParseVariant(...
                    nullptr) != CSSParseResult::OK) {
    return false;
  }

  switch(...) {
    ...
  }

  return true;

::: layout/style/nsCSSParser.cpp:10645
(Diff revision 1)
> +        ReduceCalcOps<eCSSUnit_Number>().
> +          ComputeCoefficient(aRadius, parseResult);

At least in C++ code, Mozilla coding style doesn't typically wrap after a "." -- it's better to restructure things so that the statement is shorter, if possible (e.g. using a helper variable).

It looks like other usages of these CalcOps classes tend to declare a helper variable, too. So, let's follow that convention here.  (For this to work, I think you'll need to add braces around the switch-case body, too).

Like so:
      case eCSSUnit_Calc: {
        ReduceCalcOps<eCSSUnit_Number> ops;
        ops.ComputeCoefficient(...);
        break;
      }

::: layout/style/nsCSSParser.cpp:10649
(Diff revision 1)
> +      case eCSSUnit_Calc:
> +        ReduceCalcOps<eCSSUnit_Number>().
> +          ComputeCoefficient(aRadius, parseResult);
> +        break;
> +      default:
> +        MOZ_ASSERT(false, "Radius could not be parsed as Number or Calc");

As above, this needs a slight rewording. Right now it sounds like it might fire if a web page simply provided some broken CSS.  (But really, it'll only fire if ParseVariant "succeeds" with a value of a type that we didn't ask it for.)

::: layout/style/test/property_database.js:590
(Diff revision 1)
> +    // Linear-gradient with calc expression (bug 1363349)
> +    "-webkit-gradient(linear, calc(5 + 5%) top, calc(10 + 10) top, from(blue), to(lime))",
> +    "-webkit-gradient(linear, left calc(25 - 10%), right calc(75% + 10%), from(blue), to(lime))",
> +

Please include some invalid radial gradients here, too.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

https://reviewboard.mozilla.org/r/151826/#review156918

::: layout/style/CSSCalc.h:468
(Diff revision 1)
>  /**
> + * ReduceCalcOps is a CalcOps implementation for pure percent or pure
> + * number/float calc() (sub-)expressions, input as nsCSSValues.
> + * For example, nsCSSParser::ParseCalcMultiplicativeExpression uses it to
> + * simplify numeric sub-expressions in order to check for division-by-zero.
> + */
> +template<nsCSSUnit unit>
> +struct ReduceCalcOps : public mozilla::css::BasicFloatCalcOps,
> +                       public mozilla::css::CSSValueInputCalcOps
> +{
> +  bool ComputeLeafValue(result_type& result, const nsCSSValue& aValue)
> +  {

If possible, I'd rather avoid creating this special percent-or-number-with-boolean-return-values alternate CalcOps class (and its associated ComputeCalc2 reduction function).  All of this CalcOps stuff is already pretty confusing, and this seems to be creating another point of incompatibility (where you have to know whether to use the boolean-returning "fallible" methods or not).

Instead of this, could we just use the existing ReduceNumberCalcOps and a new similar-looking class called "ReducePercentCalcOps"?

These would be stricter than your code is here (e.g. ReduceNumberCalcOps will fatally assert if it encounters a percentage).  To make that strictness OK [and for things to work in general with these two separate CalcOps classes), you'll want to do a first-pass traversal over the calc() expression's possibly-nested contents to discover whether it is entirely-numbers (in which case, ReduceNumberCalcOps) or entirely-percent (in which case, ReducePercentCalcOps) or a mixture (in which case, we'd want to bail & return false without instantiating any CalcOps at all).

::: layout/style/test/property_database.js:412
(Diff revision 1)
>      // Linear-gradient with unitless-0 <angle> (normally invalid for <angle>
>      // but accepted here for better webkit emulation):
>      "-webkit-linear-gradient(0, red, blue)",
>  
> +    // Linear-gradient with calc expression (bug 1363349)
> +    "-webkit-gradient(linear, calc(5 + 5) top, calc(10 + 10) top, from(blue), to(lime))",

Please include some "valid" values here that include multiplication or division, like:
   calc(10 * 0)
   calc(2% / 5)

This will verify that any "pure percent" checks are correctly excluding numeric coefficients (as they should).

::: layout/style/test/property_database.js:590
(Diff revision 1)
> +    // Linear-gradient with calc expression (bug 1363349)
> +    "-webkit-gradient(linear, calc(5 + 5%) top, calc(10 + 10) top, from(blue), to(lime))",
> +    "-webkit-gradient(linear, left calc(25 - 10%), right calc(75% + 10%), from(blue), to(lime))",

Please:
 (1) include a divide by zero calc expression here, like calc(5% / 0) (which should be rejected, hopefully automagically & not by anything special you have to add)


 (2) move this whole chunk up a few lines to insert it right here:
http://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/layout/style/test/property_database.js#560
... to fit better into the existing groupings here (the surrounding code uses a bulleted list to categorize the different types of invalid values into sub-categories -- I think this fits best into the first category where we're not saying it's valid for any of the other types of gradient expressions).

(3) Adjust the comment to clarify what specifically *makes this* invalid. (It's not just "with calc expression" that makes it invalid, because those will be accepted now.  Maybe say "with calc expression mixing percent and number", and "with divide by 0 in calc expression" for the other invalid testcase that I suggested above)

Comment 18

[Kevin Hsieh]

Attachment: bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

Review commit: https://reviewboard.mozilla.org/r/152324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152324/

Comment 19

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/1-2/

Comment 20

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/2-3/

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

https://reviewboard.mozilla.org/r/152324/#review157382

::: layout/style/CSSCalc.h:71
(Diff revision 1)
>   * (The CalcOps in the CSS parser that reduces purely numeric
>   * expressions in turn calls ComputeCalc on numbers; other ops can
>   * presume that expressions in the coefficient positions have already been
>   * normalized to a single numeric value and derive from, if their coefficient
>   * types are floats, FloatCoeffsAlreadyNormalizedCalcOps.)
> + * 

EOL whitespace

::: layout/style/CSSCalc.h:85
(Diff revision 1)
>   *   MergeMultiplicativeL for Times_L (coeff * value)
>   *   MergeMultiplicativeR for Times_R (value * coeff) and Divided
>   */
>  template <class CalcOps>
> -static typename CalcOps::result_type
> -ComputeCalc(const typename CalcOps::input_type& aValue, CalcOps &aOps)
> +static bool
> +ComputeCalc(typename CalcOps::result_type& result,

s/result/aResult/

("a" = "arg" -- mozilla coding convention, to distinguish local variables from arguments.)

This affects a number of places in this patch -- every "coeff_type& result" and "result_type& ret"

::: layout/style/CSSCalc.h:368
(Diff revision 1)
> -struct ReduceNumberCalcOps : public mozilla::css::BasicFloatCalcOps,
> -                             public mozilla::css::CSSValueInputCalcOps
> +template<typename type, nsCSSUnit unit>
> +struct ReduceCalcOps : public mozilla::css::CSSValueInputCalcOps
>  {
> -  result_type ComputeLeafValue(const nsCSSValue& aValue)
> +  typedef type result_type;
> +  typedef type coeff_type;
> +  

EOL whitespace

::: layout/style/CSSCalc.h:398
(Diff revision 1)
> +      return aValue1 * aValue2;
> +    }
> +    MOZ_ASSERT(aCalcFunction == eCSSUnit_Calc_Divided, "unexpected unit");
> +    return aValue1 / aValue2;
> +  }
> +  

EOL whitespace

::: layout/style/nsCSSParser.cpp:13629
(Diff revision 1)
> -      mozilla::css::ReduceNumberCalcOps ops;
> -      float number = mozilla::css::ComputeCalc(*storage, ops);
> +      float number;
> +      mozilla::css::ReduceCalcOps<float, eCSSUnit_Number> ops;
> +      MOZ_ASSERT(mozilla::css::ComputeCalc(number, *storage, ops),
> +                 "unexpected unit");
>        if (number == 0.0 && afterDivision)
>          return false;
>        storage->SetFloatValue(number, eCSSUnit_Number);

MOZ_ASSERT only evaluates the condition that you give it in debug builds (so that you could e.g. do MOZ_ASSERT(SomeExpensiveCheck()) without having to worry about slowing down optimized builds for no gain).

(This is true of nearly all mozilla assertion types, except for the ones that explicitly mention "runtime" or something like that their name.)

So: you'll need to pull ComputeCalc out of the assertion here, and capture its return value in a local variable (e.g. "bool success")  This is true for every MOZ_ASSERT(...ComputeCalc(...)) expression in this patch.

Also: are we justified in asserting that ComputeCalc *must* succeed, in all of these places?  (What if variantMask has VARIANT_NUMBER | VARIANT_PERCENT -- would our ReduceCalcOps::ComputeCalc call fail in that case?)

Comment 22

[Kevin Hsieh]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152324/diff/1-2/

Comment 23

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/3-4/

Comment 24

[Kevin Hsieh]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152324/diff/2-3/

Comment 25

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/4-5/

Comment 26

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/5-6/

Comment 27

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/6-7/

Comment 28

[Kevin Hsieh]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152324/diff/3-4/

Comment 29

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/7-8/

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

https://reviewboard.mozilla.org/r/151826/#review157996

[not quite done with review; bouncing between various things this week and need to think a bit more about Part 1 here. No need to post updated patches here until I've gotten through that, if you like. I'll flush this small batch of comments for the moment, though]

::: layout/style/nsCSSParser.cpp:10520
(Diff revision 8)
> +  if (status == CSSParseResult::Error) {
> +    return false;
> +  }
> +  else if (status == CSSParseResult::Ok) {

Two Gecko style notes:
 (1) Generally, "else" should go on same line as "}"
 (2) ...*but* in this case, just drop all three "else" keywords in this chunk of added code -- they're unnecessary, because they immediately follows a "return" statement.
( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices )

So: please just drop this "else" keyword and the two that follow it.

::: layout/style/nsCSSParser.cpp:10530
(Diff revision 8)
> +        ReduceCalcOps<float, eCSSUnit_Number> ops_number;
> +        ReduceCalcOps<float, eCSSUnit_Percent> ops_percent;
> +        if (ComputeCalc(result, aComponent, ops_number)) {
> +          aComponent.SetFloatValue(result, eCSSUnit_Pixel);
> +          return true;
> +        }
> +        else if (ComputeCalc(result, aComponent, ops_percent)) {

(1) Please camelcase for variables ("opsNumber", "opsPercent")

(2) Please declare ops_percent further down, just before it's used (which you can do now that the "else" will be going away here).

  ReduceCalcOps ... opsNumber;
  if (ComputeCalc(...)) {
  }

  ReduceCalcOps ... opsNercent;
  if (ComputeCalc(...)) {
  }

::: layout/style/nsCSSParser.cpp:10651
(Diff revision 8)
> +    case eCSSUnit_Calc: {
> +      ReduceCalcOps<float, eCSSUnit_Number> ops;
> +      return ops.ComputeCoefficient(aRadius, parseResult);

I think this should be ComputeCalc, not ComputeCoefficient.

(They're probably equivalent in practice, since our calc() happens to be number-only here.  But semantically, we're working with a toplevel calc() expression here, not with a coefficient.)

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

https://reviewboard.mozilla.org/r/152324/#review159984

Sorry for the long lag time here! I've been slacking & putting this off, in part because the various typedefs and templates in this code make my mind hurt a bit (which is to say, thanks for being willing to dive into this)!

Review comments on the CSSCalc refactoring patch below. This isn't quite r+ -- it'll merit another look once you've addressed this feedback -- but it'll likely be ready to land at that point, I think.

::: commit-message-2403c:1
(Diff revision 4)
> +bug 1363349 (part 1) - refactor layout/style/CSSCalc.h r=dholbert

The prose in the commit message needs to be a little clearer about the intent & what's actually changing.  How about:

"Refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class"

::: layout/style/CSSCalc.h:54
(Diff revision 4)
> - *   coeff_type
> - *   ComputeCoefficient(const coeff_type& aValue);
> + *   bool
> + *   ComputeCoefficient(result_type& aResult, const coeff_type& aValue);

I don't think it's actually possible for ComputeCoefficient to return false, in practice. So we probably shouldn't change its signature and make clients/implementations have to reason about & include logic for that possibility.

Specifically: I think it should only be called (by ComputeCalc) on expressions that were unitless CSS values or calc expressions of unitless CSS values (not pixel/percent/etc).  That's enforced because ComputeCalc() only calls it when it sees eCSSUnit_Calc_Times_L or eCSSUnit_Calc_Times_R, and the parser should enforce that the LHS or RHS are really a unitless coefficient there (or a calc expression that only involves unitless coefficients).

So, unless I'm missing something, I think you should:
 (1) restore ComputeCoefficient to returning "coeff_type" rather than bool
 (2) Adjust your ReduceCalcOps::ComputeCoefficient() implementation to *assert* that its ComputeCalc invocation always succeeds, rather than having it pass up the return value.  If its ComputeCalc expression fails, that means something is very wrong and we have a bug to fix.

::: layout/style/CSSCalc.h:72
(Diff revision 4)
> + * ComputeCalc will fail and return false if ComputeLeafValue or
> + * ComputeCoefficient return false at any point during the computation.

Please add a comment about:
 * the circumstances under which ComputeCalc/ComputeLeafValue implementations are allowed to fail
 * the circumstances under which they can be assumed to be _guaranteed_ to pass (since nearly every caller assumes & asserts that they pass right now, and someone adding a new caller will need some help to understand whether they're justified in copying one of those assertions).

(The same goes for ComputeCoefficient, if that keeps its bool return-type -- though per my comment above, I think it shouldn't need to return bool)

::: layout/style/CSSCalc.h:352
(Diff revision 4)
> - * ReduceNumberCalcOps is a CalcOps implementation for pure-number calc()
> - * (sub-)expressions, input as nsCSSValues.
> + * ReduceCalcOps is a CalcOps implementation for pure-number (as well as 
> + * percent, integer, and float) calc() (sub-)expressions, input as nsCSSValues.

"...for pure-number, pure-percent, and pure-integer calc() sub-expressions..."

(Drop "float" because it's the same as "number" in this context (it's eCSSUnit_Number), and add "pure" to the other types for emphasis that you're not talking about mixing them).

::: layout/style/CSSCalc.h:399
(Diff revision 4)
> +  bool ComputeLeafValue(result_type& aResult, const nsCSSValue& aValue)
>    {
> -    MOZ_ASSERT(aValue.GetUnit() == eCSSUnit_Integer, "unexpected unit");
> -    return aValue.GetIntValue();
> +    if (aValue.GetUnit() != unit) {
> +      return false;

Please add an assertion to very beginning of this function, to check that "coeff_type" and "unit" are one of the combinations that we expect them to be -- to be sure some caller doesn't accidentally instantiate a bogus ReduceCalcOps<double, eCSSUnit_Integer> or something like that -- something that would compile, but would produce bad results due to e.g. double*integer multiplication producing a double and then getting truncated to int under the hood.

I think this should do it:
    static_assert((unit == eCSSUnit_Integer &&
                  std::is_same<coeff_type, int>::value) ||
                  ((unit == eCSSUnit_Percent || unit == eCSSUnit_Number) &&
                  std::is_same<coeff_type, float>::value),
                 "We only support integer with int coefficient and "
                 "percent/number with float coefficient");

::: layout/style/CSSCalc.h:404
(Diff revision 4)
> +    aResult = unit == eCSSUnit_Percent ? aValue.GetPercentValue() :
> +             unit == eCSSUnit_Integer ? aValue.GetIntValue() :
> +                                        aValue.GetFloatValue();

Nit: the last two lines need one more space of indentation here.

::: layout/style/nsCSSParser.cpp:13626
(Diff revision 4)
>      if (variantMask & VARIANT_NUMBER) {
>        // Simplify the value immediately so we can check for division by
>        // zero.
> -      mozilla::css::ReduceNumberCalcOps ops;
> -      float number = mozilla::css::ComputeCalc(*storage, ops);
> +      float number;
> +      mozilla::css::ReduceCalcOps<float, eCSSUnit_Number> ops;
> +      if (!mozilla::css::ComputeCalc(number, *storage, ops)) {
> +        MOZ_ASSERT_UNREACHABLE("unexpected unit");

(just an observation: I don't 100% understand why this MOZ_ASSERT_UNREACHABLE and the ones that follow are justified, but I suppose they must be valid, because in the old version of the code, ReduceNumberCalcOps asserts that all of its leaves have eCSSUnit_Number as their units -- and similar for the other Reduce[...]CalcOps types. So, this seems fine & like a valid carrying forward of that assertion.)

::: layout/style/nsRuleNode.cpp:347
(Diff revision 4)
> -  result_type ComputeLeafValue(const nsCSSValue& aValue)
> +  bool ComputeLeafValue(result_type& aResult, const nsCSSValue& aValue)
>    {
> -    return CalcLengthWith(aValue, mFontSize, mStyleFont,
> +    aResult = CalcLengthWith(aValue, mFontSize, mStyleFont,
> -                          mStyleContext, mPresContext, mUseProvidedRootEmSize,
> +                            mStyleContext, mPresContext, mUseProvidedRootEmSize,
> -                          mUseUserFontSet, mConditions);
> +                            mUseUserFontSet, mConditions);

The last 2 lines here (the args) need one more space of indentation.

Comment 32

[Kevin Hsieh]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152324/diff/4-5/

Comment 33

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/8-9/

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

https://reviewboard.mozilla.org/r/152324/#review159984

> I don't think it's actually possible for ComputeCoefficient to return false, in practice. So we probably shouldn't change its signature and make clients/implementations have to reason about & include logic for that possibility.
> 
> Specifically: I think it should only be called (by ComputeCalc) on expressions that were unitless CSS values or calc expressions of unitless CSS values (not pixel/percent/etc).  That's enforced because ComputeCalc() only calls it when it sees eCSSUnit_Calc_Times_L or eCSSUnit_Calc_Times_R, and the parser should enforce that the LHS or RHS are really a unitless coefficient there (or a calc expression that only involves unitless coefficients).
> 
> So, unless I'm missing something, I think you should:
>  (1) restore ComputeCoefficient to returning "coeff_type" rather than bool
>  (2) Adjust your ReduceCalcOps::ComputeCoefficient() implementation to *assert* that its ComputeCalc invocation always succeeds, rather than having it pass up the return value.  If its ComputeCalc expression fails, that means something is very wrong and we have a bug to fix.

I think this ^ comment still hasn't been addressed in the latest version.  Can you restore the original ComputeCoefficient function-signature, since in practice it can't fail for reasons described above?  (That'll make it a little easier to reason about the space of logical possibilities, and it'll make the patch a little more targeted.)

Comment 35

[Kevin Hsieh]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152324/diff/5-6/

Comment 36

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/9-10/

Comment 37

[Daniel Holbert [:dholbert]]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

https://reviewboard.mozilla.org/r/152324/#review161830

Thanks for updating this! I somehow missed the bugmail for the updated patch, but I noticed it now.

r=me with the following fixes addressed:

::: layout/style/CSSCalc.h:123
(Diff revision 6)
> -      typename CalcOps::result_type lhs = ComputeCalc(arr->Item(0), aOps);
> +      typename CalcOps::result_type lhs;
>        typename CalcOps::coeff_type rhs = aOps.ComputeCoefficient(arr->Item(1));
> -      return aOps.MergeMultiplicativeR(CalcOps::GetUnit(aValue), lhs, rhs);
> +      if (!ComputeCalc(lhs, arr->Item(0), aOps)) {
> +        return false;
> +      }

Please shift the "coeff_type rhs" line here down 3 lines -- after the whole "if(...)" block -- so that the other variable (lhs)'s declaration and first usage are closer together (and so that this patch isn't needlessly reordering operations).

(We try to keep variable declarations and their first usage as close together as possible.)

::: layout/style/CSSCalc.h:351
(Diff revision 6)
> - * ReduceNumberCalcOps is a CalcOps implementation for pure-number calc()
> - * (sub-)expressions, input as nsCSSValues.
> + * ReduceCalcOps is a CalcOps implementation for pure-number, pure-percent, and
> + * pure-integer) calc() (sub-)expressions, input as nsCSSValues.

Drop the stray close-paren after "pure-integer".

::: layout/style/CSSCalc.h:395
(Diff revision 6)
> -  ComputeLeafValue(const nsCSSValue& aValue)
> +  MergeMultiplicativeR(nsCSSUnit aCalcFunction,
> +                       result_type aValue1, coeff_type aValue2)
>    {
> -    MOZ_ASSERT(aValue.GetUnit() == eCSSUnit_Integer, "unexpected unit");
> -    return aValue.GetIntValue();
> +    if (aCalcFunction == eCSSUnit_Calc_Times_R) {
> +      return aValue1 * aValue2;
> +    }
> +    MOZ_ASSERT(aCalcFunction == eCSSUnit_Calc_Divided, "unexpected unit");

The old integer-specific version of this code has an assertion about no-divisions-for-integers -- let's preserve that assertion in this now-unified MergeMultiplicativeR function, like so, inserted after the other MOZ_ASSERT here:
    MOZ_ASSERT(unit != eCSSUnit_Integer,
               "We should catch and prevent divisions in integer "
               "calc()s in the parser");

::: layout/style/CSSCalc.h:416
(Diff revision 6)
> +  coeff_type ComputeCoefficient(const nsCSSValue& aValue)
> +  {
> +    coeff_type coeff;
> +    if (!mozilla::css::ComputeCalc(coeff, aValue, *this)) {
> +      MOZ_ASSERT_UNREACHABLE("unexpected unit");

It's not clear (just from the contextual code & assertion-message) quite what this assertion means or why it's expected to hold.

Please add a code-comment like the following just before it the MOZ_ASSERT_UNREACHABLE to clarify:
      // Something's wrong; parser should enforce that calc() coefficients are
      // unitless, but failure in ComputeCalc means there was a unit mismatch.

::: layout/style/nsRuleNode.cpp:557
(Diff revision 6)
> -    return css::ComputeCalc(aValue, ops);
> +    nscoord aResult;
> +    if (!css::ComputeCalc(aResult, aValue, ops)) {
> +      MOZ_ASSERT_UNREACHABLE("unexpected ComputeCalc failure");
> +    }
> +    return aResult;

s/aResult/result/

(Local variables should not have an "a" prefix)

::: layout/style/nsRuleNode.cpp:762
(Diff revision 6)
> -    return result_type(CalcLength(aValue, mContext, mPresContext,
> +    aResult = result_type(CalcLength(aValue, mContext, mPresContext,
> -                                  mConditions),
> +                         mConditions),
> -                       0.0f);
> +                         0.0f);

To preserve correct alignment here, it looks like:
* mConditions needs 12 more spaces of indentation
* 0.0f needs 1 more space of indentation

::: layout/style/nsRuleNode.cpp:4620
(Diff revision 6)
> -  result_type ComputeLeafValue(const nsCSSValue& aValue)
> +  bool ComputeLeafValue(result_type& aResult, const nsCSSValue& aValue)
>    {
>      LengthNumberCalcObj result;
>      if (aValue.IsLengthUnit()) {
>        result.mIsNumber = false;
>        result.mValue = CalcLength(aValue, mStyleContext,
>                                        mPresContext, mConditions);
>      }
>      else if (eCSSUnit_Number == aValue.GetUnit()) {
>        result.mIsNumber = true;
>        result.mValue = aValue.GetFloatValue();
>      } else {
>        MOZ_ASSERT(false, "unexpected value");
>        result.mIsNumber = true;
>        result.mValue = 1.0f;
>      }
>  
> -    return result;
> +    aResult = result;
> +    return true;

(This is inside LengthNumberCalcOps)

It's a bit confusing to have both "aResult" and "result" in scope at the same time (and it's slightly wasteful to create an extra unnecessary local instance of this struct, and then copy it into the outparam.)

Let's just get rid of the local "result" variable entirely, and operate directly on aResult throughout this function.

::: layout/style/nsRuleNode.cpp:4651
(Diff revision 6)
> -  result_type ComputeLeafValue(const nsCSSValue& aValue)
> +  bool ComputeLeafValue(result_type& aResult, const nsCSSValue& aValue)
>    {
>      LengthNumberCalcObj result;
>      if (aValue.IsLengthUnit()) {

Same here (in this similar code inside SetLineHeightCalcOps) -- please get rid of the 'result' local var, and replace its usages with aResult.

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

https://reviewboard.mozilla.org/r/151826/#review161866

r=me on this part, with the following tweaks:

::: layout/style/nsCSSParser.cpp:10515
(Diff revision 10)
> +  // Attempts to use ParseVariant to process the token. Failing that, it may be
> +  // a keyword or unknown token, in which case we execute the rest of the
> +  // function.
> +  CSSParseResult status = ParseVariant(aComponent, VARIANT_PN | VARIANT_CALC,
> +                                       nullptr);

Let's expand on "to process the token" slightly, since what follows is a bit complex and has a number of special cases for things that we only do in this function (treat number as pixel, and check for calc() unit mismatches)

Maybe swap in this text or something like it to the comment:
"[...] to process the token, as a number (representing pixels), or a percent, or a calc expression of purely one or the other of those (we enforce this pureness via ReduceCalcOps::ComputeCalc below). If ParseVariant fails, the token may instead be a keyword or [...]"

::: layout/style/nsCSSParser.cpp:10637
(Diff revision 10)
>  CSSParserImpl::ParseWebkitGradientRadius(float& aRadius)
>  {
> -  if (!GetToken(true)) {
> +  nsCSSValue parseResult;
> +  CSSParseResult status = ParseVariant(parseResult,
> +                                       VARIANT_NUMBER | VARIANT_CALC, nullptr);
> +  if (status != CSSParseResult::Ok) {
>      return false;
>    }
> -
> -  if (mToken.mType != eCSSToken_Number) {
> -    UngetToken();
> +  switch (parseResult.GetUnit()) {
> +    case eCSSUnit_Number:
> +      aRadius = parseResult.GetFloatValue();
> +      return true;
> +    case eCSSUnit_Calc: {
> +      ReduceCalcOps<float, eCSSUnit_Number> ops;
> +      return ComputeCalc(aRadius, parseResult, ops);
> +    }

Since we're only allowing for VARIANT_NUMBER inside our calc(), we should be guaranteed that ComputeCalc succeeds here.  There's no chance it should fail.

So, for consistency & clarity, let's make this match all of the other no-chance-of-failure cases that you upgraded in part 1, like so:
  if (!ComputeCalc(...)) {  
    MOZ_ASSERT_UNREACHABLE("unexpected unit");
  }
  return true;

Comment 39

[Kevin Hsieh]

Comment on attachment 8880955 [details]
bug 1363349 (part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152324/diff/6-7/

Comment 40

[Kevin Hsieh]

Comment on attachment 8880494 [details]
bug 1363349 (part 2) - accept calc expressions in webkit gradient

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151826/diff/10-11/

Comment 41

[Kevin Hsieh]

Recent successful try run (except some intermittent failures): https://treeherder.mozilla.org/#/jobs?repo=try&revision=686c8f5077efa0d5a58930ef2292ef96dd38a45f

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9c6e87b9304
(part 1) - refactor CSSCalc.h to share code better, via a templated ReduceCalcOps class r=dholbert
https://hg.mozilla.org/integration/autoland/rev/853946cd3d40
(part 2) - accept calc expressions in webkit gradient r=dholbert

Comment 43

[Xidorn Quan [:xidorn] UTC+11]

It looks like Servo now has a worse support of calc in gradient than Gecko... The newly-added tests are failing on stylo build.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3de33cf4f989
followup - Update test expectations for stylo.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9c6e87b9304
https://hg.mozilla.org/mozilla-central/rev/853946cd3d40
https://hg.mozilla.org/mozilla-central/rev/3de33cf4f989