Bug 203312 – Implement the CSS exponent functions: pow(), sqrt(), hypot()

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

https://drafts.csswg.org/css-values-4/#exponent-funcs

[Kevin Turner]

Giving this a look!

[Kevin Turner]

Created attachment 417716 [details]
Patch

[Darin Adler]

Comment on attachment 417716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417716&action=review

Thanks for contributing this. Looks generally good.

Preliminary comments before EWS run is done, so not setting review+ or review- yet.

Not sure we have covered enough of the edge cases in our tests.

> Source/WebCore/css/CSSCalculationValue.cpp:914
> +    

Extra blank line here.

> Source/WebCore/css/CSSCalculationValue.cpp:916
> +    for (auto child : values) {

Should be auto& instead of auto so we don’t churn reference counts.

> Source/WebCore/css/CSSCalculationValue.cpp:930
> +    if (expectedCategory != CalculationCategory::Number && expectedCategory != CalculationCategory::Length && expectedCategory != CalculationCategory::Percent)

Helper function so we don’t include this twice, here and in the loop.

> Source/WebCore/css/CSSCalculationValue.cpp:1679
> +        double base = children[0];
> +        double exp = children[1];
> +        return std::pow(base, exp);

I don’t think the local variables help us here.

> Source/WebCore/css/CSSCalculationValue.cpp:1685
> +        double val = children[0];

WebKit coding style says to call this "value", not "val".

> Source/WebCore/css/CSSCalculationValue.cpp:1833
> -
> +        

Please don’t add trailing whitespace.

> Source/WebCore/platform/CalculationValue.cpp:183
> +        float val = m_children[0]->evaluate(maxValue);

WebKit coding style says to call this "value", not "val".

> LayoutTests/fast/css/calc-parsing.html:61
> +            testExpression('calc(sqrt(100px)', '999px', '999px');

How is this working given we forgot the end parenthesis?

[Simon Fraser (smfr)]

Comment on attachment 417716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417716&action=review

> Source/WebCore/css/CSSCalculationValue.cpp:785
> +    bool isExpFuncNode() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt || m_operator == CalcOperator::Pow; }

What is "exp" a contraction for here?

> Source/WebCore/css/CSSCalculationValue.cpp:1810
> +    // TODO: clamp, sin, cos, tan, asin, acos, atan, atan2.

Remove clamp

> Source/WebCore/css/CSSCalculationValue.cpp:1863
> +    // TODO: clamp, sin, cos, tan, asin, acos, atan, atan2.

Remove clamp

[Kevin Turner]

Comment on attachment 417716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417716&action=review

>> Source/WebCore/css/CSSCalculationValue.cpp:785
>> +    bool isExpFuncNode() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt || m_operator == CalcOperator::Pow; }
> 
> What is "exp" a contraction for here?

"Exponential" - I derived the naming from the title on the CSS spec for these functions: "11.5. Exponential Functions: pow(), sqrt(), hypot(), log(), exp()".

[Kevin Turner]

Comment on attachment 417716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417716&action=review

>> LayoutTests/fast/css/calc-parsing.html:61
>> +            testExpression('calc(sqrt(100px)', '999px', '999px');
> 
> How is this working given we forgot the end parenthesis?

I believe a mismatch in parentheses would cause us to fail silently (leaving the width set to 999px), but the original intention of this test case was to ensure we fail when trying to take the square root of a dimension type since sqrt() operates purely on numbers according to the spec. Will fix this in an updated patch shortly.

[Kevin Turner]

Created attachment 417728 [details]
Patch

[Darin Adler]

Comment on attachment 417728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417728&action=review

> Source/WebCore/css/CSSCalculationValue.cpp:943
> +
> +        if (!isValidHypotType(currentCategory))
> +            return nullptr;

Just realized this is dead code. We’e already checked expectedCategory and currentCategory is == it. So we don’t need this and thus don’t really need the helper function.

[Kevin Turner]

Created attachment 417738 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 417738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417738&action=review

> Source/WebCore/css/CSSCalculationValue.cpp:785
> +    bool isExpFuncNode() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt || m_operator == CalcOperator::Pow; }

isExponentialFunctionNode()

