[webkit-reviews] review granted: [Bug 79188] CSS3 calc(): handle non-negative values : [Attachment 128127] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 79188: CSS3 calc(): handle non-negative values
https://bugs.webkit.org/show_bug.cgi?id=79188

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=128127&action=review


> Source/WebCore/ChangeLog:8
> +	   Some CSS properties (eg padding) are required to be non-negative.
These

Nit: eg => e.g.

("e.g." is an acronym for the Latin phrase "example gratia")

> Source/WebCore/ChangeLog:16
> +	   (WebCore::CSSCalcValue::clampToRange):

For your consideration, I suggest adding the word "Added."  to right of the ':'
for all methods that were added in this patch. This makes it straightforward to
identify the added methods from reading the change log message and also helps
when searching on Trac for a change that introduced a method.

> Source/WebCore/css/CSSCalculationValue.cpp:84
> +double CSSCalcValue::clampToRange(double value) const
> +{
> +    return m_nonNegative && value < 0 ? 0 : value;
> +}	

I don't really like the name of this method or doubleValue() as they are
somewhat disingenuous, but I can't think of a better name at the moment. I
mean, the value is only clamped to 0 if its negative and its permitted value
range is non-negative.

> Source/WebCore/css/CSSCalculationValue.cpp:90
>  double CSSCalcValue::doubleValue() const 
>  { 
> +    return clampToRange(doubleValueUnclamped());
> +}
> +

I take it that you've ensured that all call sites of doubleValue() have been
changed to doubleValueUnclamped() since you've changed the meaning of
doubleValue() from being an unclamped value to a "clamped value" (see my remark
for clampToRange() above).

> Source/WebCore/css/CSSCalculationValue.cpp:91
> +double CSSCalcValue::doubleValueUnclamped() const 

You may want to consider inlining this function in the header since it's a
simple expression and its referenced in doubleValue().

> Source/WebCore/css/CSSParser.cpp:710
> +    if (!parseCalculation(value, unitflags & FNonNeg ?
CalculationRangeNonNegative : CalculationRangeAll))

I suggest defining a variable, say isNonNegativeValue, for the expression
"unitflags & FNonNeg" since it appears three times in this code block and is
evaluated at most twice.