[webkit-reviews] review denied: [Bug 229702] Add support for CSSUnparsedValue parsing through CSSStyleValue.parse() : [Attachment 436858] 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 436858: Patch

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




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

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

Looks like some test expectations need updating.  Looks like a progression.

> Source/WebCore/css/CSSVariableReferenceValue.h:64
> +    // FIXME: is friend the best solution?
> +    friend class CSSStyleValue;

It isn't.  A better solution would probably be to expose public functions to do
what CSSStyleValue needs to do.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:75
> +	   m_fallback->serialize(builder);

nice!

> Source/WebCore/css/typedom/CSSStyleImageValue.h:57
> +    CSSStyleImageValue(Ref<CSSImageValue>&&, Document* = nullptr);

It looks like the default value is unneeded because only one call site has
nullptr.
Also, it looks like the Document is used by the
CanvasRenderingContext2DBase::drawImage which takes a CSSStyleImageValue&, and
having a null Document would make this unable to be drawn.  I'm not sure what
the consequences of that would be, and I'm not sure how finished
CanvasRenderingContext2DBase::drawImage is.  It might be worth a comment saying
that the CSSStyleImageValue created with no document currently can't be used
for drawing.

> Source/WebCore/css/typedom/CSSStyleValue.cpp:51
> +ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValue::parseStyleValue(const
String& cssProperty, const String& cssText, bool)

Ah, I see the comment below.  parseMultiple will be brought back.

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

nit: unnecessary space addition.

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

We can't return nullptr here.  An exception?

> Source/WebCore/css/typedom/CSSStyleValue.cpp:119
> +CSSParserContext CSSStyleValue::getVariableReferenceParserContext()
> +{
> +    auto parserContext = CSSParserContext(HTMLStandardMode);

Since this is static, do we need to make a new CSSParserContext every time it
is called?  Could we use NeverDestroyed and return a const CSSParserContext&? 
It doesn't look like the CSSParserContext stores state that we would need to
reset or anything like that.