[webkit-reviews] review granted: [Bug 111149] Simplify parsed CSS Calc expressions : [Attachment 191151] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alan Cutter
<alancutter at chromium.org>'s request for review:
Bug 111149: Simplify parsed CSS Calc expressions
https://bugs.webkit.org/show_bug.cgi?id=111149

Attachment 191151: Patch
https://bugs.webkit.org/attachment.cgi?id=191151&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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*>(evaluatedPrimitiv
e.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.