Bug 80411

Created attachment 190684 [details] Patch

(In reply to comment #1) > Created an attachment (id=190684) [details] > Patch Added parse time detection of divide by 0 cases. This patch also comes with simplification of number expressions. As part of the patch I have cleaned up trailing whitespace in CSSCalculationValue.cpp and CSSCalculationValue.h. Please let me know if this should be done in a separate bug.

Created attachment 190689 [details] Patch

(In reply to comment #3) > Created an attachment (id=190689) [details] > Patch Same code patch as before. Split the simplification and catch divide by 0 tests apart.

Comment on attachment 190689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190689&action=review I'm not a reviewer, but I'll do a shadow review. > Source/WebCore/css/CSSCalculationValue.cpp:91 > + return result.toString(); These whitespace only changes are typically discouraged. > Source/WebCore/css/CSSCalculationValue.cpp:258 > + Extra whitespace block? > Source/WebCore/css/CSSCalculationValue.cpp:462 > + bool isSimplifiableExpression(CSSCalcExpressionNode* lhs, CSSCalcExpressionNode* rhs) Do you intend to extend this in the future for eg 25% * 2 = 50%. ie this would ultimately be return lhs->category() == CalcNumber || rhs->category() == CalcNumber; > Source/WebCore/css/CSSCalculationValue.cpp:467 > + // This assumes you have checked lhs and rhs with isSimplifiableExpression already. Rather than a comment like this, add an ASSERT at the top of the function. > Source/WebCore/css/CSSCalculationValue.cpp:-515 > - result->value = CSSCalcBinaryOperation::create(result->value, rhs.value, static_cast<CalcOperator>(operatorCharacter)); Did you consider putting all this logic inside the CSSCalcBinaryOperation::create function? It seems like a better place for it to live. It would mean you wouldn't have cut'n'pastes like you have down on line 576. You'd have to change the signature to return a PassRefPtr<CSSCalcExpressionNode> instead, and I haven't completely thought through the consequences of this. > LayoutTests/css3/calc/cssom.html:-17 > - "(100px) + 200px", I don't understand why this line changed?

Thanks for the shadow review Mike. I have cancelled the patch as I believe this bug was already resolved before I even started. Simplifying the calc expression detects the divide by 0 case just a tiny bit quicker. This existing line was already catching them at parse time: http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSCalculationValue.cpp#L288 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. I am open as to where to go from here with calc simplification.

Created attachment 190905 [details] Patch

(In reply to comment #7) > Created an attachment (id=190905) [details] > Patch Added just the divide by 0 tests without code to show this bug is already resolved.

I don't think this bug is completely resolved, as the intention was to handle divide by zero 'in all cases'. The spec (http://www.w3.org/TR/css3-values/#calc-notation) says "If an operator does not pass the above checks, the expression is invalid. Also, division by zero is invalid. This includes both dividing by the literal number zero, as well as any numeric expression that evaluates to zero (as purely-numeric expressions can be evaluated without any additional information at parse time)." So until we are simplifying the purely numeric cases such as calc(100px / (5 - 5)) this implementation isn't strictly compliant.

(In reply to comment #9) > I don't think this bug is completely resolved, as the intention was to handle divide by zero 'in all cases'. The spec (http://www.w3.org/TR/css3-values/#calc-notation) says "If an operator does not pass the above checks, the expression is invalid. Also, division by zero is invalid. This includes both dividing by the literal number zero, as well as any numeric expression that evaluates to zero (as purely-numeric expressions can be evaluated without any additional information at parse time)." > > So until we are simplifying the purely numeric cases such as calc(100px / (5 - 5)) this implementation isn't strictly compliant. The original code does catch cases like calc(100px / (5 - 5)). When isZero() is called on (5 - 5) it invokes an evaluation of the expression and returns true. These tests all pass on ToT: https://bugs.webkit.org/attachment.cgi?id=190905&action=prettypatch

Comment on attachment 190905 [details] Patch This seems OK to me. Mike, did you have any further comments?

> The original code does catch cases like calc(100px / (5 - 5)). When isZero() is called on (5 - 5) it invokes an evaluation of the expression and returns true. > These tests all pass on ToT: https://bugs.webkit.org/attachment.cgi?id=190905&action=prettypatch Ah - good point. I was cleverer than I remember. Thanks Alan! > This seems OK to me. Mike, did you have any further comments? No, all looks good. Thanks Tony!

Comment on attachment 190905 [details] Patch Clearing flags on attachment: 190905 Committed r144689: <http://trac.webkit.org/changeset/144689>

All reviewed patches have been landed. Closing bug.