Bug 230795 – Add implementation for CSSNumericValue math functions

• Status: REOPENED
• Component: CSS

[Qiaosong Zhou]

Add implementation for CSSNumericValue math functions

[Qiaosong Zhou]

Created attachment 439259 [details]
Patch

[Qiaosong Zhou]

Created attachment 439260 [details]
Patch

[Qiaosong Zhou]

Created attachment 439268 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83796319>

[Qiaosong Zhou]

*** Bug 233400 has been marked as a duplicate of this bug. ***

[Qiaosong Zhou]

Created attachment 445056 [details]
Patch

[Qiaosong Zhou]

Passing all tests in http://wpt.live/css/css-typed-om/stylevalue-subclasses/numeric-objects/arithmetic.tentative.html except the ones concerning CSSNumericType, which will be added in a separate patch.

[Qiaosong Zhou]

Created attachment 445057 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 445057 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

You need a description here of what this change is. If you're implementing a spec, provide a link to the spec.

[Qiaosong Zhou]

Created attachment 445099 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 445099 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Johnson Zhou  <[e-mail]>

This should be your Apple email address.

> Source/WebCore/ChangeLog:6
> +        Add implementation for CSSNumericValue math functions according to CSS Typed OM Spec. 
> +        https://drafts.css-houdini.org/css-typed-om-1/#numeric-value
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=230795

The correct format here is prepared for you by "webkit-patch upload":

    Add implementation for CSSNumericValue math functions
    https://bugs.webkit.org/show_bug.cgi?id=230795

    Add implementations of the CSS Types OM numeric operations: https://drafts.css-houdini.org/css-typed-om-1/#numeric-value

and add additional notes if there is anything interesting or confusing in the implementation that needs to be explained.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:60
> +        if (!is<CSSUnitValue>(*it) || downcast<CSSUnitValue>(numericVector.begin()->get()).unit() != downcast<CSSUnitValue>(it->get()).unit())

Might be worth copying downcast<CSSUnitValue>(numericVector.begin()->get()).unit() into a local variable.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:102
> +    // FIXME: Lacks impl. of "Let type be the result of adding the types of every item in values. If type is failure, throw a TypeError."

