[webkit-reviews] review denied: [Bug 229069] Addition of CSSUnparsedValue. (TypedOM) : [Attachment 435664] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Qiaosong Zhou
<qiaosong_zhou at apple.com>'s request for review:
Bug 229069: Addition of CSSUnparsedValue. (TypedOM)
https://bugs.webkit.org/show_bug.cgi?id=229069

Attachment 435664: Patch

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




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 435664
  --> https://bugs.webkit.org/attachment.cgi?id=435664
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)->to
String());

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.