1293743 - Extend ParseCalc/ParseVariant to allow VARIANT_INTEGER | VARIANT_CALC (i.e. pure integer calc()s)

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jonathan Chan [:jyc]]

With a few modifications, we can extend ParseVariant/ParseCalc to parse pure-integer calc()s. This is needed for the Properties & Values API [1], which lets users register <integer> properties that should be able to accept calc() values [2].

This shouldn't change anyone else's behavior, because currently we throw an assert if VARIANT_INTEGER | VARIANT_CALC is specified (we & VARIANT_LPN before passing to ParseCalc in ParseVariant).

Patch done, will be uploaded very soon.

[1]: https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-strings
[2]: https://drafts.csswg.org/css-values-3/#calc-notation

Comment 1

[Jonathan Chan [:jyc]]

Attachment: Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.

ParseVariant now parses integer calc()s when given VARIANT_INTEGER | VARIANT_CALC.
Integer calcs cannot contain any division and must contain only integers.

We also move ReduceNumberCalcOps from nsCSSParser to CSSCalc.

[1]: https://drafts.csswg.org/css-values-3/#funcdef-calc

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

Comment 2

[Jonathan Chan [:jyc]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42435f9fdf57&selectedJob=25410290

Comment 3

[Daniel Holbert [:dholbert]]

Sorry for the delay here. Also: dbaron should probably take a look at this as well, since this is code that he wrote, & which I'm not super-familiar with.  I should have noticed that sooner & redirected the review to him, probably. Sorry :-/

But, while I'm here, my main up-front question (which I think dbaron would ask too) is:  could you explain why the patch needs to move ReduceNumberCalcOps to CSSCalc.h, and why the similar ReduceIntegerCalcOps is getting added there as well, rather than in nsCSSParser.cpp [its usage site]  It seems like in current mozilla-central, this code is structured such that CSSCalc.h mostly contains "Basic" CalcOps superclasses, and .cpp files contain subclasses with more-complete CalcOps implementations (as subclasses).  Seems like maybe this new code should follow that pattern as well -- unless you need to reference these CalcOps classes in multiple .cpp files, in a later patch...

Comment 4

[Jonathan Chan [:jyc]]

Hi Daniel,

In a future patch (for Properties & Values), IIRC ReduceNumberCalcOps is used in both nsCSSParser.cpp and nsRuleNode.cpp -- for reducing numbers in all calc expressions except pure-integer calcs (so that we can avoid division by zero (see nsCSSParser.cpp ParseCalcMultiplicativeExpression)) but also in nsRuleNode.cpp so we can compute calc() expressions for <number> values.

That is why I moved it to CSSCalc.h. Before I added integers, I just exposed it on nsCSSParser, but that felt pretty messy, and CSSCalc seemed like a better place because already exposes some other calc-related classes.

Integer calcs have an analogous situation -- simplification in nsCSSParser, and computation in nsRuleNode.

If we simplify pure-integer or pure-number calcs completely like Chrome does (e.g. if you set animation-iteration-count to calc(5 * 3), and you get the declaration value, you get 15, not calc(15) which Safari does -- Firefox doesn't do calc()s there at all) then we would not need them in nsRuleNode.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8779511 [details]
Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.

https://reviewboard.mozilla.org/r/70494/#review69584

Initial review comments:

::: layout/style/CSSCalc.h:30
(Diff revision 1)
>   *   //   uint32_t input_array_type::Count() const;
>   *   //   input_type& input_array_type::Item(uint32_t);
>   *   typedef ... input_type;
>   *   typedef ... input_array_type;
>   *
> + *   typedef ... number_type;

The name "number_type" is confusing here, because:
 a) All of the arguments in these operations are numbers of some sort. It's unclear what this particular "number" is supposed to represent.
 b) <number> is a specific thing in CSS -- and in your new code here, we're specifically operating on <integer>s, and yet we still have a number_type (which happens to be "int").

Perhaps this should be renamed to "coeff_type" (for "coefficient type")?  It seems like this type is used specifically used as coefficients in multiplication/division operations, AFAICT)

