Bug 212296

Created attachment 400221 [details] Patch

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

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

<rdar://problem/63635065>

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

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.