Bug 239483

Created attachment 457859 [details]
Patch

Comment on attachment 457859 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-serialization/cssTransformValue.tentative-expected.txt:13
> +FAIL CSSPerspective with negative length serializes correctly assert_equals: expected "perspective(calc(-1px))" but got "perspective(-1px)"

Filed https://github.com/w3c/css-houdini-drafts/issues/1069 about this test.

Created attachment 457930 [details]
Patch

Comment on attachment 457930 [details]
Patch

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

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:64
> +    serialize(builder, false, false);

Can we use an OptionSet<> instead of two bools? Hard to know what false, false mean here.

> Source/WebCore/css/typedom/numeric/CSSMathInvert.cpp:79
> +    if (!withoutParentheses)

Double negative makes my brain hurt.

Created attachment 457941 [details]
Patch

Committed r293052 (249787@main): <https://commits.webkit.org/249787@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457941 [details].

<rdar://problem/91997078>

Comment on attachment 457930 [details]
Patch

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

> Source/WebCore/css/typedom/CSSStyleValue.h:106
> +    virtual void serialize(StringBuilder&, bool nested = false, bool withoutParentheses = false) const;

I think these bools would be better as an OptionSet (SerializationOptions?). We generally shy away from bool arguments these unless it is clear at call sites what is happening.

> Source/WebCore/css/typedom/CSSUnitValue.cpp:117
> +    builder.append(FormattedCSSNumber::create(m_value));
> +    builder.append(unitSerialization());

This can all be in one call:

builder.append(FormattedCSSNumber::create(m_value), unitSerialization());

This is also not a very efficient way to implement this as it is doing unnecessary temporary string allocation. It would be better to have StringTypeAdapters for these types. (For an example, see ColorSerialization.cpp)

Comment on attachment 457941 [details]
Patch

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

> Source/WebCore/css/typedom/CSSUnitValue.cpp:117
> +    builder.append(unitSerialization());

I agree this can be optimized to reduce String allocations, but I think that should be done by fixing the FIXME at the definition of CSSPrimitiveValue::unitTypeString and making it return a const char*

(In reply to Alex Christensen from comment #9)
> Comment on attachment 457941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457941&action=review
> 
> > Source/WebCore/css/typedom/CSSUnitValue.cpp:117
> > +    builder.append(unitSerialization());
> 
> I agree this can be optimized to reduce String allocations, but I think that
> should be done by fixing the FIXME at the definition of
> CSSPrimitiveValue::unitTypeString and making it return a const char*

Ok. Or an ASCIILiteral would be good too.

I would also think we should rename FormattedCSSNumber::create(). create() has the connotation in our source base that it will allocate something, and that should just be a normal constructor (or a free function called formattedCSSNumber() like pad().