Implement units for CSS typed om
Created attachment 456083 [details] Patch
Created attachment 456089 [details] Patch
Created attachment 456093 [details] Patch
Created attachment 456126 [details] Patch
Created attachment 456136 [details] Patch
Comment on attachment 456136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456136&action=review > Source/WebCore/ChangeLog:3 > + Implement units for CSS typed om "CSS Typed OM" > Source/WebCore/css/typedom/CSSUnitValue.cpp:81 > + if (unit == "number"_s) Case sensitive comparison is OK? > Source/WebCore/css/typedom/CSSUnitValue.h:59 > + CSSUnitType m_unit; This can be const > Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:39 > +static std::optional<CSSNumericType> addTypes(CSSNumericType a, CSSNumericType b) You used const CSSNumericType& arguments for multiply. > Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:67 > +ExceptionOr<Ref<CSSMathSum>> CSSMathSum::create(FixedVector<CSSNumberish> numberishes) const FixedVector&? > Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:72 > +ExceptionOr<Ref<CSSMathSum>> CSSMathSum::create(Vector<Ref<CSSNumericValue>> values) const Vector&?
Comment on attachment 456136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456136&action=review >> Source/WebCore/css/typedom/CSSUnitValue.cpp:81 >> + if (unit == "number"_s) > > Case sensitive comparison is OK? Yes, CSSParserToken::stringToUnitType (used by the CSS parser) is case sensitive and so is this. >> Source/WebCore/css/typedom/CSSUnitValue.h:59 >> + CSSUnitType m_unit; > > This can be const Wonderful! >> Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:39 >> +static std::optional<CSSNumericType> addTypes(CSSNumericType a, CSSNumericType b) > > You used const CSSNumericType& arguments for multiply. Multiplying two types can be done in a way that doesn't need to mutate a type so I used const CSSNumericType& there to avoid unnecessary copies. The strangeness of the percent hint operations in this type addition algorithm necessitate copying. >> Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:67 >> +ExceptionOr<Ref<CSSMathSum>> CSSMathSum::create(FixedVector<CSSNumberish> numberishes) > > const FixedVector&? FixedVector&& would do the minimal number of copies, but so does this! Sam opened my eyes to using less && in the code and still having the minimum number of vector copies and I kind of like it. >> Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:72 >> +ExceptionOr<Ref<CSSMathSum>> CSSMathSum::create(Vector<Ref<CSSNumericValue>> values) > > const Vector&? ditto
Comment on attachment 456136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456136&action=review >> Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:67 >> +ExceptionOr<Ref<CSSMathSum>> CSSMathSum::create(FixedVector<CSSNumberish> numberishes) > > const FixedVector&? Since this doesn’t take ownership of the FixedVector, I suggest we take a Span<const CSSNumberish>. That lets us pass a FixedVector, or a Vector, or anything else array-like. Just need to make sure that WTF::map knows how to work with a span as an argument. >> Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:72 >> +ExceptionOr<Ref<CSSMathSum>> CSSMathSum::create(Vector<Ref<CSSNumericValue>> values) > > const Vector&? This takes ownership of the Vector, so I think it should not be const& or a Span, needs to either be Vector or Vector&&, really a stylistic choice which we prefer in such cases. I think Vector&& normally to emphasize the fact that it takes ownership and make it less likely we’ll accidentally copy a vector. > Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:87 > +CSSMathSum::CSSMathSum(Vector<Ref<CSSNumericValue>> values, CSSNumericType type) Same thought here, maybe Vector&& is the way to go, as the old code did. > Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:88 > + : CSSMathValue(WTFMove(type)) This struct is made up of scalars, so there is no benefit to calling WTFMove on it. I suppose it’s a stylistic choice whether to just write WTFMove anyway just in case some day this becomes an optimization? > Source/WebCore/css/typedom/numeric/CSSNumericArray.cpp:41 > +Ref<CSSNumericArray> CSSNumericArray::create(FixedVector<CSSNumberish>&& numberishes) Since this doesn’t take ownership of the FixedVector, I suggest we take a Span<const CSSNumberish>. That lets us pass a FixedVector, or a Vector, or anything else array-like. Just need to make sure that WTF::map knows how to work with a span as an argument.
Created attachment 456198 [details] Patch
Comment on attachment 456198 [details] Patch Darin's Span suggestion is great, but since those create functions are called from the generated bindings which pass in a FixedVector the change would be more involved. I leave that for future work.
Committed r292150 (249057@main): <https://commits.webkit.org/249057@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 456198 [details].
<rdar://problem/91095475>