Similarly, perhaps the existing ComputeNumber() should be renamed to ComputeCoefficient()...? (Its documentation suggests that it's used purely for computing coefficient values, I think).

It might be nice to do that refactoring as a "part 1" patch here -- introducing & consistently using that typedef & renaming ComputeNumber(), before any new code is added. And then "part 2" would actually add the integer-calc implementation, perhaps.

::: layout/style/CSSCalc.h:45
(Diff revision 1)
>   *   MergeAdditive(nsCSSUnit aCalcFunction,
>   *                 result_type aValue1, result_type aValue2);
>   *
>   *   result_type
>   *   MergeMultiplicativeL(nsCSSUnit aCalcFunction,
> - *                        float aValue1, result_type aValue2);
> + *                        number_type aValue1, result_type aValue2);

Please change the actual MergeMultiplicativeL function declarations (in this file) to match this change. (Right now, they still use hardcoded "float" for aValue1 -- they don't match this updated sample-code.)

You may need to add a number_type typedef to the "Basic*" classes for this to work, too, I think.

::: layout/style/CSSCalc.h:73
(Diff revision 1)
>   * presume that expressions in the number positions have already been
>   * normalized to a single numeric value and derive from
>   * NumbersAlreadyNormalizedCalcOps.)
>   *
> + * number_type will be float most of the time (e.g. what you get with
> + * ParseVariant and VARIANT_NUMBER, or what a CSS <number> is), but it's

Consider dropping or rewording this clause:
   ", or what a CSS <number> is"

(I don't quite understand what that clause is saying.)

::: layout/style/CSSCalc.h:365
(Diff revision 1)
>    } else {
>      aOps.AppendLeafValue(aValue);
>    }
>  }
>  
> +struct ReduceNumberCalcOps : public mozilla::css::BasicFloatCalcOps,

This struct should ideally have a brief comment to explain its purpose, like we have for the other CalcOps structs declared in this file.

(I notice it didn't have any explanatory comment in its previous location, before this patch was applied -- I guess that's because it was buried in the middle of nsCSSParser.cpp, and it's only used inside of that .cpp file. But now that it's being exposed in a well-documented header and gaining additional usages in a later bug, we should include some brief documentation for this type.)

::: layout/style/CSSCalc.h:383
(Diff revision 1)
> +  {
> +    return mozilla::css::ComputeCalc(aValue, *this);
> +  }
> +};
> +
> +struct ReduceIntegerCalcOps : public mozilla::css::CSSValueInputCalcOps

Same goes for this struct - please include some brief documentation.

::: layout/style/CSSCalc.h:389
(Diff revision 1)
> +{
> +  typedef int result_type;
> +  typedef int number_type;
> +
> +  result_type
> +  MergeAdditive(nsCSSUnit aCalcFunction, int aValue1, int aValue2)

These "MergeXYZ" functions should probably be refactored out into a "BasicIntegerCalcOps" superclass, for consistency with (& easier comparison to) the BasicFloatCalcOps and BasicCoordCalcOps superclasses.

Also: the "int" parameter-type should be replaced with the typedeffed "result_type" here, for consistency with other functions (and with the comment at the top of this file).

::: layout/style/CSSCalc.h:400
(Diff revision 1)
> +               "unexpected unit");
> +    return aValue1 - aValue2;
> +  }
> +
> +  result_type
> +  MergeMultiplicativeL(nsCSSUnit aCalcFunction, int aValue1, int aValue2)

Please fix the args here to use the typedefs, to match the prototype for this function in the comment at the top of this file.

::: layout/style/CSSCalc.h:408
(Diff revision 1)
> +               "unexpected unit");
> +    return aValue1 * aValue2;
> +  }
> +
> +  result_type
> +  MergeMultiplicativeR(nsCSSUnit aCalcFunction, int aValue1, int aValue2)

Same (please use the typedefs).

