[webkit-reviews] review granted: [Bug 212296] Eliminate Color constructors that take strings, moving color parsing entirely into the CSS parser : [Attachment 400221] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 212296: Eliminate Color constructors that take strings, moving color
parsing entirely into the CSS parser
https://bugs.webkit.org/show_bug.cgi?id=212296

Attachment 400221: Patch

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




--- Comment #3 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 400221
  --> https://bugs.webkit.org/attachment.cgi?id=400221
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.