Bug 241167 - Implement CSSNumericValue.to
Summary: Implement CSSNumericValue.to
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-31 21:30 PDT by Alex Christensen
Modified: 2022-06-03 13:00 PDT (History)
8 users (show)

See Also:


Attachments
Patch (41.98 KB, patch)
2022-05-31 21:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (45.16 KB, patch)
2022-06-03 11:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2022-05-31 21:30:47 PDT
Some strange algorithms, but well specified.
Comment 1 Alex Christensen 2022-05-31 21:34:31 PDT
Created attachment 459920 [details]
Patch
Comment 2 Chris Dumez 2022-06-03 09:45:30 PDT
Comment on attachment 459920 [details]
Patch

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

r=me but please fix the parameter types to use r-value references.

> Source/WebCore/css/typedom/CSSUnitValue.h:53
> +    RefPtr<CSSUnitValue> convertTo(CSSUnitType);

Can this be const?

> Source/WebCore/css/typedom/numeric/CSSMathMax.cpp:42
> +ExceptionOr<Ref<CSSMathMax>> CSSMathMax::create(FixedVector<CSSNumberish> numberishes)

I commented the same thing on an earlier patch but I would really prefer if we used FixedVector<CSSNumberish>&&, especially for copyable types.
While the flexibility to copy or move may sound nice in theory. I fear that people won't be careful and do unnecessary copies.

> Source/WebCore/css/typedom/numeric/CSSMathMax.cpp:47
> +ExceptionOr<Ref<CSSMathMax>> CSSMathMax::create(Vector<Ref<CSSNumericValue>> values)

Vector<Ref<CSSNumericValue>>&&

> Source/WebCore/css/typedom/numeric/CSSMathMax.cpp:59
> +CSSMathMax::CSSMathMax(Vector<Ref<CSSNumericValue>> values, CSSNumericType type)

Vector<Ref<CSSNumericValue>>&&

> Source/WebCore/css/typedom/numeric/CSSMathMax.cpp:91
> +    for (size_t i = 1; i < valuesArray.size(); i++) {

++i

> Source/WebCore/css/typedom/numeric/CSSMathMin.cpp:40
> +ExceptionOr<Ref<CSSMathMin>> CSSMathMin::create(FixedVector<CSSNumberish> numberishes)

FixedVector<CSSNumberish>&&

> Source/WebCore/css/typedom/numeric/CSSMathMin.cpp:45
> +ExceptionOr<Ref<CSSMathMin>> CSSMathMin::create(Vector<Ref<CSSNumericValue>> values)

Vector<Ref<CSSNumericValue>>&&

> Source/WebCore/css/typedom/numeric/CSSMathMin.cpp:57
> +CSSMathMin::CSSMathMin(Vector<Ref<CSSNumericValue>> values, CSSNumericType type)

Vector<Ref<CSSNumericValue>>&&

> Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:88
> +    auto convertToNumericType= [] (const UnitMap& units) -> std::optional<CSSNumericType> {

missing space before =

> Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:123
> +    for (size_t i = 1; i < values.size(); i++) {

++i
Comment 3 Alex Christensen 2022-06-03 11:47:11 PDT
Created attachment 460018 [details]
Patch
Comment 4 EWS 2022-06-03 12:59:36 PDT
Committed r295219 (251276@main): <https://commits.webkit.org/251276@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 460018 [details].
Comment 5 Radar WebKit Bug Importer 2022-06-03 13:00:14 PDT
<rdar://problem/94361549>