[webkit-reviews] review granted: [Bug 226473] Factor out token-type dependent CSS property parsing functions to allow more code sharing : [Attachment 430211] 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 226473: Factor out token-type dependent CSS property parsing functions to
allow more code sharing
https://bugs.webkit.org/show_bug.cgi?id=226473

Attachment 430211: Patch

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




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

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

> Source/WebCore/ChangeLog:11
> +	   a FunctionToken) into their own functions. This allow sharing
between functions that

typo: allows

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:92
> +    // FIXME: Presentational HTML attributes shouldn't use the CSS parser
for lengths

Our style includes periods in sentence style comments.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:100
> +    if (value == 0 && unitlessZero == UnitlessZeroQuirk::Allow)
> +	   return true;
> +
> +    if (isUnitLessValueParsingEnabledForMode(cssParserMode))
> +	   return true;
> +    
> +    return cssParserMode == HTMLQuirksMode && unitless ==
UnitlessQuirk::Allow;

I would be tempted to write a single boolean expression in the "vertical style"
rather than three return statement paragraphs.

Noticing that "unitless" is capitalized as if it was two words in the
isUnitLessValueParsingEnabledForMode function.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:319
> +    CalcParser calcParser(range, CalculationCategory::Number, valueRange,
symbolTable, cssValuePool);
> +    return calcParser.consumeValueIfCategory(CalculationCategory::Number);

Just call the local variable "parser?" When the scope is this tight, for me the
single word just makes the code more readable even if it’s not entirely
unambiguous.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:504
> +static RefPtr<CSSPrimitiveValue>
consumeLengthCSSPrimitiveValueWithCalcWithKnownTokenTypeNumber(CSSParserTokenRa
nge& range, const CSSCalcSymbolTable& symbolTable, ValueRange valueRange,
CSSParserMode cssParserMode, UnitlessQuirk unitless, CSSValuePool&
cssValuePool)

Consider taking of the extra words off some of these argument names? I would
just call the CSSValuePool pool.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1223
> +static inline bool divisibleBy100(double value)
> +{
> +    return static_cast<int>(value / 100) * 100 == value;
> +}

This works only because the caller already checks that the value is in range
between 0 and 100. I think it’s also non-obvious that this checks if the number
is an integer as a side effect.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1239
> +#if !ENABLE(VARIATION_FONTS)
> +	   if (*result <= 0 || *result >= 1000 || !divisibleBy100(*result))
> +	       break;
> +#endif

I noticed that the code below does [1,1000] but this does [0,1000].

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1245
> +	   if (result < 1 || result > 1000)

For better NaN handling I think we want to reverse the logic:

    if (!(result >= 1 && result <= 1000))