[webkit-reviews] review granted: [Bug 226272] Support calc() on components inside relative color syntax colors : [Attachment 430143] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 226272: Support calc() on components inside relative color syntax colors
https://bugs.webkit.org/show_bug.cgi?id=226272

Attachment 430143: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 430143
  --> https://bugs.webkit.org/attachment.cgi?id=430143
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.