Bug 239483 – Implement most of CSS typed OM object stringifiers

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

[Alex Christensen]

Implement most of CSS typed OM object stringifiers

[Alex Christensen]

Created attachment 457859 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 457930 [details]
Patch

[Simon Fraser (smfr)]

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.

[Alex Christensen]

Created attachment 457941 [details]
Patch

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/91997078>

[Sam Weinig]

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)

[Alex Christensen]

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*

[Sam Weinig]

(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().