calc() on components inside relative color syntax colors currently does not work: https://codepen.io/leaverou/pen/VwpWNgL?editors=0100&fbclid=IwAR18C0aanphaAXkutH2oXzZp2bgsNHhWjwl0NqG8wz7D1rfKZHlq2iPg5xw (Test case by Lea Verou). To make this work, we will need to add support to our calc() implementation to allow injection of variables, which hopefully will be relatively straight forward, though we will see.
Created attachment 430128 [details] Patch
Created attachment 430139 [details] Patch
Created attachment 430141 [details] Patch
Created attachment 430142 [details] Patch
Created attachment 430143 [details] Patch
Comment on attachment 430143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430143&action=review Some of the code feels a little bit copy/pastey. Two similar functions. > Source/WebCore/css/makevalues.pl:212 > +template<> struct DefaultHash<WebCore::CSSValueID> : IntHash<unsigned> { }; > +template<> struct HashTraits<WebCore::CSSValueID> : GenericHashTraits<WebCore::CSSValueID> { Explicitly doing this here is a little annoying. We don’t have to do that when we use an integer type. Can we teach HashFunctions.h and HashTraits.h to default to hashing enums based on their underlying integer types? I think that using uint16_t::max would probably be just as good as lastCSSValueKeyword + 1. > Source/WebCore/css/calc/CSSCalcValue.cpp:316 > + return CSSCalcValue::create(function, tokens, destinationCategory, range, { }); Don’t need to write CSSCalcValue here. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:586 > + CalcParser angleCalcParser(range, CalculationCategory::Angle, valueRange); Should we scope this so the first parser is destroyed before the next one is allocated? Maybe not needed > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:588 > + if (const CSSCalcValue* calculation = angleCalcParser.value()) { > + if (calculation->category() == CalculationCategory::Angle) auto, also could do a single if: if (auto calculation = angleCalcParser.value(); calculation && calculation->category() == CalculationCategory::Angle) Seems like a good style. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:594 > + if (const CSSCalcValue* calculation = percentCalcParser.value()) { > + if (calculation->category() == CalculationCategory::Percent) Ditto. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:791 > + if (std::isinf(token.numericValue())) Use !isfinite instead of isinf? What handling do we want for NaN? > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:934 > + if (std::isinf(token.numericValue())) Same !isfinite thought. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1237 > + auto normalizedSaturation = clampTo(*saturation, 0.0, 100.0); > + auto normalizedLightness = clampTo(*lightness, 0.0, 100.0); > + auto normalizedAlpha = clampTo(*alpha, 0.0, 1.0); For all these calls to clampTo, I suggest using std::clamp instead. Our own special WebKit clampTo is only needed when we are mixing types or when we need defined behavior if max < min.
(In reply to Darin Adler from comment #6) > Comment on attachment 430143 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430143&action=review > > Some of the code feels a little bit copy/pastey. Two similar functions. > > > Source/WebCore/css/makevalues.pl:212 > > +template<> struct DefaultHash<WebCore::CSSValueID> : IntHash<unsigned> { }; > > +template<> struct HashTraits<WebCore::CSSValueID> : GenericHashTraits<WebCore::CSSValueID> { > > Explicitly doing this here is a little annoying. We don’t have to do that > when we use an integer type. > > Can we teach HashFunctions.h and HashTraits.h to default to hashing enums > based on their underlying integer types? I think that using uint16_t::max > would probably be just as good as lastCSSValueKeyword + 1. I was a little surprised it was needed. We have a struct called StrongEnumHashTraits that we can probably use that does std::numeric_limits<UnderlyingType>::max() for empty() and std::numeric_limits<UnderlyingType>::max() - 1, for deleted. I will try to just use that. > > > Source/WebCore/css/calc/CSSCalcValue.cpp:316 > > + return CSSCalcValue::create(function, tokens, destinationCategory, range, { }); > > Don’t need to write CSSCalcValue here. Fixed. > > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:586 > > + CalcParser angleCalcParser(range, CalculationCategory::Angle, valueRange); > > Should we scope this so the first parser is destroyed before the next one is > allocated? Maybe not needed. Not sure it gets us anything. That said, there are some optimizations we should consider here in the future. We could hoist the CSSCalcValue::isCalcFunction() check out of the CalcParser constructor, and given we don't ever want a CSSCalcValue, we could probably bypass its allocation entirely by having a static way to parse the calc() into the desired raw value. > > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:588 > > + if (const CSSCalcValue* calculation = angleCalcParser.value()) { > > + if (calculation->category() == CalculationCategory::Angle) > > auto, also could do a single if: > > if (auto calculation = angleCalcParser.value(); calculation && > calculation->category() == CalculationCategory::Angle) > > Seems like a good style. I added a consumeValueIfCategory(CalculationCategory) that simplifies things for a bunch of call sites. > > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:594 > > + if (const CSSCalcValue* calculation = percentCalcParser.value()) { > > + if (calculation->category() == CalculationCategory::Percent) > > Ditto. > > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:791 > > + if (std::isinf(token.numericValue())) > > Use !isfinite instead of isinf? What handling do we want for NaN? Not sure, but this is copied code, so I don't want to change it in this patch. Will work on unifying these checks and making sure we have tests. > > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:934 > > + if (std::isinf(token.numericValue())) > > Same !isfinite thought. > > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1237 > > + auto normalizedSaturation = clampTo(*saturation, 0.0, 100.0); > > + auto normalizedLightness = clampTo(*lightness, 0.0, 100.0); > > + auto normalizedAlpha = clampTo(*alpha, 0.0, 1.0); > > For all these calls to clampTo, I suggest using std::clamp instead. Our own > special WebKit clampTo is only needed when we are mixing types or when we > need defined behavior if max < min. Ok. Fixed. Thanks for review.
Meta question: Is it time to split up CSSPropertyParserHelpers.h/cpp into some meaningful structure?
Created attachment 430152 [details] Patch
Committed r278261 (238298@main): <https://commits.webkit.org/238298@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430152 [details].
<rdar://problem/78669976>
Re-opened since this is blocked by bug 226477
Bizarre, I'm guessing "enum CSSValueID : uint16_t" is the issue, but why?
(In reply to Sam Weinig from comment #13) > Bizarre, I'm guessing "enum CSSValueID : uint16_t" is the issue, but why? And why only "win" and not "wincairo"? Well, that was just an optimization to avoid a header include, so I can just leave it for now.
Created attachment 430222 [details] Patch
Committed r278304 (238341@main): <https://commits.webkit.org/238341@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430222 [details].