Bug 111149 – Simplify parsed CSS Calc expressions

• Component: New Bugs
• Status: NEW

[Alan Cutter]

Simplify parsed CSS Calc expressions

[Alan Cutter]

Created attachment 190909 [details]
Patch

[Alan Cutter]

This bug was split from bug 80411.

[Alan Cutter]

From bug 80411:

> In terms of simplification I wonder how worthwhile it is to do. To simplify the expression tree would blow the linear parsing complexity out (please correct me on this if I'm mistaken).
> Example: multiplying ((var(a) * 4px) - (12 * var(b))) by the number 8 would require a traversal of the tree or some other bookkeeping data structure. This would occur for every token in the expression.
> 
> It is possible to reduce the calc expression into a dimension and a multiplier instead of an expression tree to preserve the linear time parsing however that would leave little room for css variables in calc expressions.
> 
> If the calc expression is going to be evaluated hundreds of times perhaps the simplification could be a deferred process that occurs iteratively as the expression is evaluated again and again.
>

[Ojan Vafai]

Comment on attachment 190909 [details]
Patch

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

I'd like Mike to do the initial pass here since he's most familiar with the calc code. I just have some high-level style comments.

> Source/WebCore/css/CSSCalculationValue.cpp:91
> -    return result.toString(); 
> +    return result.toString();

Here and elsewhere, please don't fix up the style/whitespace of blocks/functions where you not actually modifying the code as well. It makes reviews harder as well as doing code archaeology (i.e. it adds another hop you need to jump through in order to figure out why a line of code was added).

> Source/WebCore/css/CSSCalculationValue.cpp:260
> +/* CalcNumber        */ { CalcNumber,        CalcOther,         CalcPercentNumber, CalcPercentNumber, CalcOther },

WebKit style is typically to avoid /* */ comments. You'd usually just put these at the end of the line using a "//" comment.

[Mike Lawther]

Comment on attachment 190909 [details]
Patch

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

Looking good Alan!

> Source/WebCore/css/CSSCalculationValue.cpp:313
> +    static PassRefPtr<CSSCalcExpressionNode> createSimplified(PassRefPtr<CSSCalcExpressionNode> leftSide, PassRefPtr<CSSCalcExpressionNode> rightSide, CalcOperator op)

Is there really any time that we *don't* want to simplify the expression? I can't think of one off the top of my head. I'd prefer to keep the interface to a single 'create' function and just do the simplification in there.

> Source/WebCore/css/CSSCalculationValue.cpp:315
> +        // FIXME: Extend simplification to incorporate other categories.

Nit - please, no FIXMEs without a bug. Please file a new bug for this, and reference it here in this comment.

> Source/WebCore/css/CSSCalculationValue.cpp:318
> +            if (evaluation != evaluation) // Only true for NaN.

You should change the comment to "// Only true for NaN, unless -ffast-math is turned on for GCC" :)

