[webkit-reviews] review denied: [Bug 78446] CSS3 calc: embed calc expressions in CSSPrimitiveValue : [Attachment 126697] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 78446: CSS3 calc: embed calc expressions in CSSPrimitiveValue
https://bugs.webkit.org/show_bug.cgi?id=78446

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126697&action=review


R- just for the multiplier issue.

> Source/WebCore/css/CSSCalculationValue.cpp:85
> +double CSSCalcValue::computeLengthPx(RenderStyle* currStyle, RenderStyle*
rootStyle, double multiplier, bool computingFontSize) const

nit: s/currStyle/currentStyle. here and below

> Source/WebCore/css/CSSPrimitiveValue.cpp:59
> +    case CSSPrimitiveValue:: CSS_CALC_PERCENTAGE_NUMBER:
> +    case CSSPrimitiveValue:: CSS_CALC_PERCENTAGE_LENGTH:

I find these names a little confusing. It's a bit verbose, but how about
CSS_CALC_PERCENTAGE_WITH_NUMBER and CSS_CALC_PERCENTAGE_WITH_LENGTH?

> Source/WebCore/css/CSSPrimitiveValue.cpp:485
> +    double value;

How about s/value/computedValue?

> Source/WebCore/css/CSSPrimitiveValue.cpp:487
> +	   value = m_value.calc->computeLengthPx(style, rootStyle, 1.0,
computingFontSize);

Should you not be passing multiplier instead of 1.0? If 1.0 is correct, then
this at least needs a comment explaining why.

> Source/WebCore/css/CSSPrimitiveValue.h:141
> +	   const unsigned short type = primitiveType();

I don't think the const buys you anything here, does it?