[webkit-reviews] review granted: [Bug 235269] Canvas functions that take colors as strings don't support all the syntax that CSS supports : [Attachment 449264] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 235269: Canvas functions that take colors as strings don't support all the
syntax that CSS supports
https://bugs.webkit.org/show_bug.cgi?id=235269

Attachment 449264: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 449264
  --> https://bugs.webkit.org/attachment.cgi?id=449264
Patch

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

r=me assuming you resolve the failing tests

> Source/WebCore/css/parser/CSSParser.cpp:97
> +Color CSSParser::parseColor(const String& string, const CSSParserContext&
context)

Not new, but occurs to me I should ask: Is it valuable to have this take const
String& rather than StringView? Could be helpful if:

1) it has to call functions that take String or AtomicString
2) it needs to store a copy of the string
3) it needs to compute the hash of the string

But logically it would be better to take StringView.

> Source/WebCore/html/canvas/CanvasStyle.cpp:65
> +    Color color;
> +    if (is<HTMLCanvasElement>(canvasBase))
> +	   color = CSSParser::parseColor(colorString, CSSParserContext {
downcast<HTMLCanvasElement>(canvasBase).document() });
> +    else
> +	   color = CSSParser::parseColor(colorString);

This makes me think parseColor should take a std::optional<CSSParserContext>.
That would let us encapsulate the rule that helps us find the appropriate
context in a function instead of writing out an if here interspersed with the
parseColor function call.