[webkit-reviews] review granted: [Bug 181716] Parse calc() in CSS media queries : [Attachment 331736] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Chris Nardi
<christopherncarmel at hotmail.com>'s request for review:
Bug 181716: Parse calc() in CSS media queries
https://bugs.webkit.org/show_bug.cgi?id=181716

Attachment 331736: Patch

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




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

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

r=me

> Source/WebCore/css/MediaQueryExpression.cpp:185
> +    firstValue = CSSPropertyParserHelpers::consumeLength(range,
HTMLStandardMode, ValueRangeNonNegative);
> +    if (firstValue)
> +	   return firstValue;

These could be written somewhat more compactly as

if (auto value = CSSPropertyParserHelpers::consumeLength(range,
HTMLStandardMode, ValueRangeNonNegative)) 
    return value;

> Source/WebCore/css/parser/MediaQueryParser.cpp:252
> +    if (m_state != ReadFeatureValue || type == WhitespaceToken) {
> +	   handleBlocks(token);
> +	   m_blockWatcher.handleToken(token);
> +	   range.consume();
> +    }
>  
>      // Call the function that handles current state
>      if (type != WhitespaceToken)
> -	   ((this)->*(m_state))(type, token);
> +	   ((this)->*(m_state))(type, token, range);

I think a good refactoring would be to make State a normal enum class instead
of a function pointer and replace this code with a switch (m_state) that simply
invokes the right function. Then different cases could have different arguments
(eliminating unused parameters and the no-op done() function) and the whole
thing would be more readable.

That can be left for future patches though.