Bug 235269 – Canvas functions that take colors as strings don't support all the syntax that CSS supports

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Canvas functions that take colors as strings don't support all the syntax that CSS supports

[Sam Weinig]

Created attachment 449264 [details]
Patch

[Darin Adler]

Comment on attachment 449264 [details]
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.

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 449264 [details]
> 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.

I had the same thought, in this case it is #1. parseSingleValue() takes a const String&, and it takes one because the CSSTokenizer constructor takes one. I would hope we could figure out a way to remove the requirement that the CSSTokenizer take one (it does some string mutation shenanigans on a copy) but I didn't really go any farther than this.
 

> 
> > 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.

I don't think we should ever call the one without a context, and was considering renaming it to something like deprecatedParseColor() to make it clear (and to help find the call sites).

[Sam Weinig]

Created attachment 449284 [details]
Patch

[Darin Adler]

Comment on attachment 449284 [details]
Patch

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

> Source/WebCore/css/parser/CSSParser.h:92
> +    // FIXME: All callers are not getting the right Settings for parsing due to lack of CSSParserContext and should switch to the function above.

Does it make the patch too big to rename the function below deprecatedParseColor or parseColorWithoutContext at the same time as we introduce the new function?

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> Comment on attachment 449284 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449284&action=review
> 
> > Source/WebCore/css/parser/CSSParser.h:92
> > +    // FIXME: All callers are not getting the right Settings for parsing due to lack of CSSParserContext and should switch to the function above.
> 
> Does it make the patch too big to rename the function below
> deprecatedParseColor or parseColorWithoutContext at the same time as we
> introduce the new function?

Nah, not too big. That seems like a good idea.

[Sam Weinig]

Created attachment 449361 [details]
Patch

[EWS]

Committed r288134 (246141@main): <https://commits.webkit.org/246141@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449361 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87727545>