Addition of CSSUnparsedValue. (TypedOM)
Created attachment 435474 [details] Patch
Created attachment 435664 [details] Patch
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.
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.
<rdar://problem/82162438>
Created attachment 436317 [details] Patch
Still in debugging phase. A lot of comments, commented-out code, and WTFLogs. Will be removed in the future.
Created attachment 436352 [details] Patch
Created attachment 436360 [details] Patch
Created attachment 436363 [details] Patch
Created attachment 436364 [details] Patch
Created attachment 436368 [details] Patch
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)
Created attachment 436410 [details] Patch
Created attachment 436731 [details] Patch
Created attachment 436732 [details] Patch
Created attachment 436733 [details] Patch
Created attachment 436734 [details] Patch
Created attachment 436750 [details] Patch
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
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.
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.
Created attachment 436812 [details] Patch
(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.
Created attachment 436820 [details] Patch
Created attachment 436821 [details] Patch
Created attachment 436822 [details] Patch
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].
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.
Reopening to attach new patch.
Created attachment 436870 [details] Patch
qiaosong_zhou@apple.com 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.
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].