Bug 238532 – Implement units for CSS typed om

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Implement units for CSS typed om

[Alex Christensen]

Created attachment 456083 [details]
Patch

[Alex Christensen]

Created attachment 456089 [details]
Patch

[Alex Christensen]

Created attachment 456093 [details]
Patch

[Alex Christensen]

Created attachment 456126 [details]
Patch

[Alex Christensen]

Created attachment 456136 [details]
Patch

[Simon Fraser (smfr)]

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&?

[Alex Christensen]

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

[Darin Adler]

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.

[Alex Christensen]

Created attachment 456198 [details]
Patch

[Alex Christensen]

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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/91095475>