[webkit-reviews] review denied: [Bug 229702] Add support for CSSUnparsedValue parsing through CSSStyleValue.parse() : [Attachment 437322] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied  review:
Bug 229702: Add support for CSSUnparsedValue parsing through
CSSStyleValue.parse()
https://bugs.webkit.org/show_bug.cgi?id=229702

Attachment 437322: Patch

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




--- Comment #17 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 437322
  --> https://bugs.webkit.org/attachment.cgi?id=437322
Patch

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

Great progress.  Let's get this polished up and landed.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:75
> +    auto parserContext = strictCSSParserContext();

This isn't used in this patch.	Please remove it from this patch and add it
when it is used.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:66
> +	       if (currentToken.value() == "var") {

Would it be equivalent to check .functionId() and see if it's CSSValueVar? 
That would probably be better than hard-coding a css keyword here.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:79
> +		       // Create a new vector of CSSUnparsedSegment
> +		       segmentStack.append({ });

I don't think this comment adds anything.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88
> +		       ASSERT_NOT_REACHED();

I feel like this assertion can be hit with invalid input.  What happens when it
is hit?  What is the behavior of Chrome?  Please add such a test if possible.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:96
> +	       ASSERT(segmentStack.size() > 0);

"> 0" is unnecessary

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:102
> +	       ASSERT(!identifiers.isEmpty());
> +	       if (identifiers.isEmpty())
> +		   return Exception { SyntaxError, "Cannot parse string as
CSSUnparsedValue, parentheses do not match."_s };

If identifiers can be empty when parentheses don't match, the assertion should
be removed and we should add a test that tries to parse a value with
mismatching parentheses and verify that a SyntaxError exception is thrown. 
ASSERT should be used for things that can't be hit no matter how invalid the
input, not for things that shouldn't happen with valid input.