Bug 235269 - Canvas functions that take colors as strings don't support all the syntax that CSS supports
Summary: Canvas functions that take colors as strings don't support all the syntax tha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-15 12:17 PST by Sam Weinig
Modified: 2022-01-18 12:02 PST (History)
17 users (show)

See Also:


Attachments
Patch (7.80 KB, patch)
2022-01-15 12:30 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (7.29 KB, patch)
2022-01-16 11:18 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (16.44 KB, patch)
2022-01-17 18:57 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2022-01-15 12:17:39 PST
Canvas functions that take colors as strings don't support all the syntax that CSS supports
Comment 1 Sam Weinig 2022-01-15 12:30:11 PST
Created attachment 449264 [details]
Patch
Comment 2 Darin Adler 2022-01-15 14:20:08 PST
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.
Comment 3 Sam Weinig 2022-01-15 19:59:37 PST
(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).
Comment 4 Sam Weinig 2022-01-16 11:18:08 PST
Created attachment 449284 [details]
Patch
Comment 5 Darin Adler 2022-01-16 11:44:25 PST
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?
Comment 6 Sam Weinig 2022-01-17 09:49:14 PST
(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.
Comment 7 Sam Weinig 2022-01-17 18:57:00 PST
Created attachment 449361 [details]
Patch
Comment 8 EWS 2022-01-18 12:01:03 PST
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].
Comment 9 Radar WebKit Bug Importer 2022-01-18 12:02:17 PST
<rdar://problem/87727545>