While I agree with the theory, there are compiler options that will cause this to be optimised away and misbehave. C99 and C++11 provide functions like std::isnan() (http://en.cppreference.com/w/cpp/numeric/math/isnan), and WebKit uses these (see eg https://bugs.webkit.org/show_bug.cgi?id=109325).

> Source/WebCore/css/CSSCalculationValue.cpp:410
> +        : CSSCalcExpressionNode(category, op != CalcDivide && leftSide->isInteger() && rightSide->isInteger())

Ah - I see why you're doing this. But it's still not right, as 10 / 5 is an integer. You need to check leftSide->isInteger() && rightSide->isInteger() && (op != CalcDivide || (left % right == 0))

Now that I see you have it in two places, it should be split out to a common function.

[Alan Cutter]

(In reply to comment #4)
> (From update of attachment 190909 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190909&action=review
> 
> I'd like Mike to do the initial pass here since he's most familiar with the calc code. I just have some high-level style comments.
> 
> > Source/WebCore/css/CSSCalculationValue.cpp:91
> > -    return result.toString(); 
> > +    return result.toString();
> 
> Here and elsewhere, please don't fix up the style/whitespace of blocks/functions where you not actually modifying the code as well. It makes reviews harder as well as doing code archaeology (i.e. it adds another hop you need to jump through in order to figure out why a line of code was added).

I understand, will take out style fixes.
It just seems a shame that style problems may never get fixed!

> 
> > Source/WebCore/css/CSSCalculationValue.cpp:260
> > +/* CalcNumber        */ { CalcNumber,        CalcOther,         CalcPercentNumber, CalcPercentNumber, CalcOther },
> 
> WebKit style is typically to avoid /* */ comments. You'd usually just put these at the end of the line using a "//" comment.

Will put the row headers at end of line.


(In reply to comment #5)
> (From update of attachment 190909 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190909&action=review
> 
> Looking good Alan!

Thanks!

> 
> > Source/WebCore/css/CSSCalculationValue.cpp:313
> > +    static PassRefPtr<CSSCalcExpressionNode> createSimplified(PassRefPtr<CSSCalcExpressionNode> leftSide, PassRefPtr<CSSCalcExpressionNode> rightSide, CalcOperator op)
> 
> Is there really any time that we *don't* want to simplify the expression? I can't think of one off the top of my head. I'd prefer to keep the interface to a single 'create' function and just do the simplification in there.

I'm hesitant to have a create function that might not return a ref to an instance of the actual class, it seems to go against the create idiom that WebKit uses and I don't want readers of the code to be mislead. Perhaps removing create and keeping createSimplified?
 
> > Source/WebCore/css/CSSCalculationValue.cpp:315
> > +        // FIXME: Extend simplification to incorporate other categories.
> 
> Nit - please, no FIXMEs without a bug. Please file a new bug for this, and reference it here in this comment.

Still not sure if further simplification is desired. Will remove the comment.
(The simplification this patch does appears to be equivalent to the level of simplification that Firefox performs).

> 
> > Source/WebCore/css/CSSCalculationValue.cpp:318
> > +            if (evaluation != evaluation) // Only true for NaN.
> 
> You should change the comment to "// Only true for NaN, unless -ffast-math is turned on for GCC" :)
> 
> While I agree with the theory, there are compiler options that will cause this to be optimised away and misbehave. C99 and C++11 provide functions like std::isnan() (http://en.cppreference.com/w/cpp/numeric/math/isnan), and WebKit uses these (see eg https://bugs.webkit.org/show_bug.cgi?id=109325).

Thanks for the links, using std::isnan sounds like a much better solution. I'll make sure I understand all the implications in using it first.

> 
> > Source/WebCore/css/CSSCalculationValue.cpp:410
> > +        : CSSCalcExpressionNode(category, op != CalcDivide && leftSide->isInteger() && rightSide->isInteger())
> 
> Ah - I see why you're doing this. But it's still not right, as 10 / 5 is an integer. You need to check leftSide->isInteger() && rightSide->isInteger() && (op != CalcDivide || (left % right == 0))

I did consider this as well however the spec wants us to be sloppy: "At ‘/’, check that the right side is <number>. If the left side is <integer>, resolve to <number>. Otherwise, resolve to the type of the left side."
http://www.w3.org/TR/css3-values/#calc-type-checking

> 
> Now that I see you have it in two places, it should be split out to a common function.

Will do.

[Alan Cutter]

Created attachment 191151 [details]
Patch

[Alan Cutter]

(In reply to comment #7)
> Created an attachment (id=191151) [details]
> Patch

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 190909 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190909&action=review
> > 
> > > Source/WebCore/css/CSSCalculationValue.cpp:260
> > > +/* CalcNumber        */ { CalcNumber,        CalcOther,         CalcPercentNumber, CalcPercentNumber, CalcOther },
> > 
> > WebKit style is typically to avoid /* */ comments. You'd usually just put these at the end of the line using a "//" comment.
> 
> Will put the row headers at end of line.

Putting the row headers at the end of the line made the data harder to read and violated the "only one space before EOL comments" style guide. I did not see any entry in the style guide about block comments either. I have opted to keep the row headers on the left.

[Darin Adler]

Comment on attachment 191151 [details]
Patch

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

I don’t know enough about CSS3 calc to be sure this is OK, but I’m assuming it is. Would be nice to know how this is supposed to stay interoperable with other implementations and maybe see tests that are part of the standard.

cq- because of the unneeded static_cast

> Source/WebCore/ChangeLog:13
> +        (WebCore):

Please remove bogus lines like this from the change log before landing, and if possible fix the script that generates these bogus lines.

> Source/WebCore/ChangeLog:15
> +        (CSSCalcBinaryOperation):

Please remove bogus lines like this from the change log before landing, and if possible fix the script that generates these bogus lines.

> Source/WebCore/css/CSSCalculationValue.cpp:308
> +            if (std::isnan(evaluation))
> +                return 0;

Do we also need a check for infinity here? Maybe isfinite is the better test to use.

> Source/WebCore/css/CSSCalculationValue.cpp:312
> +            RefPtr<CSSPrimitiveValue> evaluatedPrimitive = CSSPrimitiveValue::create(evaluation, evaluationType);
> +            return CSSCalcPrimitiveValue::create(static_cast<CSSPrimitiveValue*>(evaluatedPrimitive.get()), isInteger);

Why the typecast here? There should be no need for that. Also, I don’t think we need a local value fro evaluatedPrimitive.

    return CSSCalcPrimitiveValue::create(CSSPrimitiveValue::create(evaluation, evaluationType).get(), isInteger);

> Source/WebCore/css/CSSCalculationValue.cpp:435
> +    static bool evaluateIsInteger(CSSCalcExpressionNode* leftSide, CSSCalcExpressionNode* rightSide, CalcOperator op)
> +    {
> +        return op != CalcDivide && leftSide->isInteger() && rightSide->isInteger();
> +    }

I think this is an awkward stilted function name. But it seems to be along the lines of the rest of the functions in this class. But this really doesn’t even evaluate a value that determines if a result is an integer. It checks if a value is guaranteed to be integral, but doesn’t take into account issues such as overflow, nor does it detect that 4/2 is an integer. I think a better name could make it clearer that this is a sort of “best try” at proving something could be an integer. And I’d like to see test cases that show the overflow thing is not a problem.

[Alan Cutter]

Thanks for the review Darin.

(In reply to comment #9)
> (From update of attachment 191151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191151&action=review
> 
> I don’t know enough about CSS3 calc to be sure this is OK, but I’m assuming it is. Would be nice to know how this is supposed to stay interoperable with other implementations and maybe see tests that are part of the standard.
> 
> cq- because of the unneeded static_cast
> 
> > Source/WebCore/ChangeLog:13
> > +        (WebCore):
> 
> Please remove bogus lines like this from the change log before landing, and if possible fix the script that generates these bogus lines.
> 
> > Source/WebCore/ChangeLog:15
> > +        (CSSCalcBinaryOperation):
> 
> Please remove bogus lines like this from the change log before landing, and if possible fix the script that generates these bogus lines.
> 

Will do.

> > Source/WebCore/css/CSSCalculationValue.cpp:308
> > +            if (std::isnan(evaluation))
> > +                return 0;
> 
> Do we also need a check for infinity here? Maybe isfinite is the better test to use.
> 

std::numeric_limits<double>::quiet_NaN() is explicitly returned by the evaluateOperator function so that's what it's checking for. You make a good point about checking for infinity as well. I will add that check to the if.
http://www.w3.org/TR/css3-values/#numeric-types
"UAs should support reasonably useful ranges and precisions."

> > Source/WebCore/css/CSSCalculationValue.cpp:312
> > +            RefPtr<CSSPrimitiveValue> evaluatedPrimitive = CSSPrimitiveValue::create(evaluation, evaluationType);
> > +            return CSSCalcPrimitiveValue::create(static_cast<CSSPrimitiveValue*>(evaluatedPrimitive.get()), isInteger);
> 
> Why the typecast here? There should be no need for that. Also, I don’t think we need a local value fro evaluatedPrimitive.
> 
>     return CSSCalcPrimitiveValue::create(CSSPrimitiveValue::create(evaluation, evaluationType).get(), isInteger);
> 

My mistake, that's a remnant of what used to be a downcast. Will remove.

> > Source/WebCore/css/CSSCalculationValue.cpp:435
> > +    static bool evaluateIsInteger(CSSCalcExpressionNode* leftSide, CSSCalcExpressionNode* rightSide, CalcOperator op)
> > +    {
> > +        return op != CalcDivide && leftSide->isInteger() && rightSide->isInteger();
> > +    }
> 
> I think this is an awkward stilted function name. But it seems to be along the lines of the rest of the functions in this class. But this really doesn’t even evaluate a value that determines if a result is an integer. It checks if a value is guaranteed to be integral, but doesn’t take into account issues such as overflow, nor does it detect that 4/2 is an integer. I think a better name could make it clearer that this is a sort of “best try” at proving something could be an integer. And I’d like to see test cases that show the overflow thing is not a problem.

The function name in this case is somewhat misleading. It's actually checking if the result of the operation is of integer type according to the spec. This classifies all divide operations as non-integers.
http://www.w3.org/TR/css3-values/#calc-type-checking
I don't like the name "evaluateIsInteger" either, it was chosen so as not to conflict with the existing isInteger. Perhaps it should be called isIntegerOperation and placed outside of the CSSCalcBinaryOperation class to avoid this conflict. I'll add a comment stating that it is determining the integer type rather than reality with a link to the spec.

[Mike Lawther]

> > > +    static bool evaluateIsInteger(CSSCalcExpressionNode* leftSide, CSSCalcExpressionNode* > > 

> > I think this is an awkward stilted function name.

> I don't like the name "evaluateIsInteger" either, it was chosen so as not to conflict with the existing isInteger. Perhaps it should be called isIntegerOperation and placed outside of the CSSCalcBinaryOperation class to avoid this conflict. I'll add a comment stating that it is determining the integer type rather than reality with a link to the spec.

isIntegerResult()?

[Alan Cutter]

Created attachment 191882 [details]
Patch

[Build Bot]

Comment on attachment 191882 [details]
Patch

Attachment 191882 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17050271

[Build Bot]

Comment on attachment 191882 [details]
Patch

Attachment 191882 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17005133

[Alan Cutter]

Created attachment 193491 [details]
Patch

[Alan Cutter]

(In reply to comment #15)
> Created an attachment (id=193491) [details]
> Patch

Extended simplification to CSS unit values as well as numbers.

Example:
calc(((100px + 20 * 5px) * 10 - 5 * (10em * 5 + 10em)) * 2)
becomes
calc((2000px - 300em) * 2)

The previous simplification patch would not have altered the expression at all.

[Build Bot]

Comment on attachment 193491 [details]
Patch

Attachment 193491 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17180398

[Alan Cutter]

Created attachment 193497 [details]
Patch

[Mike Lawther]

Comment on attachment 193497 [details]
Patch

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

> Source/WebCore/css/CSSCalculationValue.cpp:363
> +    case CSSPrimitiveValue::CSS_PARSER_INTEGER:

How come CSS_PARSER_INTEGER is not simplifiable?

> Source/WebCore/css/CSSCalculationValue.cpp:417
> +        // Simplify multiplying or dividing by a number for simplifiable types.

This comment doesn't match the code below?

> Source/WebCore/css/CSSCalculationValue.cpp:418
> +        if (op != CalcMultiply && op != CalcDivide)

I'd prefer to see if (op == CalcAdd || op == CalcSubtract) if that's what you mean. It's less of a consideration, but 'mod' may reappear in the spec at some point.

> Source/WebCore/css/CSSCalculationValue.cpp:419
> +            return create(leftSide, rightSide, op);

tbh, I 'd prefer to see lines 410-419 look more like:
if (op == CalcAdd || op == CalcSubtract) {
  if (leftType == rightSide->primitiveType() && isSimplifiableType(leftType)) {
     return CSSCalcPrimitiveValue::create(evaluateOperator(leftSide->doubleValue(), rightSide->doubleValue(), op), leftType, isInteger);
  return create(leftSide, rightSide, op);
}

Since that's what these two blocks boil down to? Now the reader is clear that Add and Subtract has been dealt with in this block, and any remaining code must be only Multiply and Divide.

> Source/WebCore/css/CSSCalculationValue.cpp:421
> +        CSSCalcExpressionNode* numberSide = pickByCategory(leftSide.get(), rightSide.get(), CalcNumber);

The purpose of this function wasn't obvious to me. Unless you can forsee needing this to be general, I'd consider renaming this to 'chooseNumberSide'/'findNumberSide'  or something, and remove the category parameter.

> Source/WebCore/css/CSSCalculationValue.cpp:523
> +        case CalcNumber:

If it's of category calcNumber,  both left and right must be CalcNumber right? So can the contents of the first if block below be moved up to this case?

> Source/WebCore/css/CSSCalculationValue.cpp:556
> +        : CSSCalcExpressionNode(category, isIntegerResult(leftSide.get(), rightSide.get(), op))

Do you need the .get()'s?

[Alan Cutter]

(In reply to comment #19)
> (From update of attachment 193497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193497&action=review
> 

Thanks for the review Mike.

> > Source/WebCore/css/CSSCalculationValue.cpp:363
> > +    case CSSPrimitiveValue::CSS_PARSER_INTEGER:
> 
> How come CSS_PARSER_INTEGER is not simplifiable?
> 

Overlooked that one, will fix.

> > Source/WebCore/css/CSSCalculationValue.cpp:417
> > +        // Simplify multiplying or dividing by a number for simplifiable types.
> 
> This comment doesn't match the code below?
> 
> > Source/WebCore/css/CSSCalculationValue.cpp:418
> > +        if (op != CalcMultiply && op != CalcDivide)
> 
> I'd prefer to see if (op == CalcAdd || op == CalcSubtract) if that's what you mean. It's less of a consideration, but 'mod' may reappear in the spec at some point.
> 

This comment was really for the entire section below it but I see now that's not so obvious. Maybe it should be explicitly grouped like:
// Simplify multiplying or dividing by a number for simplifiable types.
if (op == CalcMultiply || op == CalcDivide) {
    Simplify...
}

> > Source/WebCore/css/CSSCalculationValue.cpp:419
> > +            return create(leftSide, rightSide, op);
> 
> tbh, I 'd prefer to see lines 410-419 look more like:
> if (op == CalcAdd || op == CalcSubtract) {
>   if (leftType == rightSide->primitiveType() && isSimplifiableType(leftType)) {
>      return CSSCalcPrimitiveValue::create(evaluateOperator(leftSide->doubleValue(), rightSide->doubleValue(), op), leftType, isInteger);
>   return create(leftSide, rightSide, op);
> }
> 
> Since that's what these two blocks boil down to? Now the reader is clear that Add and Subtract has been dealt with in this block, and any remaining code must be only Multiply and Divide.
> 

Your rearrangement makes the code clearer and has a more efficient ordering of if conditions. Will adopt.

> > Source/WebCore/css/CSSCalculationValue.cpp:421
> > +        CSSCalcExpressionNode* numberSide = pickByCategory(leftSide.get(), rightSide.get(), CalcNumber);
> 
> The purpose of this function wasn't obvious to me. Unless you can forsee needing this to be general, I'd consider renaming this to 'chooseNumberSide'/'findNumberSide'  or something, and remove the category parameter.
> 

Will rename to getNumberSide (since it's a member function on CSSCalcBinaryOperation) and remove category parameter.

> > Source/WebCore/css/CSSCalculationValue.cpp:523
> > +        case CalcNumber:
> 
> If it's of category calcNumber,  both left and right must be CalcNumber right? So can the contents of the first if block below be moved up to this case?
> 

This is a great observation, will rearrange and add an ASSERT that both sides are CalcNumbers so it's clear to readers this should be the case.

> > Source/WebCore/css/CSSCalculationValue.cpp:556
> > +        : CSSCalcExpressionNode(category, isIntegerResult(leftSide.get(), rightSide.get(), op))
> 
> Do you need the .get()'s?

Yes, PassRefPtrs don't auto convert to raw pointers, so says the compiler. Most likely a safety decision.

[Alan Cutter]

Created attachment 194192 [details]
Patch

[Alan Cutter]

(In reply to comment #21)
> Created an attachment (id=194192) [details]
> Patch

 - Made modifications as per review.
 - Renamed isSimplifiableType to hasDoubleValue and used it in CSSCalcPrimitiveValue::getDoubleValue().

[Mike Lawther]

Comment on attachment 194192 [details]
Patch

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

Overall lgtm. Would 'r+ with comments' if I was a reviewer :)

> Source/WebCore/css/CSSCalculationValue.cpp:81
> +static bool hasDoubleValue(CSSPrimitiveValue::UnitTypes type)

I like this name better :)

> Source/WebCore/css/CSSCalculationValue.cpp:267
>              return m_value->getDoubleValue();

I'm wondering if it would be better to just ASSERT(hasDoubleValue(primitiveType()), as I thought that getDoubleValue() would return 0 anyway. However, getDoubleValue() allows a calc values, while hasDoubleValue would reject these. So it's best to leave it as is.

> Source/WebCore/css/CSSCalculationValue.cpp:407
> +        if (op == CalcMultiply || op == CalcDivide) {

At this point, the op *must* be Multiply or Divide right? If you're worried about illegal values, ASSERT at the top of this function that op is either add, subtract, divide or multiply.

> LayoutTests/css3/calc/simplification.html:18
> +];

Can you throw in some tests for 
  calc(100px + 1in + 100px) = calc(100px + 1in + 100px)
  calc(100px + 1em + 100px) = calc(100px + 1em + 100px)

 just to clarify that this form of expression won't simplify?

Then if you want to do further simplification by converting all possible values to px at parse time (ie everything except em,rem,ex and %) you can 'fix' that first case.

[Alan Cutter]

Created attachment 195217 [details]
Patch

[Alan Cutter]

Created attachment 195219 [details]
Patch

[Alan Cutter]

(In reply to comment #23)
> (From update of attachment 194192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194192&action=review
> 
> Overall lgtm. Would 'r+ with comments' if I was a reviewer :)
> 
> > Source/WebCore/css/CSSCalculationValue.cpp:81
> > +static bool hasDoubleValue(CSSPrimitiveValue::UnitTypes type)
> 
> I like this name better :)
> 

Thanks. (:

> > Source/WebCore/css/CSSCalculationValue.cpp:407
> > +        if (op == CalcMultiply || op == CalcDivide) {
> 
> At this point, the op *must* be Multiply or Divide right? If you're worried about illegal values, ASSERT at the top of this function that op is either add, subtract, divide or multiply.
> 

The execution may reach this point if the expression is an add or subtract expression that was not simplified in the earlier if statement.

> > LayoutTests/css3/calc/simplification.html:18
> > +];
> 
> Can you throw in some tests for 
>   calc(100px + 1in + 100px) = calc(100px + 1in + 100px)
>   calc(100px + 1em + 100px) = calc(100px + 1em + 100px)
> 
>  just to clarify that this form of expression won't simplify?
> 

Added extra tests.

> Then if you want to do further simplification by converting all possible values to px at parse time (ie everything except em,rem,ex and %) you can 'fix' that first case.

Added type conversion between compatible types for further simplification.
100px + 1in => 196px

[Mike Lawther]

Comment on attachment 195219 [details]
Patch

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

> Source/WebCore/css/CSSCalculationValue.cpp:414
> +                            return CSSCalcPrimitiveValue::create(evaluateOperator(leftValue, rightValue, op), canonicalType, isInteger);

Nice.

> Source/WebCore/css/CSSCalculationValue.cpp:422
> +        if (op == CalcMultiply || op == CalcDivide) {

This should be an 'else if', as it's mutually exclusive to the above test for Add or Subtract, and control can pass out of that top block and do a unnecessary test here. I don't say this for optimisation reasons, but so nobody wonders if it's possible to enter both of these if statements.

Either that or early return at the end of the if (add || subtract) block as I had suggested in my original suggestion for reworking this code. I didn't notice this in the previous patch - when you'd said you'd adopt the suggestion I missed that the early return wasn't used.

[Alan Cutter]

Created attachment 195226 [details]
Patch

[Alan Cutter]

(In reply to comment #27)
> (From update of attachment 195219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195219&action=review
> 

Thanks for the speedy review!

> > Source/WebCore/css/CSSCalculationValue.cpp:422
> > +        if (op == CalcMultiply || op == CalcDivide) {
> 
> This should be an 'else if', as it's mutually exclusive to the above test for Add or Subtract, and control can pass out of that top block and do a unnecessary test here. I don't say this for optimisation reasons, but so nobody wonders if it's possible to enter both of these if statements.
> 
> Either that or early return at the end of the if (add || subtract) block as I had suggested in my original suggestion for reworking this code. I didn't notice this in the previous patch - when you'd said you'd adopt the suggestion I missed that the early return wasn't used.

This makes sense, changed into an else and made the if condition an ASSERT for clarity and to catch any unexpected operators.

(In reply to comment #28)
> Created an attachment (id=195226) [details]
> Patch

 - Updated unittest that was failing on Safari.

[Mike Lawther]

lgtm - would r+ :)

[Brent Fulgham]

Ben -- should we take this? It's been sitting dormant for a long time, but seems like a good idea.

[Benjamin Poulain]

(In reply to comment #31)
> Ben -- should we take this? It's been sitting dormant for a long time, but
> seems like a good idea.

While simplifying the expressions seems like a good idea, doing it at parse time is dubious.

This is directly observable through CSSOM. Is there anything in CSSOM that allows us to do this kind of resolution?

[Darin Adler]

Comment on attachment 195226 [details]
Patch

As Ben said, wrong to do this in a way that is visible to JavaScript through the CSS object model. If we want to optimize we need to do it separately from parsing.

[Ahmad Saleem]

>>> [e-mail] - what is difference between this and bug 117782 ?

Link - https://bugs.webkit.org/show_bug.cgi?id=117782

[Ryosuke Niwa]

*** Bug 117782 has been marked as a duplicate of this bug. ***

[Alan Cutter]

It is perhaps dubious but certainly matches precedence. If you set { left: 1e0px; } the CSSOM will read it back as { left: 1px; }. As long as the behaviour is unchanged and it round trips back to the same thing a second time it should be considered okay as a parser transformation. The dubious part comes in whether there's any behaviour in CSS that relies on the expression shape of a calc() (e.g. interpolation?).

[Radar WebKit Bug Importer]

<rdar://problem/97694269>