You should file a bugzilla bug for this work and reference it here.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:114
> +    if (is<CSSMathSum>(*this)) {

It might be worth adding links to the spec for these; for example, at https://drafts.css-houdini.org/css-typed-om-1/#dom-cssnumericvalue-add, it's useful to know that the values from |this| are prepended.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:147
> +    

Blank line.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:156
> +    // FIXME: Lacks impl. of "Let type be the result of adding the types of every item in values. If type is failure, throw a TypeError."

File and reference a bug.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:225
> +            if (value < minValue)
> +                minValue = value;

Can we just use std::min()?

> Source/WebCore/css/typedom/CSSNumericValue.cpp:256
> +            if (value > maxValue)
> +                maxValue = value;

Can we just use std::max()?

> Source/WebCore/css/typedom/CSSNumericValue.cpp:320
> +    if (is<CSSUnitValue>(*this))
> +        return CSSUnitValue::create(-downcast<CSSUnitValue>(*this).value(), downcast<CSSUnitValue>(*this).unit());

Put downcast<CSSUnitValue>(*this) into a local variable.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:334
> +    if (is<CSSUnitValue>(*this) && downcast<CSSUnitValue>(*this).unit() == "number") {
> +        if (downcast<CSSUnitValue>(*this).value() == 0.0)
> +            return Exception { RangeError, "Cannot invert CSSUnitValue with value 0.0 or -0.0"_s };
> +    
> +        return Ref<CSSNumericValue> { CSSUnitValue::create(1.0 / downcast<CSSUnitValue>(*this).value(), downcast<CSSUnitValue>(*this).unit()) };

Put downcast<CSSUnitValue>(*this) into a local variable.

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

We don't generally use "get" in function names. I suggest:

commonUnit()
unitForProduct()

> Source/WebCore/css/typedom/CSSNumericValue.h:55
>      Ref<CSSNumericValue> add(Vector<CSSNumberish>&&);
>      Ref<CSSNumericValue> sub(Vector<CSSNumberish>&&);
>      Ref<CSSNumericValue> mul(Vector<CSSNumberish>&&);
> -    Ref<CSSNumericValue> div(Vector<CSSNumberish>&&);
> +    ExceptionOr<Ref<CSSNumericValue>> div(Vector<CSSNumberish>&&);
>      Ref<CSSNumericValue> min(Vector<CSSNumberish>&&);
>      Ref<CSSNumericValue> max(Vector<CSSNumberish>&&);

I think these should all be const functions, right? They don't change the value of |this|. Ideally we'd name them something different to make this more clear, but this naming matches the spec.

> Source/WebCore/css/typedom/CSSNumericValue.h:69
> +    Ref<CSSNumericValue> addImpl(Vector<Ref<CSSNumericValue>>&&);
> +    Ref<CSSNumericValue> mulImpl(Vector<Ref<CSSNumericValue>>&&);

Can these be const?

[Qiaosong Zhou]

(In reply to Simon Fraser (smfr) from comment #11)

> > Source/WebCore/ChangeLog:1
> > [phone]  Johnson Zhou  <[e-mail]>
> 
> This should be your Apple email address.
> 

I'm no longer working at Apple, should I still use my Apple email that I no longer have access to?

[Simon Fraser (smfr)]

(In reply to Qiaosong Zhou from comment #12)
> (In reply to Simon Fraser (smfr) from comment #11)
> 
> > > Source/WebCore/ChangeLog:1
> > > [phone]  Johnson Zhou  <[e-mail]>
> > 
> > This should be your Apple email address.
> > 
> 
> I'm no longer working at Apple, should I still use my Apple email that I no
> longer have access to?

I see! Probably not.

[Qiaosong Zhou]

(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 445099 [details]
> 
> > Source/WebCore/css/typedom/CSSNumericValue.h:55
> >      Ref<CSSNumericValue> add(Vector<CSSNumberish>&&);
> >      Ref<CSSNumericValue> sub(Vector<CSSNumberish>&&);
> >      Ref<CSSNumericValue> mul(Vector<CSSNumberish>&&);
> > -    Ref<CSSNumericValue> div(Vector<CSSNumberish>&&);
> > +    ExceptionOr<Ref<CSSNumericValue>> div(Vector<CSSNumberish>&&);
> >      Ref<CSSNumericValue> min(Vector<CSSNumberish>&&);
> >      Ref<CSSNumericValue> max(Vector<CSSNumberish>&&);
> 
> I think these should all be const functions, right? They don't change the
> value of |this|. Ideally we'd name them something different to make this
> more clear, but this naming matches the spec.
> 
> > Source/WebCore/css/typedom/CSSNumericValue.h:69
> > +    Ref<CSSNumericValue> addImpl(Vector<Ref<CSSNumericValue>>&&);
> > +    Ref<CSSNumericValue> mulImpl(Vector<Ref<CSSNumericValue>>&&);
> 
> Can these be const?

I can't seem to make them const because I'm converting *this into a Ref that is then prepended into the Vector.

[Qiaosong Zhou]

Created attachment 445117 [details]
Patch

[Chris Dumez]

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

[Qiaosong Zhou]

Created attachment 448316 [details]
Patch

[Antoine Quint]

Lack of support for CSSNumericValue is causing two unique failures for Web Animations tests:

https://wpt.fyi/results/web-animations/timing-model/animations/setting-the-current-time-of-an-animation.html?label=experimental&label=master&aligned

https://wpt.fyi/results/web-animations/timing-model/animations/setting-the-start-time-of-an-animation.html?label=experimental&label=master&aligned

[Antoine Quint]

Scratch that, those tests rely on CSSNumericValue.parse().

[Antoine Quint]

This test now passes with STP 161.

[Antoine Quint]

Sorry, closed this early, but it's not only about the Web Animations tests.