Bug 229069 – Addition of CSSUnparsedValue. (TypedOM)

• Component: New Bugs
• Status: RESOLVED FIXED

[Qiaosong Zhou]

Addition of CSSUnparsedValue. (TypedOM)

[Qiaosong Zhou]

Created attachment 435474 [details]
Patch

[Qiaosong Zhou]

Created attachment 435664 [details]
Patch

[Alex Christensen]

Comment on attachment 435664 [details]
Patch

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

This looks mostly reasonable.  Add some test expectation changes to this patch and polish a few things first.

> Source/WebCore/bindings/js/JSCSSStyleValueCustom.cpp:41
> +    if (value->getType() == CSSStyleValueType::CSSUnitValue)

using a switch statement instead would make it so getType is only called once.

> Source/WebCore/css/typedom/CSSNumericValue.h:37
> +    CSSStyleValueType getType() const { return CSSStyleValueType::CSSNumericValue; }

Should this be marked as "override" or "final", or should we remove it?

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:42
> +ExceptionOr<Ref<CSSOMVariableReferenceValue>> CSSOMVariableReferenceValue::create(String variable, RefPtr<CSSUnparsedValue>&& fallback)

String&& or const String&

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:50
> +ExceptionOr<void> CSSOMVariableReferenceValue::setVariable(String variable)

ditto

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:47
> +    String variable() const { return m_variable; }

const String&

> Source/WebCore/css/typedom/CSSStyleValue.cpp:46
> +ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValue::parseStyleValue(String property, String cssText, bool parseMultiple)

More String&& or const String& in this file.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:65
> +    auto parseResult = CSSParser::parseValue(styleDeclaration, propertyID, cssText, true, HTMLStandardMode);

Could you give "true" a name so we know what it means just by looking at the code?

> Source/WebCore/css/typedom/CSSStyleValue.cpp:75
> +    auto cssValue = styleDeclaration->getPropertyCSSValue(propertyID);
> +    auto valueType = cssValue->cssValueType();
> +    
> +    if (!cssValue)
> +        return Exception { SyntaxError };

It looks like some of the tests crash because of this.  You need to move the null check to before you use cssValue.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:106
> +    auto res = WTFMove(parseResult.releaseReturnValue().at(0));

This needs a bounds check in case an empty Vector is returned.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:62
> +        if (WTF::holds_alternative<String>(segment))
> +            serialization.append(WTF::get<String>(segment));
> +        else
> +            serialization.append(WTF::get<RefPtr<CSSOMVariableReferenceValue>>(segment)->toString());

This should probably use WTF::visit instead of this in case someone adds another type to the variant.  Currently we just assume it's either a String or a RefPtr.  If someone added another type, it would crash trying to get a RefPtr, but if you use WTF::visit it would make a compile failure.

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:51
> +    CSSValue* valuePtr = value.get();

Don't store this value in a local variable.  Just use value.get() at all the places you use this.

> Source/WebCore/dom/StyledElement.cpp:112
> +            return StylePropertyMapReadOnly::customPropertyValueOrDefault(name, element.document(), value.copyRef(), &element);

This can be WTFMove(value) instead of value.copyRef() to reduce ref churn because you don't use value after this call.  Same 5 more times below this.

[Chris Dumez]

Comment on attachment 435664 [details]
Patch

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

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:44
> +    if (!variable.startsWith("--"_s))

There is a startsWith() overload that takes in a `const char*` so no need to construct a WTFString here by using _s suffix. This is actually less efficient.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:45
> +        return Exception { TypeError };

Please provide an exception message as second parameter.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:52
> +    if (!variable.startsWith("--"_s))

ditto.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:53
> +        return Exception { TypeError };

ditto.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:61
> +    return makeString("var("_s, m_variable, (m_fallback ? ", "_s : ""_s), (m_fallback ? m_fallback->toString() : ""_s), ")"_s);

Drop all the _s. makeString() works fine with const char*.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:28
> +#if ENABLE(CSS_TYPED_OM)

Do we really need a build flag for this? Wouldn't a runtime flag be sufficient?

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:42
> +    static ExceptionOr<Ref<CSSOMVariableReferenceValue>> create(String, RefPtr<CSSUnparsedValue>&& fallback = { });

Also, please pass the String parameter as a `String&&` or a `const String&`.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:48
> +    RefPtr<CSSUnparsedValue> fallback() const { return m_fallback.copyRef(); }

