[webkit-reviews] review granted: [Bug 202501] Support percentages in the scale() transform functions, and the scale property : [Attachment 437518] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 202501: Support percentages in the scale() transform functions, and the
scale property
https://bugs.webkit.org/show_bug.cgi?id=202501

Attachment 437518: Patch

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




--- Comment #4 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 437518
  --> https://bugs.webkit.org/attachment.cgi?id=437518
Patch

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

> Source/WebCore/ChangeLog:9
> +	   on the scale property. These value are converted by numbers by
dividing by 100

"converted by numbers" -> "converted to numbers".

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1796
> +static bool consumeNumbersOrPercentsDividedBy100(CSSParserTokenRange& args,
RefPtr<CSSFunctionValue>& transformValue, unsigned numberOfArguments)

RefPtr<CSSFunctionValue> -> Ref<CSSFunctionValue>.

I think also naming this something about being a comma separated list would be
good.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1799
> +	   RefPtr<CSSPrimitiveValue> parsedValue =
consumeNumberOrPercentDividedBy100(args, ValueRange::All);

I would use auto here.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1805
> +    } while (numberOfArguments);

You could make this a tad more efficient (remove a branch) if you did it like:

auto parseNumberAndAppend = [&] {
    auto parsedValue = consumeNumberOrPercentDividedBy100(args,
ValueRange::All);
    if (!parsedValue)
	return false;

    transformValue->append(parsedValue.releaseNonNull());
    --numberOfArguments;
    return true;
};

if (!parseNumberAndAppend())
    return false;

while (numberOfArguments) {
    if (!consumeCommaIncludingWhitespace(args))
	return false;

    if (!parseNumberAndAppend())
	return false;
}

return true;