Add modern HSL syntax fast path to CSSParserFastPaths#62026
Add modern HSL syntax fast path to CSSParserFastPaths#62026webkit-commit-queue merged 1 commit intoWebKit:mainfrom
Conversation
|
EWS run on previous version of this PR (hash 6f4b85c) Details |
|
I can’t review in detail right away, and I have no problem with adding this, but I have been curious for a while if we have any data on how common the fast parser gets hit in the wild and/or perf tests. My recollection is that it only gets applied for JS element.style[prop] = “foo”. Is that still the case? |
I can't say I'm very familiar with our CSS parser so I asked Claude and it appears you are mostly correct!
I did not realize that when I wrote the PR but I did think it was odd to support the legacy syntax and not the modern one in the fast path. I figured more and more developers are likely switching to the new syntax 🤷🏻 I'll let CSS experts decide if it's worth it. |
darinadler
left a comment
There was a problem hiding this comment.
I’m kind of surprised that the syntax for modern HSL is the same inside hsl() and hsla().
|
|
||
| skip(characters, hueEnd); | ||
|
|
||
| auto parsePercentage = [&](std::span<const CharacterType>& characters) -> std::optional<double> { |
There was a problem hiding this comment.
If we’re going to capture something here, why not capture characters too?
| auto resultColor = convertToColor<HSLFunctionModern, CSSColorFunctionForm::Absolute>(typedColor, 0); | ||
| return resultColor.tryGetAsSRGBABytes(); |
There was a problem hiding this comment.
| auto resultColor = convertToColor<HSLFunctionModern, CSSColorFunctionForm::Absolute>(typedColor, 0); | |
| return resultColor.tryGetAsSRGBABytes(); | |
| return convertToColor<HSLFunctionModern, CSSColorFunctionForm::Absolute>(typedColor, 0).tryGetAsSRGBABytes(); |
| if (mightBeHSLA(characters)) { | ||
| auto innerCharacters = characters.subspan(5); | ||
| if (auto color = parseLegacyHSL(innerCharacters)) | ||
| return color; | ||
| return parseModernHSL(innerCharacters); | ||
| } |
There was a problem hiding this comment.
Why not add a parseHSL that does both legacy and modern?
| if (mightBeHSLA(characters)) { | |
| auto innerCharacters = characters.subspan(5); | |
| if (auto color = parseLegacyHSL(innerCharacters)) | |
| return color; | |
| return parseModernHSL(innerCharacters); | |
| } | |
| if (mightBeHSLA(characters)) | |
| return parseHSL(characters.subspan(5)); |
| if (mightBeHSL(characters)) { | ||
| auto innerCharacters = characters.subspan(4); | ||
| if (auto color = parseLegacyHSL(innerCharacters)) | ||
| return color; | ||
| return parseModernHSL(innerCharacters); | ||
| } |
There was a problem hiding this comment.
Why not add a parseHSL that does both legacy and modern?
| if (mightBeHSL(characters)) { | |
| auto innerCharacters = characters.subspan(4); | |
| if (auto color = parseLegacyHSL(innerCharacters)) | |
| return color; | |
| return parseModernHSL(innerCharacters); | |
| } | |
| if (mightBeHSL(characters)) | |
| return parseHSL(characters.subspan(4)); |
6f4b85c to
9ad8e88
Compare
|
EWS run on current version of this PR (hash 9ad8e88) Details
|
https://bugs.webkit.org/show_bug.cgi?id=311468 Reviewed by Darin Adler. The existing HSL fast path only handled the legacy comma-separated syntax (e.g., hsl(120, 100%, 50%)). Modern CSS uses space-separated syntax (e.g., hsl(120 100% 50%) or hsl(120 100% 50% / 0.5)), which was falling through to the full tokenizer and parser. Rename parseLegacyHSL() to parseHSL() and teach it to handle the modern space-separated syntax with optional slash-separated alpha, including support for angle units (deg, rad) and percentage alpha values. The legacy path is tried first since its comma check is effectively free when it fails. Also add API tests for both legacy and modern HSL parsing through parseSimpleColor(), covering primary colors, black/white, alpha variants, angle units, hue wrapping, and modern-vs-legacy equivalence, along with negative tests for malformed modern HSL inputs. Test: Tools/TestWebKitAPI/Tests/WebCore/CSSParserFastPaths.cpp * Source/WebCore/css/parser/CSSParserFastPaths.cpp: (WebCore::parseHSL): (WebCore::parseNumericColor): (WebCore::parseColor): (WebCore::CSSParserFastPaths::parseNamedColor): (WebCore::isUniversalKeyword): (WebCore::parseLegacyHSL): Deleted. * Tools/TestWebKitAPI/Tests/WebCore/CSSParserFastPaths.cpp: (TestWebKitAPI::TEST(CSSParserFastPaths, ParseLegacyHsl)): (TestWebKitAPI::TEST(CSSParserFastPaths, ParseModernHsl)): (TestWebKitAPI::TEST(CSSParserFastPaths, ParseModernHslInvalid)): Canonical link: https://commits.webkit.org/310603@main
9ad8e88 to
524b854
Compare
|
Committed 310603@main (524b854): https://commits.webkit.org/310603@main Reviewed commits have been landed. Closing PR #62026 and removing active labels. |
524b854
9ad8e88