This does ref-counting churn, you could return a CSSUnparsedValue* and do `return m_fallback.get()`. Lets the call site decide if they want to ref it or not.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:51
> +    explicit CSSOMVariableReferenceValue(String variable, RefPtr<CSSUnparsedValue>&& fallback)

explicit is not needed given that you have 2 non-optional parameters.

Also, please pass the String parameter as a `String&&` or a `const String&`.

Also, this constructor could be moved to the cpp file.

> Source/WebCore/css/typedom/CSSStyleImageValue.h:50
> +    CSSStyleValueType getType() const final { return CSSStyleValueType::CSSStyleImageValue; }

Should probably be private as it doesn't make sense to call getType on a CSSStyleImageValue.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:53
> +        property.convertToASCIILowercase();

This does nothing. convertToASCIILowercase() is const and returns a value that you ignore.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:67
> +        return Exception { SyntaxError };

Please provide an exception message as second parameter.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:69
> +

Extra blank line here.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:70
> +    // RefPtr<CSSValue>

Please drop this comment.

>> Source/WebCore/css/typedom/CSSStyleValue.cpp:75
>> +        return Exception { SyntaxError };
> 
> It looks like some of the tests crash because of this.  You need to move the null check to before you use cssValue.

Please provide an exception message as second parameter.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:79
> +    if (valueType == CSSValue::CSS_VALUE_LIST) {

Why not use if (is<CSSValueList>(cssValue.get())) ?

> Source/WebCore/css/typedom/CSSStyleValue.cpp:80
> +        ASSERT(is<CSSValueList>(cssValue.get()));

Then we wouldn't need this.

>> Source/WebCore/css/typedom/CSSStyleValue.cpp:106
>> +    auto res = WTFMove(parseResult.releaseReturnValue().at(0));
> 
> This needs a bounds check in case an empty Vector is returned.

Also, unnecessary local variable here.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:136
> +    // FIXME: Add Reification control flow. Returns nullptr if failed

Please end comments with a period.

> Source/WebCore/css/typedom/CSSStyleValue.h:73
> +    CSSPropertyID m_cssPropertyID;

Please add a blank line before the data members.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:70
> +        return Exception { RangeError };

Please provide an exception message as second parameter.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:71
> +    return ExceptionOr<CSSUnparsedSegment> { CSSUnparsedSegment { m_segments[index] } };

ExceptionOr<CSSUnparsedSegment> { } should not be needed.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:77
> +        return Exception { RangeError };

ditto.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:79
> +    return ExceptionOr<CSSUnparsedSegment> { CSSUnparsedSegment { val } };

ditto.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:41
> +class CSSUnparsedValue : public CSSStyleValue {

Should this class be marked as final?

> Source/WebCore/css/typedom/CSSUnparsedValue.h:49
> +    CSSStyleValueType getType() const final { return CSSStyleValueType::CSSUnparsedValue; }

Should probably be private.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:51
> +    unsigned long length() const { return m_segments.size(); }

unsigned long is not the right return type here. We should return a size_t, live Vector::size().

> Source/WebCore/css/typedom/CSSUnparsedValue.h:53
> +    ExceptionOr<CSSUnparsedSegment> item(unsigned long);

I would use size_t here to match.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:55
> +    ExceptionOr<CSSUnparsedSegment> setItem(unsigned long, CSSUnparsedSegment);

ditto.

[Radar WebKit Bug Importer]

<rdar://problem/82162438>

[Qiaosong Zhou]

Created attachment 436317 [details]
Patch

[Qiaosong Zhou]

Still in debugging phase. A lot of comments, commented-out code, and WTFLogs. Will be removed in the future.

[Qiaosong Zhou]

Created attachment 436352 [details]
Patch

[Qiaosong Zhou]

Created attachment 436360 [details]
Patch

[Qiaosong Zhou]

Created attachment 436363 [details]
Patch

[Qiaosong Zhou]

Created attachment 436364 [details]
Patch

[Qiaosong Zhou]

Created attachment 436368 [details]
Patch

[Alex Christensen]

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

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:61
> +    return makeString("var(", m_variable, (m_fallback ? ", " : emptyString()), (m_fallback ? m_fallback->toString() : emptyString()), ")");

I think it would actually be a little more efficient if the first ternary expression were (m_fallback ? ", "_s : ""_s) so that the result of the expression is an ASCIILiteral instead of a String, possibly allocated and copied from a const char*

> Source/WebCore/css/typedom/CSSStyleValue.cpp:52
> +    if (isCustomPropertyName(property))

I'm guessing this might mean to be if (!isCustomPropertyName(cssProperty)
Otherwise, you're always passing in a null string.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:99
> +    auto parseResult = parseStyleValue(property, cssText, false);

Could you give "false" a name?
constexpr auto <insert name here> = false;
auto parseResult = parseStyleValue(property, cssText, <insert name here>);
That makes the code easier to read.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:106
> +    // Returned vector should not be empty. If parsing failed, an exception should be returned.
> +    ASSERT(!returnValue.isEmpty());

Can parsing succeed and return an empty vector?  If not, this should probably be a RELEASE_ASSERT.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:113
> +    return parseStyleValue(property, cssText, true);

Ditto with named boolean.

> Source/WebCore/css/typedom/CSSStyleValue.h:40
> +enum class CSSStyleValueType: uint8_t {

nit: space before the colon.

> Source/WebCore/css/typedom/CSSStyleValue.h:67
> +    // FIXME: probably should remove this in the future

More specific is better.
// FIXME: Remove this when CSSStyleValue::reifyValue is implemented.

> Source/WebCore/css/typedom/CSSStyleValue.h:71
>      CSSStyleValue() = default;

This constructor should also have a FIXME to remove it.

> Source/WebCore/css/typedom/CSSStyleValue.h:74
> +    CSSPropertyID m_cssPropertyID;

As the code stands now, this is uninitialized memory when the default constructor is called.  Probably not too big a deal because we're about to remove the default constructor, but still not great.
Would it be too disruptive to just return nullptr from CSSStyleValue::reifyValue and remove this class's default constructor instead of adding an empty CSSStyleValue::create()?

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:57
> +    String serialization = emptyString();

This should use a StringBuilder to avoid an O(n^2) algorithm with copying each substring as it's assembled.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:62
> +        }, [&] (const RefPtr<CSSOMVariableReferenceValue>& value) {
> +            serialization.append(value->toString());

Can value be null here?  If so, we should add a null check.  If not, can we change the Variant type to be a Ref instead of a RefPtr?

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:46
> +RefPtr<CSSStyleValue> StylePropertyMapReadOnly::reifyValue(RefPtr<CSSValue>&& value, const String&, Document& document, Element*)

Can we remove the unused Element*?

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:50
> +    

nit: remove extra whitespace.

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:45
> +    

nit: whitespace

> Source/WebCore/dom/StyledElement.cpp:112
> +            return StylePropertyMapReadOnly::customPropertyValueOrDefault(name, element.document(), value.copyRef(), &element);

WTFMove(value) to reduce ref churn since we won't use value after this line.

> Source/WebCore/html/CustomPaintImage.cpp:69
> +        return StylePropertyMapReadOnly::customPropertyValueOrDefault(name, element.document(), value.copyRef(), &element);

WTFMove(value)

> Source/WebCore/html/CustomPaintImage.cpp:77
> +    return StylePropertyMapReadOnly::reifyValue(value.copyRef(), name, element.document(), &element);

WTFMove(value)

> Source/WebCore/html/CustomPaintImage.cpp:93
> +            return StylePropertyMapReadOnly::customPropertyValueOrDefault(name, element.document(), value.copyRef(), &element);

WTFMove(value)

> Source/WebCore/html/CustomPaintImage.cpp:101
> +        return StylePropertyMapReadOnly::reifyValue(value.copyRef(), name, element.document(), &element);

WTFMove(value)

[Qiaosong Zhou]

Created attachment 436410 [details]
Patch

[Qiaosong Zhou]

Created attachment 436731 [details]
Patch

[Qiaosong Zhou]

Created attachment 436732 [details]
Patch

[Qiaosong Zhou]

Created attachment 436733 [details]
Patch

[Qiaosong Zhou]

Created attachment 436734 [details]
Patch

[Qiaosong Zhou]

Created attachment 436750 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Alex Christensen]

Comment on attachment 436750 [details]
Patch

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

Great!  I think once these final few comments are addressed it'll be ready to land.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:48
> +    CSSUnparsedValue* fallback() const { return m_fallback.get(); }

This should probably be one of these instead:
const CSSUnparsedValue* fallback() const;
CSSUnparsedValue* fallback();

> Source/WebCore/css/typedom/CSSStyleValue.cpp:67
> +    bool important = true;

constexpr

> Source/WebCore/css/typedom/CSSStyleValue.cpp:84
> +            auto reifiedValue = CSSStyleValue::reifyValue(propertyID, currentValue.copyRef());
> +            if (reifiedValue)

Nit: these can go on the same line:
if (auto reifiedValue = CSSStyleValue::reifyValue(propertyID, currentValue.copyRef()))

> Source/WebCore/css/typedom/CSSStyleValue.cpp:90
> +    } else {
> +        auto reifiedValue = CSSStyleValue::reifyValue(propertyID, cssValue.copyRef());
> +        if (reifiedValue)

else if (auto reifiedValue = CSSStyleValue::reifyValue(propertyID, cssValue.copyRef()))

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:51
> +CSSUnparsedValue::CSSUnparsedValue(const Vector<CSSUnparsedSegment>& segments)

Can this be a Vector<CSSUnparsedSegment>&& instead?  That would prevent a Vector copy if it were.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:52
> +    : CSSStyleValue(nullptr)

This line is probably unnecessary since CSSStyleValue has a default constructor that does exactly this.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:65
> +void CSSUnparsedValue::serialize(StringBuilder& builder) const

I don't think this needs to be a separate function unless you're planning to use it elsewhere in the future.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:32
> +#include <wtf/Variant.h>

This is probably not needed

> Source/WebCore/css/typedom/CSSUnparsedValue.h:33
> +#include <wtf/text/StringBuilder.h>

This is not needed.  <wtf/Forward.h> should have enough.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:52
> +    ExceptionOr<CSSUnparsedSegment> setItem(size_t, CSSUnparsedSegment);

Can this be a CSSUnparsedSegment&& or a const CSSUnparsedSegment&?  As it stands now, you're probably unnecessarily instantiating a whole object when calling this function.

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:58
>      // FIXME: should use raw CSSStyleValue

I think this comment can be removed since we're using CSSStyleValue now.

[Chris Dumez]

Comment on attachment 436750 [details]
Patch

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

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:39
> +class CSSOMVariableReferenceValue: public RefCounted<CSSOMVariableReferenceValue> {

missing space before ':'

> Source/WebCore/css/typedom/CSSStyleValue.cpp:99
> +    constexpr auto parseMultiple = false;

bool is just as long as auto.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:107
> +    if (!returnValue.size())

returnValue.isEmpty()

> Source/WebCore/css/typedom/CSSStyleValue.cpp:115
> +    constexpr auto parseMultiple = true;

bool is just as long as auto.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:83
> +ExceptionOr<CSSUnparsedSegment> CSSUnparsedValue::setItem(size_t index, CSSUnparsedSegment val)

CSSUnparsedSegment should not be passed by value. `const CSSUnparsedSegment&` or `CSSUnparsedSegment&&`. Probably the latter.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88
> +    return CSSUnparsedSegment { val };

doesn't `return val;` work? or `return m_segments[index];` if you make the change I suggested above.

[Qiaosong Zhou]

Created attachment 436812 [details]
Patch

[Qiaosong Zhou]

(In reply to Chris Dumez from comment #22)

> > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88
> > +    return CSSUnparsedSegment { val };
> 
> doesn't `return val;` work? or `return m_segments[index];` if you make the
> change I suggested above.

It doesn't work. The compiler throws a type error. It is not able to infer the ExceptionOr type for some reason.

[Qiaosong Zhou]

Created attachment 436820 [details]
Patch

[Qiaosong Zhou]

Created attachment 436821 [details]
Patch

[Qiaosong Zhou]

Created attachment 436822 [details]
Patch

[EWS]

Committed r281786 (241123@main): <https://commits.webkit.org/241123@main>

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

[Darin Adler]

Comment on attachment 436822 [details]
Patch

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

> Source/WebCore/css/typedom/CSSStyleValue.h:58
> +    virtual CSSStyleValueType getType() const { return CSSStyleValueType::CSSStyleValue; }

Is this name web-exposed? If not, then it should *not* have "get" in its name to match WebKit coding style. If it is web-exposed then there may be some other style guide that overrides the WebKt one.

[Qiaosong Zhou]

Reopening to attach new patch.

[Qiaosong Zhou]

Created attachment 436870 [details]
Patch

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 436870 [details] from commit queue.

[EWS]

Committed r281809 (241145@main): <https://commits.webkit.org/241145@main>

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