[webkit-reviews] review granted: [Bug 202794] Make CSS font shorthands parsable within a worker (i.e. without CSSValuePool) : [Attachment 410511] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 202794: Make CSS font shorthands parsable within a worker (i.e. without
CSSValuePool)
https://bugs.webkit.org/show_bug.cgi?id=202794

Attachment 410511: Patch

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




--- Comment #16 from Darin Adler <darin at apple.com> ---
Comment on attachment 410511
  --> https://bugs.webkit.org/attachment.cgi?id=410511
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410511&action=review

So much new code here. Wisher there was a way to save a bit more and make the
code a bit less repetitive.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:140
> +	   if (category != CalculationCategory::Percent) {
> +	       if (!allowPercentOf || (category !=
CalculationCategory::PercentLength
> +		   && category != CalculationCategory::PercentNumber))
> +		   return WTF::nullopt;
> +	   }

This logic is confusing enough that I think it might require a helper function.
I also find the bool argument here both subtle and confusing.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:217
> +	   if (const CSSCalcValue* calcValue = calcParser.value()) {

auto

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:423
> +		   if (auto result = calcParser.consumePercentRaw(true))

This mysterious "true" is something we try to avoid in WebKit code.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1970
> +    String familyName = concatenateFamilyName(range);
> +    if (familyName.isNull())
> +	   return WTF::nullopt;
> +    return familyName;

This shows that Optional<String> is a strange type. Since String already has a
null value built in, it’s overkill to use Optional with it, despite the fact
that it seems logical to do that. Another possibilities to change
concatenateFamilyName to return Optional<String>.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:2090
> +AtomString genericFontFamilyFromValueID(CSSValueID ident)

This could return const AtomString&.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:2108
> +	   return { };

This would return nullAtom.

> Source/WebCore/style/StyleBuilderCustom.h:995
>		   isGenericFamily = true;

The old code used to say "default break". The new code instead sets
isGenericFamily to true and calls
CSSPropertyParserHelpers::genericFontFamilyFromValueID. Is that going to do the
right thing?

> Source/WebCore/style/StyleResolveForFontRaw.cpp:199
> +	       return
static_cast<float>(CSSPrimitiveValue::computeNonCalcLengthDouble(conversionData
, length.type, length.value));

Here we are casting to a float.

> Source/WebCore/style/StyleResolveForFontRaw.cpp:201
> +	       return (parentSize * percentage) / 100.f;

Here we are writing an expression that will return a double without casting to
a float. Unclear why the difference.