Summary: | Factor out token-type dependent CSS property parsing functions to allow more code sharing | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, macpherson, menard, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Sam Weinig
2021-05-31 12:07:55 PDT
Created attachment 430208 [details]
Patch
Created attachment 430211 [details]
Patch
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)) Created attachment 430264 [details]
Patch
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]. |