[Webkit-unassigned] [Bug 230795] Add implementation for CSSNumericValue math functions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230795

--- Comment #16 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 445117
  --> https://bugs.webkit.org/attachment.cgi?id=445117
Patch

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

> Source/WebCore/css/typedom/CSSNumericValue.cpp:54
> +inline const String* CSSNumericValue::commonUnit(const Vector<Ref<CSSNumericValue>>& numericVector)

Should probably return a String instead of a String*. A String is already a pointer (to a StringImpl) and already has a "null" state.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:57
> +        return nullptr;

return { };

> Source/WebCore/css/typedom/CSSNumericValue.cpp:61
> +    for (auto it = numericVector.begin(); it != numericVector.end(); ++it) {

It looks like this loop is unnecessarily checking the first item in numericVector again.

I think I would write this like so:
```
if (numericVector.isEmpty())
    return { };

String unit;
for (auto& numericValue : numericVector) {
    if (!is<CSSUnitValue>(numericValue.get()))
        return { };
    auto& unitValue = downcast<CSSUnitValue>(numericValue.get());
    if (unit.isNull())
        unit = unitValue.unit();
    else if (unitValue.unit() != unit)
        return { };
}

return unit;
```

> Source/WebCore/css/typedom/CSSNumericValue.cpp:63
> +            return nullptr;

return { };

> Source/WebCore/css/typedom/CSSNumericValue.cpp:69
> +inline const String* CSSNumericValue::unitForProduct(const Vector<Ref<CSSNumericValue>>& numericVector)

Same comment, seems this should return a `String`.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:178
> +        numericVector.append(Ref<CSSNumericValue> { *this });

`Ref<CSSNumericValue> { *this }` -> `Ref { *this }`

> Source/WebCore/css/typedom/CSSNumericValue.cpp:190
> +        return Ref<CSSNumericValue> { *this };

return Ref { *this };

> Source/WebCore/css/typedom/CSSNumericValue.cpp:198
> +        numericVector.append(Ref<CSSNumericValue> { *this });

Ref { *this }

> Source/WebCore/css/typedom/CSSNumericValue.cpp:222
> +        numericVector.append(Ref<CSSNumericValue> { *this });

Ref { *this }

> Source/WebCore/css/typedom/CSSNumericValue.cpp:251
> +        numericVector.append(Ref<CSSNumericValue> { *this });

Ref { *this }

> Source/WebCore/css/typedom/CSSNumericValue.cpp:327
> +    return CSSMathNegate::create(RefPtr<CSSNumericValue> { this });

RefPtr { this }

> Source/WebCore/css/typedom/CSSNumericValue.cpp:344
> +    return Ref<CSSNumericValue> { CSSMathInvert::create(RefPtr<CSSNumericValue> { this }) };

RefPtr { this }

> Source/WebCore/css/typedom/CSSNumericValue.h:47
> +    const String* commonUnit(const Vector<Ref<CSSNumericValue>>&);

Why is this public? Does it even need to be in the header? Can be be a static function in the cpp only?

> Source/WebCore/css/typedom/CSSNumericValue.h:48
> +    const String* unitForProduct(const Vector<Ref<CSSNumericValue>>&);

ditto.

> Source/WebCore/css/typedom/numeric/CSSMathInvert.h:38
> +    Ref<CSSNumericValue> value() const { return m_value.copyRef(); }

Not sure this change is actually useful. The caller can construct a Ref<> if they need one. Otherwise, it may cause ref counting churn unnecessarily.

> Source/WebCore/css/typedom/numeric/CSSMathMax.h:45
>      CSSMathMax(Vector<CSSNumberish>&&);

explicit.

> Source/WebCore/css/typedom/numeric/CSSMathMax.h:46
> +    CSSMathMax(Vector<Ref<CSSNumericValue>>&&);

explicit.

> Source/WebCore/css/typedom/numeric/CSSMathMin.h:45
>      CSSMathMin(Vector<CSSNumberish>&&);

explicit.

> Source/WebCore/css/typedom/numeric/CSSMathMin.h:46
> +    CSSMathMin(Vector<Ref<CSSNumericValue>>&&);

explicit.

> Source/WebCore/css/typedom/numeric/CSSMathNegate.h:41
> +    Ref<CSSNumericValue> value() const { return m_value.copyRef(); }

Not convinced this change is a good idea.

> Source/WebCore/css/typedom/numeric/CSSMathProduct.h:47
>      CSSMathProduct(Vector<CSSNumberish>&&);

explicit.

> Source/WebCore/css/typedom/numeric/CSSMathProduct.h:48
> +    CSSMathProduct(Ref<CSSNumericArray>&&);

explicit.

> Source/WebCore/css/typedom/numeric/CSSMathSum.h:47
> +    CSSMathSum(Ref<CSSNumericArray>&&);

explicit.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211129/2f84ab5d/attachment.htm>