REOPENED 230795
Add implementation for CSSNumericValue math functions
https://bugs.webkit.org/show_bug.cgi?id=230795
Summary Add implementation for CSSNumericValue math functions
Qiaosong Zhou
Reported 2021-09-25 04:10:29 PDT
Add implementation for CSSNumericValue math functions
Attachments
Patch (3.64 KB, patch)
2021-09-25 04:11 PDT, Qiaosong Zhou
no flags
Patch (3.64 KB, patch)
2021-09-25 04:33 PDT, Qiaosong Zhou
no flags
Patch (5.52 KB, patch)
2021-09-25 12:32 PDT, Qiaosong Zhou
no flags
Patch (24.66 KB, patch)
2021-11-23 18:54 PST, Qiaosong Zhou
no flags
Patch (35.31 KB, patch)
2021-11-23 19:01 PST, Qiaosong Zhou
no flags
Patch (35.42 KB, patch)
2021-11-24 10:18 PST, Qiaosong Zhou
no flags
Patch (36.01 KB, patch)
2021-11-24 21:50 PST, Qiaosong Zhou
no flags
Patch (36.13 KB, patch)
2022-01-04 11:59 PST, Qiaosong Zhou
no flags
Qiaosong Zhou
Comment 1 2021-09-25 04:11:20 PDT
Qiaosong Zhou
Comment 2 2021-09-25 04:33:26 PDT
Qiaosong Zhou
Comment 3 2021-09-25 12:32:29 PDT
Radar WebKit Bug Importer
Comment 4 2021-10-02 04:11:15 PDT
Qiaosong Zhou
Comment 5 2021-11-20 15:28:35 PST
*** Bug 233400 has been marked as a duplicate of this bug. ***
Qiaosong Zhou
Comment 6 2021-11-23 18:54:40 PST
Qiaosong Zhou
Comment 7 2021-11-23 18:58:37 PST
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
Comment 8 2021-11-23 19:01:52 PST
Simon Fraser (smfr)
Comment 9 2021-11-24 09:48:32 PST
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
Comment 10 2021-11-24 10:18:31 PST
Simon Fraser (smfr)
Comment 11 2021-11-24 10:43:17 PST
Comment on attachment 445099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445099&action=review > Source/WebCore/ChangeLog:1 > +2021-11-23 Johnson Zhou <johnson.zhou.sh@icloud.com> 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
Comment 12 2021-11-24 10:50:56 PST
(In reply to Simon Fraser (smfr) from comment #11) > > Source/WebCore/ChangeLog:1 > > +2021-11-23 Johnson Zhou <johnson.zhou.sh@icloud.com> > > 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)
Comment 13 2021-11-24 12:50:22 PST
(In reply to Qiaosong Zhou from comment #12) > (In reply to Simon Fraser (smfr) from comment #11) > > > > Source/WebCore/ChangeLog:1 > > > +2021-11-23 Johnson Zhou <johnson.zhou.sh@icloud.com> > > > > 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
Comment 14 2021-11-24 20:45:50 PST
(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
Comment 15 2021-11-24 21:50:51 PST
Chris Dumez
Comment 16 2021-11-29 12:21:17 PST
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
Comment 17 2022-01-04 11:59:15 PST
Antoine Quint
Comment 19 2022-03-01 09:51:17 PST
Scratch that, those tests rely on CSSNumericValue.parse().
Antoine Quint
Comment 20 2023-01-18 00:36:15 PST
This test now passes with STP 161.
Antoine Quint
Comment 21 2023-01-18 00:37:41 PST
Sorry, closed this early, but it's not only about the Web Animations tests.
Note You need to log in before you can comment on or make changes to this bug.