::: layout/style/nsCSSParser.cpp:8009
(Diff revision 1)
>    if ((aVariantMask & VARIANT_CALC) &&
>        IsCSSTokenCalcFunction(*tk)) {
> -    // calc() currently allows only lengths and percents and number inside it.
> +    // calc() currently allows only lengths, percents, numbers, and integers
> +    // inside it.
>      // And note that in current implementation, number cannot be mixed with
> -    // length and percent.
> +    // length and percent, nor integers with anything else.

This comment needs minor rewording. Right now, I think it says the following two things, which seem superficially contradictory:
 (1) number cannot be mixed with length and percent.
 (2) integers cannot be mixed with anything else.

These are confusing because: statement (1) implies (indirectly) that numbers *can* be mixed with integers (since integers weren't included in the excluded list). But statement 2 implies that they cannot.  Hence, the statements seem contradictory.

Perhaps rephrase as: "...in the current implementation, numbers can only be mixed with numbers, and integers can only be mixed with integers".  That's a clearer way to state what you're getting at here, if I'm understanding correctly.

::: layout/style/nsCSSParser.cpp:13406
(Diff revision 1)
>    // This can be done without lookahead when we assume that the property
>    // values cannot themselves be numbers.
>    MOZ_ASSERT(aVariantMask != 0, "unexpected variant mask");
> +  MOZ_ASSERT(!((aVariantMask & VARIANT_INTEGER) &&
> +               (aVariantMask & VARIANT_LPN)),
> +             "can't mix integers in calcs with other values");

For consistency/completeness, could you add an equivalent assertion for numbers? ("can't mix numbers in calcs with other values")

(I expect that assertion should hold up, if I'm correctly understanding the comment in my previous review-comment).

Comment 6

[Daniel Holbert [:dholbert]]

Also, ideally the ReduceNumberCalcOps code-move should happen in its own non-functional commit. (That's a sizeable enough change that it's nice to split it out, both so that it can be verified on its own as a non-functional change, and also so that it doesn't confuse/obscure what's going on in the rest of the main patch.)

Comment 7

[Jonathan Chan [:jyc]]

Attachment: Bug 1293743 - Part 2: Rename references to 'number' in CSSCalc and related code to coefficient. Introduce coefficient typedef.

"For each leaf in the calc() expression, ComputeCalc will call either
ComputeNumber (when the leaf is the left side of a Times_L or the right
side of a Times_R or Divided) or ComputeLeafValue (otherwise)."

A future patch in this series adds support for evaluating pure-integer calc()s.
We rename ComputeNumber to ComputeCoefficient and introduce a coefficient_type
typedef so that coefficients can be integers. We don't want to leave it as
'number' because that is confusing (e.g. CSS <number>s are float values).

We also rename NumbersAlreadyNormalizedCalcOps to
FloatCoeffsAlreadyNormalizedCalcOps, and expect AppendCoefficient in the
template given to SerializeCalc instead of AppendNumber.

This requires some renames in nsCSSValue and nsRuleNode.
I would split this into a separate 'fully-automated' patch, except that it's so
few renames and it feels bad to add the comments separately.

We also have to add |typedef float coeff_type| to two CalcOps implementations
in nsRuleNode because they multiply-inherit from two classes that define
coeff_type as float.

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

Comment 8

[Jonathan Chan [:jyc]]

Attachment: Bug 1293743 - Part 3: Add ReduceIntegerCalcOps, analogous to ReduceNumberCalcOps but for pure-integer calc()s.

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

Comment 9

[Jonathan Chan [:jyc]]

Attachment: Bug 1293743 - Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s).

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

Comment 10

[Jonathan Chan [:jyc]]

Comment on attachment 8779511 [details]
Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.

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

Comment 11

[Jonathan Chan [:jyc]]

Comment on attachment 8779511 [details]
Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.

https://reviewboard.mozilla.org/r/70494/#review69584

> The name "number_type" is confusing here, because:
>  a) All of the arguments in these operations are numbers of some sort. It's unclear what this particular "number" is supposed to represent.
>  b) <number> is a specific thing in CSS -- and in your new code here, we're specifically operating on <integer>s, and yet we still have a number_type (which happens to be "int").
> 
> Perhaps this should be renamed to "coeff_type" (for "coefficient type")?  It seems like this type is used specifically used as coefficients in multiplication/division operations, AFAICT)
> 
> Similarly, perhaps the existing ComputeNumber() should be renamed to ComputeCoefficient()...? (Its documentation suggests that it's used purely for computing coefficient values, I think).
> 
> It might be nice to do that refactoring as a "part 1" patch here -- introducing & consistently using that typedef & renaming ComputeNumber(), before any new code is added. And then "part 2" would actually add the integer-calc implementation, perhaps.

I have split the patch into four pieces!

> This comment needs minor rewording. Right now, I think it says the following two things, which seem superficially contradictory:
>  (1) number cannot be mixed with length and percent.
>  (2) integers cannot be mixed with anything else.
> 
> These are confusing because: statement (1) implies (indirectly) that numbers *can* be mixed with integers (since integers weren't included in the excluded list). But statement 2 implies that they cannot.  Hence, the statements seem contradictory.
> 
> Perhaps rephrase as: "...in the current implementation, numbers can only be mixed with numbers, and integers can only be mixed with integers".  That's a clearer way to state what you're getting at here, if I'm understanding correctly.

I have tried rewording it. Let me know how it looks! Running on try to make sure the new asserts don't run afoul of anything.

Comment 12

[Jonathan Chan [:jyc]]

Turns out that original comment was out-of-date: among others, line-height has VARIANT_LPN | VARIANT_CALC. Doing a try run to verify that "the variant mask should intersect with exactly one of VARIANT_LPN | VARIANT_INTEGER" is the correct invariant.

Comment 13

[Jonathan Chan [:jyc]]

Comment on attachment 8779511 [details]
Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.

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

Comment 14

[Jonathan Chan [:jyc]]

Comment on attachment 8781786 [details]
Bug 1293743 - Part 2: Rename references to 'number' in CSSCalc and related code to coefficient. Introduce coefficient typedef.

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

Comment 15

[Jonathan Chan [:jyc]]

Comment on attachment 8781787 [details]
Bug 1293743 - Part 3: Add ReduceIntegerCalcOps, analogous to ReduceNumberCalcOps but for pure-integer calc()s.

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

Comment 16

[Jonathan Chan [:jyc]]

Comment on attachment 8781788 [details]
Bug 1293743 - Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s).

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

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8779511 [details]
Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.

https://reviewboard.mozilla.org/r/70494/#review70096

r=me -- just one nit on the commit message, which currently looks like this:
> Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.
> 
> Bug 1273706 (Properties & Values) uses ReduceNumberCalcOps in nsRuleNode to
> compute <number>-typed calc() expressions.

It's a bit confusing to have multiple lines starting with "Bug NNN" in the commit message here. For the latter one, please rewrite like so:
   A later bug (Bug 1273706, CSS Properties & Values) will use [...]

r=me with that

Comment 18

[Daniel Holbert [:dholbert]]

(er, sorry for all the "&junk;" in comment 17; that's from bug 1283024 comment 6 biting me.)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8781786 [details]
Bug 1293743 - Part 2: Rename references to 'number' in CSSCalc and related code to coefficient. Introduce coefficient typedef.

https://reviewboard.mozilla.org/r/72114/#review70098

r=me with nits below addressed:

::: layout/style/CSSCalc.h:253
(Diff revision 2)
>   *
>   *   static nsCSSUnit GetUnit(const input_type& aValue);
>   *
>   *   void Append(const char* aString);
>   *   void AppendLeafValue(const input_type& aValue);
> - *   void AppendNumber(const input_type& aValue);
> + *   void AppendCoefficient(const input_type& aValue);

s/input_type/coeff_type/ here.

(This was a typo in the old code -- this should've said "float" in the past, for consistency with other number-handling stuff in this file.  In any case, now it should be coeff_type.)

::: layout/style/nsCSSValue.cpp:878
(Diff revision 2)
>                 aValue.GetUnit() == eCSSUnit_Number,
>                 "unexpected unit");
>      aValue.AppendToString(mProperty, mResult, mValueSerialization);
>    }
>  
> -  void AppendNumber(const input_type& aValue)
> +  void AppendCoefficient(const input_type& aValue)

s/input_type/coeff_type/

::: layout/style/nsRuleNode.cpp:294
(Diff revision 2)
>  
>  struct CalcLengthCalcOps : public css::BasicCoordCalcOps,
> -                           public css::NumbersAlreadyNormalizedOps
> +                           public css::FloatCoeffsAlreadyNormalizedOps
>  {
> +  // Declare that we have floats as coefficients so that we unambiguously
> +  // resolve coeff_type (BasicCoordClacOps and FloatCoeffsAlreadyNormalizedOps

Typo -- s/Clac/Calc/

::: layout/style/nsRuleNode.cpp:3220
(Diff revision 2)
>  
>  struct SetFontSizeCalcOps : public css::BasicCoordCalcOps,
> -                            public css::NumbersAlreadyNormalizedOps
> +                            public css::FloatCoeffsAlreadyNormalizedOps
>  {
> +  // Declare that we have floats as coefficients so that we unambiguously
> +  // resolve coeff_type (BasicCoordClacOps and FloatCoeffsAlreadyNormalizedOps

As above, s/Clac/Calc/

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8781787 [details]
Bug 1293743 - Part 3: Add ReduceIntegerCalcOps, analogous to ReduceNumberCalcOps but for pure-integer calc()s.

https://reviewboard.mozilla.org/r/72116/#review70100

r=me with the following addressed.

::: layout/style/CSSCalc.h:445
(Diff revision 2)
> +
> +  coeff_type
> +  ComputeCoefficient(const nsCSSValue& aValue)
> +  {
> +    MOZ_ASSERT(aValue.GetUnit() == eCSSUnit_Integer, "unexpected unit");
> +    return aValue.GetIntValue();

The float version of this function doesn't make this assertion or just directly query for a numeric value like we're doing in this new code.  The float version calls ComputeCalc(aValue, *this). We should probably do that here, too.

I believe we need to do that because |aValue| might itself be a calc() expression populated with integers -- it's not necessarily just a single integer.

(When it *is* a single integer, ComputeCalc will end up calling our ComputeLeafValue method, which does just return GetIntValue like you've got this function doing right now.)

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8781788 [details]
Bug 1293743 - Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s).

https://reviewboard.mozilla.org/r/72118/#review70102

(Thanks for splitting this bug into pieces -- that makes this much easier to reason about & process.)

r=me with the following, though I don't fully trust myself to grant "full" r+ to changes in this chunk of code -- dbaron should probably take a look at this final patch, too, since he designed this code and knows how it works better than I do.  Could you tag him for review on this part, after you've addressed my review comments?  Thanks!

::: layout/style/nsCSSParser.cpp:8008
(Diff revision 2)
>    }
>    if ((aVariantMask & VARIANT_CALC) &&
>        IsCSSTokenCalcFunction(*tk)) {
> -    // calc() currently allows only lengths and percents and number inside it.
> -    // And note that in current implementation, number cannot be mixed with
> -    // length and percent.
> +    // calc() currently allows only lengths, percents, numbers, and integers.
> +    // All calc()s can have numbers inside of them as coefficients (e.g.
> +    // |coeff * value|), except for pure-integer calcs.

In the interests of clarity, please add a parenthetical at the end here, i.e. change this to:
  [...] except for pure-integer calcs (which require integer coefficients).

::: layout/style/nsCSSParser.cpp:13507
(Diff revision 2)
>  
>    nsCSSValue *storage = &aValue;
>    for (;;) {
>      uint32_t variantMask;
> +    if (aVariantMask & VARIANT_INTEGER) {
> +      variantMask = aVariantMask;

At first glance, it's unclear why the VARIANT_INTEGER case is so simple (always using aVariantMask) whereas the VARIANT_NUMBER cases below are more complex (maybe only using VARIANT_NUMBER, maybe also using aVariantMask).

I figured it out after staring a bit, but could you make it clearer by adding comments to the pre-existing clauses to make it clearer what they're doing and why they have different variantMask behavior?

I believe the first pre-existing clause (inside the afterDivision || gotValue check) should get a comment like:
      // At this point in the calc expression, we expect a coefficient or a
      // divisor, which must be a number. (Not a length/%/etc.)

And the second one (inside "else") should say:
      // At this point in the calc expression, we'll accept a coefficient
      // (a number) or a value of whatever type |aVariantMask| specifies.

::: layout/style/nsCSSParser.cpp:13509
(Diff revision 2)
>    for (;;) {
>      uint32_t variantMask;
> +    if (aVariantMask & VARIANT_INTEGER) {
> +      variantMask = aVariantMask;
> +    } else {
> -    if (afterDivision || gotValue) {
> +      if (afterDivision || gotValue) {

No need to reindent inside of "else" here -- just use else if, i.e. replace the old code's...
  if (afterDivision || gotValue) {
...with...
  } else if (afterDivision || gotValue) {

The logic flow should be equivalent to what you've got in this patch, and the nesting will be a bit less complex.

::: layout/style/nsCSSParser.cpp:13514
(Diff revision 2)
> -    if (afterDivision || gotValue) {
> +      if (afterDivision || gotValue) {
> -      variantMask = VARIANT_NUMBER;
> +        variantMask = VARIANT_NUMBER;
> -    } else {
> +      } else {
> -      variantMask = aVariantMask | VARIANT_NUMBER;
> +        variantMask = aVariantMask | VARIANT_NUMBER;
> -    }
> +      }
> +    }

(You won't need to add this new closing-brace if you follow my above s/else{if/else if/ suggestion.)

::: layout/style/nsCSSParser.cpp:13566
(Diff revision 2)
>      } else if (mToken.IsSymbol('/')) {
> +      if (variantMask & VARIANT_INTEGER) {
> +        // Integers aren't mixed with anything else (see the assert at the top
> +        // of CSSParserImpl::ParseCalc).
> +        // We don't allow division at all in calc()s for integers.
> +        // https://drafts.csswg.org/css-values-3/#funcdef-calc

The spec reference here is a bit confusing, since (1) the spec-language about division is a good ways down from the link-anchor you're providing here, and (2) the spec-language doesn't explicitly prohibit integer division, but just says it gets converted to float. (i.e. it's not immediately obvious why it's a justification for our prohibition here)

So, please replace the last two lines of your comment with something clearer like:
        // We don't allow division at all in calc()s for expressions where an
        // integer is expected, because calc() division can't be resolved to
        // an integer, as implied by spec text about '/' here:
        // https://drafts.csswg.org/css-values-3/#calc-type-checking

Comment 22

[Jonathan Chan [:jyc]]

Comment on attachment 8781786 [details]
Bug 1293743 - Part 2: Rename references to 'number' in CSSCalc and related code to coefficient. Introduce coefficient typedef.

https://reviewboard.mozilla.org/r/72114/#review70098

> s/input_type/coeff_type/ here.
> 
> (This was a typo in the old code -- this should've said "float" in the past, for consistency with other number-handling stuff in this file.  In any case, now it should be coeff_type.)

Good catch!

> Typo -- s/Clac/Calc/

Thanks for catching!

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Chan [:jyc] from comment #22)
> Comment on attachment 8781786 [details]
> Bug 1293743 - Part 2: Rename references to 'number' in CSSCalc and related
> code to coefficient. Introduce coefficient typedef.
> 
> https://reviewboard.mozilla.org/r/72114/#review70098
> 
> > s/input_type/coeff_type/ here.
> 
> Good catch!

(Er -- per IRC discussion just now, input_type was actually correct here -- sorry about that. I believe you're adding a comment now to make it clearer that this is an input_type which is being appended as a coefficient for serialization.)

Comment 24

[Jonathan Chan [:jyc]]

Comment on attachment 8779511 [details]
Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.

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

Comment 25

[Jonathan Chan [:jyc]]

Comment on attachment 8781786 [details]
Bug 1293743 - Part 2: Rename references to 'number' in CSSCalc and related code to coefficient. Introduce coefficient typedef.

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

Comment 26

[Jonathan Chan [:jyc]]

Comment on attachment 8781787 [details]
Bug 1293743 - Part 3: Add ReduceIntegerCalcOps, analogous to ReduceNumberCalcOps but for pure-integer calc()s.

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

Comment 27

[Jonathan Chan [:jyc]]

Comment on attachment 8781788 [details]
Bug 1293743 - Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s).

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

Comment 28

[Jonathan Chan [:jyc]]

Comment on attachment 8781788 [details]
Bug 1293743 - Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s).

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

Comment 29

[Daniel Holbert [:dholbert]]

Hi dbaron! I think this is just waiting on your review -- could you take a look?

This is one of the support-pieces of Bug 1293743 which is nearly-landable (though perhaps it should still land at the same time as Bug 1293743, since IIRC it's not directly testable except via that bug's patches/tests).

Comment 30

[Daniel Holbert [:dholbert]]

(mis-paste; s/bug 1293743/bug 1273706/)

Comment 31

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

Comment on attachment 8781788 [details]
Bug 1293743 - Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s).

https://reviewboard.mozilla.org/r/72118/#review83612

r=dbaron with the following fixed.  Sorry for the delay.

::: layout/style/nsCSSParser.cpp:8007
(Diff revision 4)
> -    // And note that in current implementation, number cannot be mixed with
> -    // length and percent.
> +    // All calc()s can have numbers inside of them as coefficients (e.g.
> +    // |coeff * value|), except for pure-integer calc()s (which require

So what this comment is trying to say "number cannot be mixed with length and percent" is still true.  It's just a weaker form of "cannot be mixed".

In particular, a calc() expression that is of type VARIANT_LENGTH | VARIANT_NUMBER (as for, e.g., line-height) cannot, in our current implementation, accept a value like:
  calc(1.2 + 3px)
even though that is, logically, a sensible thing to do for line-height.

So VARIANT_NUMBER can be mixed with VARIANT_LENGTH and VARIANT_PERCENTAGE in the list of allowed types, but the resulting type is not a mixed type with number.  (The CSS WG has agreed to change this in the future, but I don't think there's a spec draft.)

VARIANT_INTEGER isn't even allowed to be mixed as a possible type.

So this comment should be reworded to explain that.

::: layout/style/nsCSSParser.cpp:13506
(Diff revision 4)
> -    if (afterDivision || gotValue) {
> +    if (aVariantMask & VARIANT_INTEGER) {
> +      variantMask = aVariantMask;

maybe assert that aVariantMask is exactly VARIANT_INTEGER?

::: layout/style/nsCSSParser.cpp:13565
(Diff revision 4)
> +        // Integers aren't mixed with anything else (see the assert at the top
> +        // of CSSParserImpl::ParseCalc).
> +        // We don't allow division at all in calc()s for expressions where an
> +        // integer is expected, because calc() division can't be resolved to
> +        // an integer, as implied by spec text about '/' here:
> +        // https://drafts.csswg.org/css-values-3/#calc-type-checking

You should have a comment here noting that we've consumed the '/' token, but it doesn't matter, since we're in an error-handling situation where we've already consumed a bunch of other tokens.

Comment 32

[Jonathan Chan [:jyc]]

Comment on attachment 8779511 [details]
Bug 1293743 - Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps.

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

Comment 33

[Jonathan Chan [:jyc]]

Comment on attachment 8781786 [details]
Bug 1293743 - Part 2: Rename references to 'number' in CSSCalc and related code to coefficient. Introduce coefficient typedef.

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

Comment 34

[Jonathan Chan [:jyc]]

Comment on attachment 8781787 [details]
Bug 1293743 - Part 3: Add ReduceIntegerCalcOps, analogous to ReduceNumberCalcOps but for pure-integer calc()s.

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

Comment 35

[Jonathan Chan [:jyc]]

Comment on attachment 8781788 [details]
Bug 1293743 - Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s).

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

Comment 36

[Jonathan Chan [:jyc]]

Comment on attachment 8781788 [details]
Bug 1293743 - Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s).

https://reviewboard.mozilla.org/r/72118/#review83612

> So what this comment is trying to say "number cannot be mixed with length and percent" is still true.  It's just a weaker form of "cannot be mixed".
> 
> In particular, a calc() expression that is of type VARIANT_LENGTH | VARIANT_NUMBER (as for, e.g., line-height) cannot, in our current implementation, accept a value like:
>   calc(1.2 + 3px)
> even though that is, logically, a sensible thing to do for line-height.
> 
> So VARIANT_NUMBER can be mixed with VARIANT_LENGTH and VARIANT_PERCENTAGE in the list of allowed types, but the resulting type is not a mixed type with number.  (The CSS WG has agreed to change this in the future, but I don't think there's a spec draft.)
> 
> VARIANT_INTEGER isn't even allowed to be mixed as a possible type.
> 
> So this comment should be reworded to explain that.

Thanks. Sorry for the delay of my own. Unexpectedly busy with school.

Reworded the comment to say this (tried to base off of what you said):

    // calc() currently allows only lengths, percents, numbers, and integers.
    //
    // Note that VARIANT_NUMBER can be mixed with VARIANT_LENGTH and
    // VARIANT_PERCENTAGE in the list of allowed types (numbers can be used as
    // coefficients).
    // However, the the resulting type is not a mixed type with number.
    // VARIANT_INTEGER can't be mixed with anything else.

> You should have a comment here noting that we've consumed the '/' token, but it doesn't matter, since we're in an error-handling situation where we've already consumed a bunch of other tokens.

OK. Updated comment to say:

        // Integers aren't mixed with anything else (see the assert at the top
        // of CSSParserImpl::ParseCalc).
        // We don't allow division at all in calc()s for expressions where an
        // integer is expected, because calc() division can't be resolved to
        // an integer, as implied by spec text about '/' here:
        // https://drafts.csswg.org/css-values-3/#calc-type-checking
        // We've consumed the '/' token, but it doesn't matter as we're in an
        // error-handling situation where we've already consumed a lot of
        // other tokens (e.g. the token before the '/'). ParseVariant will
        // indicate this with CSSParseResult::Error.

Comment 37

[Daniel Holbert [:dholbert]]

To avoid further bitrot & since it seems (based on Astley's classification) that other work might depend on this, I think we should land this soon after the imminent version-bump (end of this week).  So, I'll tentatively plan on getting this landed early next week.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15ce360d2ae9
Part 1: Move nsCSSParser's ReduceNumberCalcOps to CSSCalc.h as mozilla::css::ReduceNumberCalcOps. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/03ac7f0dca2d
Part 2: Rename references to 'number' in CSSCalc and related code to coefficient. Introduce coefficient typedef. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/daff8c0b5e06
Part 3: Add ReduceIntegerCalcOps, analogous to ReduceNumberCalcOps but for pure-integer calc()s. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/662953491bfd
Part 4: Implement ParseVariant for VARIANT_INTEGER | VARIANT_CALC (i.e. pure-integer calc()s). r=dbaron,dholbert

Comment 39

[Astley Chen (inactive)]

Daniel, Jonathan, appreciated for your help to keep calc bugs running! Thanks.

Comment 40

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/15ce360d2ae9
https://hg.mozilla.org/mozilla-central/rev/03ac7f0dca2d
https://hg.mozilla.org/mozilla-central/rev/daff8c0b5e06
https://hg.mozilla.org/mozilla-central/rev/662953491bfd

Comment 41

[areya]

https://hg.mozilla.org/mozilla-central/rev/15ce360d2ae9
https://hg.mozilla.org/mozilla-central/rev/03ac7f0dca2d
https://hg.mozilla.org/mozilla-central/rev/daff8c0b5e06
https://hg.mozilla.org/mozilla-central/rev/662953491bfd

Comment 42

[Gerry Chang [:gchang]]

We've built 51 RC. This is too late for 51. Mark 51 won't fix.

Comment 43

[Daniel Holbert [:dholbert]]

Yeah, that's fine.  This is mostly an infrastructural change, in support of another bug.  I think the "firefox51: affected" bug-status was just an accident based on when this bug was filed, rather than an indication that any particular Firefox version needs this change.

[also, clearing my needinfo for landing this, which I neglected to clear in comment 38]