[webkit-reviews] review granted: [Bug 192828] Fix CSS Variable cycle detection and import CSS Variables web platform tests : [Attachment 357977] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 192828: Fix CSS Variable cycle detection and import CSS Variables web
platform tests
https://bugs.webkit.org/show_bug.cgi?id=192828

Attachment 357977: Patch

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




--- Comment #21 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 357977
  --> https://bugs.webkit.org/attachment.cgi?id=357977
Patch

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

r=me, but I think this could be done in a nicer way.

> Source/WebCore/css/CSSVariableReferenceValue.cpp:49
> +static bool resolveTokenRange(CSSParserTokenRange, Vector<CSSParserToken>&,
ApplyCascadedPropertyState&, bool&);
>  
> -static bool resolveVariableFallback(CSSParserTokenRange range,
Vector<CSSParserToken>& result, ApplyCascadedPropertyState& state)
> +static bool resolveVariableFallback(CSSParserTokenRange range,
Vector<CSSParserToken>& result, ApplyCascadedPropertyState& state, bool&
substitutionValid)

Do all combinations of the return bool and substitutionValid return argument
make sense? If not, maybe you just want to return an 3-value enum instead?

Or you could return a struct with both bits and avoid the return argument that
way.

> Source/WebCore/css/CSSVariableReferenceValue.cpp:86
> +	   bool fallbacksubstitutionValid = true;
> +	   if (resolveVariableFallback(CSSParserTokenRange(range),
fallbackResult, state, fallbacksubstitutionValid)) {

How do I know fallbacksubstitutionValid bit needs to be set to true ? This sort
of code should not require specific initialization by the client.

Also fallbacksubstitutionValid is unused here so requiring it is annoying. Not
using return arguments in the first place would avoid both problems.

There pattern repeats in several places in this patch.