Bug 212296 – Eliminate Color constructors that take strings, moving color parsing entirely into the CSS parser

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Darin Adler]

Eliminate Color constructors that take strings, moving color parsing entirely into the CSS parser

[Darin Adler]

Created attachment 400110 [details]
Patch

[Darin Adler]

Created attachment 400221 [details]
Patch

[Sam Weinig]

Comment on attachment 400221 [details]
Patch

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

> Source/WebCore/css/parser/CSSParser.h:89
> +    WEBCORE_EXPORT static Color parseColor(const String&, bool strict = false);
> +    static Color parseColorWorkerSafe(const String&);

Should these be converted to StringViews as well at some point?

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:469
>          int red;

Might make sense to make this (and the ones below) uint8_t, and have an overload of makeSimpleColor that doesn't clamp?

> Source/WebCore/platform/graphics/SimpleColor.cpp:-138
> -// originally moved here from the CSS parser

I think you can remove HexNumber.h include from this file.

> Source/WebCore/platform/graphics/SimpleColor.h:-80
> -    static Optional<SimpleColor> parseHexColor(const String&);
> -    static Optional<SimpleColor> parseHexColor(const StringView&);
> -    static Optional<SimpleColor> parseHexColor(const LChar*, unsigned);
> -    static Optional<SimpleColor> parseHexColor(const UChar*, unsigned);

I think you can remove the uchar.h and LChar.h #includes here now.

[Darin Adler]

Comment on attachment 400221 [details]
Patch

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

>> Source/WebCore/css/parser/CSSParser.h:89
>> +    static Color parseColorWorkerSafe(const String&);
> 
> Should these be converted to StringViews as well at some point?

I think maybe. I believe I tried and didn’t for some trivial reason that could later be fixed.

>> Source/WebCore/css/parser/CSSParserFastPaths.cpp:469
>>          int red;
> 
> Might make sense to make this (and the ones below) uint8_t, and have an overload of makeSimpleColor that doesn't clamp?

Yes, I would like to do that. Also, there has to be a better way to refactor these repeated calls to parseColorIntOrPercentage so it’s clearer, but not urgent for now.

>> Source/WebCore/platform/graphics/SimpleColor.cpp:-138
>> -// originally moved here from the CSS parser
> 
> I think you can remove HexNumber.h include from this file.

Will do.

>> Source/WebCore/platform/graphics/SimpleColor.h:-80
>> -    static Optional<SimpleColor> parseHexColor(const UChar*, unsigned);
> 
> I think you can remove the uchar.h and LChar.h #includes here now.

Will do.

[Darin Adler]

Committed r262155: <https://trac.webkit.org/changeset/262155>

[Radar WebKit Bug Importer]

<rdar://problem/63635065>

[Simon Fraser (smfr)]

Very nice! We should get cssText() out of Color too.

[Darin Adler]

Comment on attachment 400221 [details]
Patch

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

>>> Source/WebCore/css/parser/CSSParser.h:89
>>> +    static Color parseColorWorkerSafe(const String&);
>> 
>> Should these be converted to StringViews as well at some point?
> 
> I think maybe. I believe I tried and didn’t for some trivial reason that could later be fixed.

I did parseColorWorkerSafe. But probably can’t do parseColor until we also change parseSingleValue, which uses CSSTokenizer, so that’s a bigger project.

>>> Source/WebCore/platform/graphics/SimpleColor.cpp:-138
>>> -// originally moved here from the CSS parser
>> 
>> I think you can remove HexNumber.h include from this file.
> 
> Will do.

I think I can’t. The hex() function is used in SimpleColor::serializationForRenderTreeAsText.