[webkit-reviews] review granted: [Bug 203442] calc() serialization doesn't match the spec : [Attachment 384721] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 203442: calc() serialization doesn't match the spec
https://bugs.webkit.org/show_bug.cgi?id=203442

Attachment 384721: Patch

https://bugs.webkit.org/attachment.cgi?id=384721&action=review




--- Comment #4 from Dean Jackson <dino at apple.com> ---
Comment on attachment 384721
  --> https://bugs.webkit.org/attachment.cgi?id=384721
Patch

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

> Source/WebCore/css/CSSCalculationValue.cpp:556
> +    double computeLengthPx(const CSSToLengthConversionData& conversionData)
const final  { return 1.0 / m_child->computeLengthPx(conversionData); }

Could conversionData ever provide a 0?

> Source/WebCore/css/CSSCalculationValue.cpp:1026
> +		   // If nodes contains any dimensions, remove them from nodes,
sort them by their units, ordered ASCII case-insensitively, and append them to
ret.

This comment isn't what you're doing here though right? You're just sorting by
ASCII.

> Source/WebCore/css/CSSCalculationValue.cpp:1329
>      Vector<double> doubleValues;
> -    for (auto& child : m_children)
> -	   doubleValues.append(child->doubleValue());
> +    doubleValues.reserveInitialCapacity(m_children.size());
> +    bool allowNumbers = calcOperator() == CalcOperator::Multiply;
> +    for (auto& child : m_children) {
> +	   CSSUnitType childType = unitType;
> +	   if (allowNumbers && unitType != CSSUnitType::CSS_NUMBER &&
child->primitiveType() == CSSUnitType::CSS_NUMBER)
> +	       childType = CSSUnitType::CSS_NUMBER;
> +	   doubleValues.uncheckedAppend(child->doubleValue(childType));
> +    }

You could do evaluate(m_children.map([&] (auto& child) {
	...
	return child->doubleValue(childType);
    })) 

and avoid the local variable + reserveInitialCapacity.

> Source/WebCore/css/CSSCalculationValue.cpp:1339
>      return evaluate(doubleValues);

Ditto.

> Source/WebCore/css/CSSCalculationValue.cpp:1873
> +    return values;

Another place where you could .map()

> Source/WebCore/platform/CalculationValue.cpp:253
> -    case CalcOperator::Min: ts << "max"; break;
> -    case CalcOperator::Max: ts << "min"; break;
> +    case CalcOperator::Min: ts << "min"; break;
> +    case CalcOperator::Max: ts << "max"; break;

Wow.