Bug 203442 – calc() serialization doesn't match the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

We fail a lot of calc() WPT because our serialization doesn't match the spec. There are a few problems:
* we fail to simplify calc(1 + -2) to calc(1 - 2)
* we don't sort the arguments
* we serialize min(a, b) as calc(min(a, b))
* we serialize calc(a + b + c) as calc(a + (b + c))

[Radar WebKit Bug Importer]

<rdar://problem/56639402>

[Simon Fraser (smfr)]

Have a mostly-working patch for this.

[Simon Fraser (smfr)]

Created attachment 384721 [details]
Patch

[Dean Jackson]

Comment on attachment 384721 [details]
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.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/253079/webkit

[Simon Fraser (smfr)]

Test cleanup in https://trac.webkit.org/changeset/253089/webkit