Bug 226473 – Factor out token-type dependent CSS property parsing functions to allow more code sharing

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Factor out token-type dependent CSS property parsing functions to allow more code sharing

[Sam Weinig]

Created attachment 430208 [details]
Patch

[Sam Weinig]

Created attachment 430211 [details]
Patch

[Darin Adler]

Comment on attachment 430211 [details]
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(CSSParserTokenRange& 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))

[Sam Weinig]

Created attachment 430264 [details]
Patch

[EWS]

Committed r278311 (238348@main): <https://commits.webkit.org/238348@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430264 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78719120>