Bug 212296 - Eliminate Color constructors that take strings, moving color parsing entirely into the CSS parser
Summary: Eliminate Color constructors that take strings, moving color parsing entirely...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-22 22:30 PDT by Darin Adler
Modified: 2020-05-26 13:10 PDT (History)
11 users (show)

See Also:


Attachments
Patch (38.99 KB, patch)
2020-05-22 22:30 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (43.62 KB, patch)
2020-05-25 14:10 PDT, Darin Adler
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-05-22 22:30:07 PDT
Eliminate Color constructors that take strings, moving color parsing entirely into the CSS parser
Comment 1 Darin Adler 2020-05-22 22:30:37 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-05-25 14:10:14 PDT
Created attachment 400221 [details]
Patch
Comment 3 Sam Weinig 2020-05-25 20:21:20 PDT
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.
Comment 4 Darin Adler 2020-05-26 10:58:49 PDT
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.
Comment 5 Darin Adler 2020-05-26 12:16:19 PDT
Committed r262155: <https://trac.webkit.org/changeset/262155>
Comment 6 Radar WebKit Bug Importer 2020-05-26 12:17:17 PDT
<rdar://problem/63635065>
Comment 7 Simon Fraser (smfr) 2020-05-26 12:56:52 PDT
Very nice! We should get cssText() out of Color too.
Comment 8 Darin Adler 2020-05-26 13:10:39 PDT
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.