> Source/WebCore/css/CSSCalculationValue.cpp:792
> +    bool canHaveOnlyChild() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt; }

This name is a bit ambiguous. Is it "can have only one child"? Maybe allowsSingleChild()?

> Source/WebCore/css/CSSCalculationValue.cpp:1267
> +        // Don't simplify out sqrt() before its evaluated

its -> it is

> Source/WebCore/css/CSSCalculationValue.cpp:1690
> +            sum += std::pow(child, 2);

(child * child) probably slightly better.

[Darin Adler]

Comment on attachment 417738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417738&action=review

A few other minor comments.

>> Source/WebCore/css/CSSCalculationValue.cpp:792
>> +    bool canHaveOnlyChild() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt; }
> 
> This name is a bit ambiguous. Is it "can have only one child"? Maybe allowsSingleChild()?

I think this name isn’t quite right. This returns true for operators that are not identity functions when they have a single child. Not, as the title implies, the only operators that allow a single child. So we should use a different name. Maybe we should also reverse the sense of the function. It’s about an optimization in how we implement the other functions like min and max.

> Source/WebCore/css/CSSCalculationValue.cpp:935
> +        auto currentCategory = values[i]->category();
> +        if (currentCategory != expectedCategory)
> +            return nullptr;

Probably reads slightly better without the local variable.

> Source/WebCore/css/CSSCalculationValue.cpp:1670
> +    case CalcOperator::Pow: {

The items above don’t use braces when they don’t have local variables. This could omit braces to match those.

> Source/WebCore/css/CSSCalculationValue.cpp:1676
> +    }
> +    
> +    case CalcOperator::Sqrt: {

These blank lines are different than the formatting above. Let’s try to be consistent. I don’t mind the blank lines, but I would like the entire switch statement to stay consistent.

> Source/WebCore/css/CSSCalculationValue.cpp:1685
> +    }
> +    
> +    case CalcOperator::Hypot: {

Ditto.

[Kevin Turner]

Comment on attachment 417738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417738&action=review

>>> Source/WebCore/css/CSSCalculationValue.cpp:792
>>> +    bool canHaveOnlyChild() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt; }
>> 
>> This name is a bit ambiguous. Is it "can have only one child"? Maybe allowsSingleChild()?
> 
> I think this name isn’t quite right. This returns true for operators that are not identity functions when they have a single child. Not, as the title implies, the only operators that allow a single child. So we should use a different name. Maybe we should also reverse the sense of the function. It’s about an optimization in how we implement the other functions like min and max.

Perhaps something like 'canBehaveAsIdentity()'? hypot() and sqrt() can not behave as identity functions under any circumstance whereas min/max() can when it has one parameter. Would a direct check like 'isIdentity()' be inappropriate here? We could check that the m_children vector is of size 1 and the operator either min or max.

[Darin Adler]

