Bug 226473 - Factor out token-type dependent CSS property parsing functions to allow more code sharing
Summary: Factor out token-type dependent CSS property parsing functions to allow more ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-31 12:07 PDT by Sam Weinig
Modified: 2021-06-01 09:50 PDT (History)
9 users (show)

See Also:


Attachments
Patch (86.42 KB, patch)
2021-05-31 12:17 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (86.48 KB, patch)
2021-05-31 13:50 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (99.45 KB, patch)
2021-06-01 08:24 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2021-05-31 12:07:55 PDT
Factor out token-type dependent CSS property parsing functions to allow more code sharing
Comment 1 Sam Weinig 2021-05-31 12:17:13 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2021-05-31 13:50:18 PDT
Created attachment 430211 [details]
Patch
Comment 3 Darin Adler 2021-05-31 16:42:35 PDT
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))
Comment 4 Sam Weinig 2021-06-01 08:24:38 PDT
Created attachment 430264 [details]
Patch
Comment 5 EWS 2021-06-01 09:49:15 PDT
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].
Comment 6 Radar WebKit Bug Importer 2021-06-01 09:50:22 PDT
<rdar://problem/78719120>