Bug 238532

Summary: Implement units for CSS typed om
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Alex Christensen
Reported 2022-03-29 17:52:09 PDT
Implement units for CSS typed om
Attachments
Patch (67.23 KB, patch)
2022-03-29 17:55 PDT, Alex Christensen
no flags
Patch (64.86 KB, patch)
2022-03-29 20:48 PDT, Alex Christensen
no flags
Patch (62.10 KB, patch)
2022-03-29 22:26 PDT, Alex Christensen
no flags
Patch (63.12 KB, patch)
2022-03-30 08:48 PDT, Alex Christensen
no flags
Patch (63.95 KB, patch)
2022-03-30 10:10 PDT, Alex Christensen
no flags
Patch (64.00 KB, patch)
2022-03-30 20:44 PDT, Alex Christensen
no flags
Alex Christensen
Comment 1 2022-03-29 17:55:56 PDT
Alex Christensen
Comment 2 2022-03-29 20:48:40 PDT
Alex Christensen
Comment 3 2022-03-29 22:26:30 PDT
Alex Christensen
Comment 4 2022-03-30 08:48:59 PDT
Alex Christensen
Comment 5 2022-03-30 10:10:38 PDT
Simon Fraser (smfr)
Comment 6 2022-03-30 19:35:38 PDT
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 7 2022-03-30 20:23:33 PDT
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 8 2022-03-30 20:27:49 PDT
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
Comment 9 2022-03-30 20:44:25 PDT
Alex Christensen
Comment 10 2022-03-30 20:59:06 PDT
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
Comment 11 2022-03-31 03:22:27 PDT
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
Comment 12 2022-03-31 03:23:19 PDT
Note You need to log in before you can comment on or make changes to this bug.