(In reply to Kevin Turner from comment #12)
> Perhaps something like 'canBehaveAsIdentity()'? hypot() and sqrt() can not
> behave as identity functions under any circumstance whereas min/max() can
> when it has one parameter. Would a direct check like 'isIdentity()' be
> inappropriate here? We could check that the m_children vector is of size 1
> and the operator either min or max.

Yes, those both sound good.

[Kevin Turner]

Created attachment 417920 [details]
Patch

[Kevin Turner]

Created attachment 418371 [details]
Patch

[Kevin Turner]

Cleaned up spacing in switch statements, added check for hypot() with no children, and added additional non-parsing test cases.

[Kevin Turner]

Created attachment 436678 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 436678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436678&action=review

> LayoutTests/fast/css/calc-parsing.html:68
> +                testExpression('hypot()', '999px', '999px');

Can we add tests for very large values? Like exp(1000)?

[Kevin Turner]

Created attachment 436786 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 436786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436786&action=review

Would be nice to make Web Platform Tests for these.

> Source/WebCore/ChangeLog:2
> +       Add support for pow(), sqrt() and hypot() per https://drafts.csswg.org/css-values-4/#exponent-funcs.

Missing blank line

> Source/WebCore/ChangeLog:7
> +       Test: fast/css/calc-parsing.html

Normally we write some words here to say what the change does. The spec link would be better here.

> Source/WebCore/ChangeLog:9
> + 
> +

Extra. blank line.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:154
> +            // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations
> +            return CalculationCategory::Other;

I can't tell if the line you copied above with the comment represents a TODO or correct behavior.

[Kevin Turner]

Created attachment 436797 [details]
Patch

[Kevin Turner]

(In reply to Simon Fraser (smfr) from comment #20)

> > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:154
> > +            // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations
> > +            return CalculationCategory::Other;
> 
> I can't tell if the line you copied above with the comment represents a TODO
> or correct behavior.

If you are referring to the comment of "The type of a min(), max()...", I was attempting to keep this comment up to date since I am adding 'hypot()' with this commit. Do you think we should remove this comment?

[Simon Fraser (smfr)]

(In reply to Kevin Turner from comment #22)
> (In reply to Simon Fraser (smfr) from comment #20)
> 
> > > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:154
> > > +            // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations
> > > +            return CalculationCategory::Other;
> > 
> > I can't tell if the line you copied above with the comment represents a TODO
> > or correct behavior.
> 
> If you are referring to the comment of "The type of a min(), max()...", I
> was attempting to keep this comment up to date since I am adding 'hypot()'
> with this commit. Do you think we should remove this comment?

I don't know! Code above does more complex things to figure out the category. Do min max, clamp etc need to also do something more complex?

[Kevin Turner]

(In reply to Simon Fraser (smfr) from comment #23)
> (In reply to Kevin Turner from comment #22)
> > (In reply to Simon Fraser (smfr) from comment #20)
> > 
> > > > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:154
> > > > +            // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations
> > > > +            return CalculationCategory::Other;
> > > 
> > > I can't tell if the line you copied above with the comment represents a TODO
> > > or correct behavior.
> > 
> > If you are referring to the comment of "The type of a min(), max()...", I
> > was attempting to keep this comment up to date since I am adding 'hypot()'
> > with this commit. Do you think we should remove this comment?
> 
> I don't know! Code above does more complex things to figure out the
> category. Do min max, clamp etc need to also do something more complex?

From my understanding of the spec for these functions is that hypot() should have the same category as min, max and clamp and pow() and sqrt() should behave purely off of numbers.

I realize now though that in rebasing this the case statements for ::other are repeated needlessly. Updated patch incoming.

[Kevin Turner]

Created attachment 436830 [details]
Patch

[Kevin Turner]

Created attachment 436835 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Kevin Turner]

Created attachment 436836 [details]
Patch

[Kevin Turner]

Created attachment 436837 [details]
Patch

[Kevin Turner]

An update of css-values WPT is being tracked here https://bugs.webkit.org/show_bug.cgi?id=229696

[Kevin Turner]

Created attachment 436898 [details]
Patch

[Darin Adler]

Comment on attachment 436898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436898&action=review

Unclear on how good the test coverage is for all this code. There are a lot of subtle things here to get right or wrong, and the tests are critical.

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:131
> +    // TODO: sin, cos, tan, asin, acos, atan, atan2.

While modifying this, should change from the non-WebKit-style TODO to a WebKit FIXME

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:181
> +    // TODO: sin, cos, tan, asin, acos, atan, atan2.

Ditto.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:151
> +            // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations

Not new wording, but what does "adding the types" mean here?

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:332
> +    for (auto& child : values) {
> +        if (child->category() != CalculationCategory::Number)
> +            return nullptr;
> +    }

I think we should make a helper function that returns whether all children have the same category, and returns that category or nullopt if they aren’t all the same category (or if the vector is empty). We could use this to share the code for PowOrSqrt and Hypot easily and read it carefully.

    if (commonCategory(values) != CalculationCategory::Number)
        return nullptr;

    static std::optional<CalculationCategory> commonCategory(const Vector<Ref<CSSCalcExpressionNode>>& vector)
    {
        // ...
    }

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:346
> +    for (unsigned i = 1; i < values.size(); ++i) {

Strange to use "unsigned" here when Vector indices are size_t.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:643
> +        // Don't simplify out sqrt() or hypot() before they are evaluated.

This comment is nice and short, but sadly it says *what* we do, but not why. Our primary job in comments is to answer the question, "Why?"

> Source/WebCore/css/calc/CSSCalcValue.cpp:186
> +        case CalcOperator::Sqrt: {
> +            auto children = createCSS(operationChildren, style);
> +            if (children.isEmpty())
> +                return nullptr;
> +            return CSSCalcOperationNode::createPowOrSqrt(CalcOperator::Sqrt, WTFMove(children));
> +        }
> +        case CalcOperator::Pow: {
> +            auto children = createCSS(operationChildren, style);
> +            if (children.isEmpty())
> +                return nullptr;
> +            return CSSCalcOperationNode::createPowOrSqrt(CalcOperator::Pow, WTFMove(children));
> +        }

Please merge this into a single case like the case above for Min/Max/Clamp.

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:89
> +    case CalcOperator::Pow: {

Really confused about how CSSCalcValue uses double, and CalcExpressionOperation uses float.

[Kevin Turner]

Created attachment 436968 [details]
Patch

[Kevin Turner]

(In reply to Darin Adler from comment #32)
> Comment on attachment 436898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436898&action=review
> 
> Unclear on how good the test coverage is for all this code. There are a lot
> of subtle things here to get right or wrong, and the tests are critical.
> 
I will look at expanding the tests included, I've been thinking through more edge cases and extreme cases to account for. 

> 
> Really confused about how CSSCalcValue uses double, and
> CalcExpressionOperation uses float.

Not sure about this incongruence either, I can't think of a reason why they shouldn't be the same.

[Radar WebKit Bug Importer]

<rdar://problem/82640883>

[Kevin Turner]

Created attachment 437175 [details]
Patch

[Kevin Turner]

(In reply to Kevin Turner from comment #34)
> (In reply to Darin Adler from comment #32)
> > Comment on attachment 436898 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=436898&action=review
> > 
> > Unclear on how good the test coverage is for all this code. There are a lot
> > of subtle things here to get right or wrong, and the tests are critical.
> > 
> I will look at expanding the tests included, I've been thinking through more
> edge cases and extreme cases to account for. 
> 
> > 
> > Really confused about how CSSCalcValue uses double, and
> > CalcExpressionOperation uses float.
> 
> Not sure about this incongruence either, I can't think of a reason why they
> shouldn't be the same.

It seems the discrepancy between CSSCalcValue using double, and CalcExpressionOperation using float is a result of CalcExpressionOperation specifically being used with percentage (%) based calculations and using float in service to that. Not familiar with the background in this area however.

[Simon Fraser (smfr)]

We should probably use double everywhere.

[Kevin Turner]

Created attachment 437888 [details]
Patch

[Kevin Turner]

Rebased to include trig implementations landed with https://bugs.webkit.org/show_bug.cgi?id=229507.

[Kevin Turner]

I've filed a separate bug to track standardizing on using double everywhere: https://bugs.webkit.org/show_bug.cgi?id=230161

[Darin Adler]

Comment on attachment 437888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437888&action=review

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:607
> +    if ((isMinOrMaxNode() || isExponentialFuncNode()) && canCombineAllChildren()) {

Not obvious why this is min/max/exponential; what makes that set of node types special?

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:677
> +        // Don't simplify out sqrt(), hypot() or trig functions before they are evaluated because they do not behave as an identity function with one argument.
> +        if (calcOperationNode.isIdentity() && depth)

This comment seems a little backwards and doesn’t really match the code below. If we need a comment explaining why some functions aren’t identity functions, that probably belongs inside the implementation of isIdentity. Here, the code is almost self-explanatory given that function’s name.

With this new refactoring, the non-obvious thing about this code is now the fact that isIdentity guarantees that there is only one child node. To me seems not super-obvious so might be worth mentioning in a comment.

I would write a different comment.

[Kevin Turner]

Comment on attachment 437888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437888&action=review

>> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:607
>> +    if ((isMinOrMaxNode() || isExponentialFuncNode()) && canCombineAllChildren()) {
> 
> Not obvious why this is min/max/exponential; what makes that set of node types special?

The checks may not be strictly necessary - I added `isExponentialFuncNode()` to the conditional since the combination-of-children logic here is what we want for these new functions. It seems like we're just enumerating which functions could have this combining of children logic applied in this way, so perhaps this enumeration of types would be better served as part of `canCombineAllChildren()` since it is only called from here.

[Kevin Turner]

W(In reply to Kevin Turner from comment #43)
> Comment on attachment 437888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437888&action=review
> 
> >> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:607
> >> +    if ((isMinOrMaxNode() || isExponentialFuncNode()) && canCombineAllChildren()) {
> > 
> > Not obvious why this is min/max/exponential; what makes that set of node types special?
> 
> The checks may not be strictly necessary - I added `isExponentialFuncNode()`
> to the conditional since the combination-of-children logic here is what we
> want for these new functions. It seems like we're just enumerating which
> functions could have this combining of children logic applied in this way,
> so perhaps this enumeration of types would be better served as part of
> `canCombineAllChildren()` since it is only called from here.

Actually, we already check for specific node types before calling combineChildren() in simplyNode() so these checks at the end here are surely redundant. We can just check canCombineAllChildren() to make sure the types of the children are correct for combining.

[Kevin Turner]

Created attachment 437900 [details]
Patch

[Darin Adler]

Comment on attachment 437900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437900&action=review

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:198
> +    // FIXME: implement asin, acos, atan, atan2.

Could put this FIXME after all the cases (as it was before) instead of in the middle of the cases (as it is now).

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1100
> +        if (value < 0)
> +            return std::numeric_limits<double>::quiet_NaN();

This isn’t needed, since std::sqrt already does this when passed a negative number.

[Kevin Turner]

Created attachment 438004 [details]
Patch

[Darin Adler]

Comment on attachment 438004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438004&action=review

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:676
> +        // Identity nodes have only one child and perform no operation on their child

WebKit coding style puts periods at the ends of sentence comments like this one.

[Kevin Turner]

Created attachment 438097 [details]
Patch

[Darin Adler]

Comment on attachment 438097 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438097&action=review

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:102
> +        if (value < 0)
> +            return std::numeric_limits<float>::quiet_NaN();

This isn’t needed, since std::sqrt already does this when passed a negative number.

[Kevin Turner]

Created attachment 438385 [details]
Patch

[Kevin Turner]

Created attachment 439221 [details]
Patch

[Kevin Turner]

Created attachment 439223 [details]
Patch

[Kevin Turner]

Created attachment 439392 [details]
Patch

[Nikos Mouchtaris]

Comment on attachment 439392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439392&action=review

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:472
> +    if (commonCategory(values) != CalculationCategory::Number)

Should check that pow has exactly 2 children and that Sqrt has exactly 1 child.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:679
> +    if (m_children.size() < 2 && !isPowOrSqrt() && !isHypot()) {

Is including isPowOrSqrt/isHypot here necessary?

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:101
> +        float base = m_children[0]->evaluate(maxValue);

Don't need local variables.

> LayoutTests/ChangeLog:10
> +        * fast/css/calc-parsing.html:

I would add a couple parsing tests to fast/css/calc-parsing.html. This would have caught the issue with pow/sqrt.

[Kevin Turner]

Created attachment 439434 [details]
Patch

[Kevin Turner]

Created attachment 439494 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 439494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439494&action=review

> Source/WebCore/ChangeLog:11
> +        Test: fast/css/calc-parsing.html
> +        
> +        Implements pow(), sqrt() and hypot() functions as specified by https://drafts.csswg.org/css-values-4/#exponent-funcs.

Test line goes after the comments (with a blank line between)

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:352
> +    for (size_t i = 1; i < values.size(); ++i) {

for (const auto& value : values)

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:487
> +    if (expectedCategory != CalculationCategory::Number && expectedCategory != CalculationCategory::Length && expectedCategory != CalculationCategory::Percent)

This needs to allow Time, Frequency, and maybe PercentNumber/PercentLength? Needs tests that validate this.

> LayoutTests/ChangeLog:10
> +        * fast/css/calc-parsing-expected.txt:
> +        * fast/css/calc-parsing.html:

Need some WPT here.

[Darin Adler]

Comment on attachment 439494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439494&action=review

>> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:352
>> +    for (size_t i = 1; i < values.size(); ++i) {
> 
> for (const auto& value : values)

That idiom is not ideal here, because it doesn’t skip the first item in the vector. Or we could write it this way and redundantly check the category of item 0; wouldn’t hurt correctness.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #59)
> Comment on attachment 439494 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439494&action=review
> 
> >> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:352
> >> +    for (size_t i = 1; i < values.size(); ++i) {
> > 
> > for (const auto& value : values)
> 
> That idiom is not ideal here, because it doesn’t skip the first item in the
> vector. Or we could write it this way and redundantly check the category of
> item 0; wouldn’t hurt correctness.

Oh I missed the 1.

[Kevin Turner]

Created attachment 439522 [details]
Patch

[Kevin Turner]

Nikos ([e-mail]) has agreed to assist me in getting WPT tests for this up in a separate patch that, to land before this one.

[Kevin Turner]

Created attachment 439691 [details]
Patch

[Kevin Turner]

Created attachment 439694 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 439694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439694&action=review

Have we imported WPT? Normally we'd import those as failing, then a patch like this would mark them as passing.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:980
> +        if (calcOperationNode.isHypotNode())

Not clear why hypot doesn't have the depth check.

> Source/WebCore/css/calc/CSSCalcOperationNode.h:76
> +    bool isIdentity() const { return m_children.size() == 1 && (m_operator == CalcOperator::Min || m_operator == CalcOperator::Max); }

isIdentity() is a confusing name here, hiding the fact that this only applies to min and max.

[Kevin Turner]

(In reply to Simon Fraser (smfr) from comment #65)
> Comment on attachment 439694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439694&action=review
> 
> Have we imported WPT? Normally we'd import those as failing, then a patch
> like this would mark them as passing.

I will check with Nikos for the WPT status.

> > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:980
> > +        if (calcOperationNode.isHypotNode())
> 
> Not clear why hypot doesn't have the depth check.

This is due to hypot() being able to take units directly like min() and max(), instead of having to be nested in a calc() function without units like the nodes with the depth check.  

> > Source/WebCore/css/calc/CSSCalcOperationNode.h:76
> > +    bool isIdentity() const { return m_children.size() == 1 && (m_operator == CalcOperator::Min || m_operator == CalcOperator::Max); }
> 
> isIdentity() is a confusing name here, hiding the fact that this only
> applies to min and max.

I believe the discussion that landed us on this naming was to have a generic way to determine if a given node would apply any operation to its child, which does only applies to min/max() with one child. Would you recommend we inline this check and do away with the potential confusion?

[Darin Adler]

Comment on attachment 439694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439694&action=review

>>> Source/WebCore/css/calc/CSSCalcOperationNode.h:76
>>> +    bool isIdentity() const { return m_children.size() == 1 && (m_operator == CalcOperator::Min || m_operator == CalcOperator::Max); }
>> 
>> isIdentity() is a confusing name here, hiding the fact that this only applies to min and max.
> 
> I believe the discussion that landed us on this naming was to have a generic way to determine if a given node would apply any operation to its child, which does only applies to min/max() with one child. Would you recommend we inline this check and do away with the potential confusion?

Simon, I’m not sure this is confusing.

This function should return true for any node that is an identity operation, making no change to value from the single child it has.

The implementation reflects that this is true only for min and max. But the function doesn’t only "apply" to min and max. It correctly returns false for other types of nodes.

Or perhaps this should also return true for sum or product with exactly one child?

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #67)
> Comment on attachment 439694 [details]

> Or perhaps this should also return true for sum or product with exactly one
> child?

It was exactly this kind of question I had in mind.

[Kevin Turner]

Created attachment 439781 [details]
Patch

[Kevin Turner]

(In reply to Simon Fraser (smfr) from comment #68)
> (In reply to Darin Adler from comment #67)
> > Comment on attachment 439694 [details]
> 
> > Or perhaps this should also return true for sum or product with exactly one
> > child?
> 
> It was exactly this kind of question I had in mind.

I think that makes sense to do. I expanded isIdentity() to include Add and Multiply operations with one child and I see no regressions. Will post an updated patch.

[Kevin Turner]

Created attachment 439795 [details]
Patch

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Kevin Turner]

Created attachment 439825 [details]
Patch

[EWS]

Committed r283359 (242368@main): <https://commits.webkit.org/242368@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439825 [details].