[Webkit-unassigned] [Bug 181716] Parse calc() in CSS media queries

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 19 02:36:14 PST 2018


https://bugs.webkit.org/show_bug.cgi?id=181716

Antti Koivisto <koivisto at iki.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #331681|review?                     |review-
              Flags|                            |

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

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

Looks generally good. Comments are mostly style related.

> LayoutTests/ChangeLog:14
> +2018-01-17  Chris Nardi  <csnardi1 at gmail.com>
> +
> +        Parse calc() in CSS media queries
> +        https://bugs.webkit.org/show_bug.cgi?id=181716
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * TestExpectations:
> +        * fast/dom/HTMLImageElement/sizes/image-sizes-w3c-1-expected.txt:
> +        * fast/dom/HTMLImageElement/sizes/image-sizes-w3c-2-expected.txt:
> +        * fast/dom/HTMLImageElement/sizes/image-sizes-w3c-3-expected.txt:
> +        * fast/dom/HTMLImageElement/sizes/image-sizes-w3c-4-expected.txt:
> +        * fast/media/w3c/README: Removed.
> +        * fast/media/w3c/test_media_queries-expected.txt: Removed.

Could you explain in the ChangeLog what you are doing with the tests? (Importing new WPTs and removing old ones I guess)

> Source/WebCore/ChangeLog:19
> +        * css/MediaQueryExpression.cpp:
> +        (WebCore::featureWithValidDensity):
> +        (WebCore::featureWithValidPositiveLength):
> +        (WebCore::featureExpectingPositiveInteger):
> +        (WebCore::featureWithPositiveInteger):
> +        (WebCore::featureWithPositiveNumber):
> +        (WebCore::featureWithZeroOrOne):

You should add some function level ChangeLog comments explaining the code changes you are making.

> Source/WebCore/css/MediaQueryExpression.cpp:55
> +static inline bool featureWithValidDensity(const String& mediaFeature, const RefPtr<CSSPrimitiveValue> value)

- We use Ref<> for non-null things. You are not null testing anywhere so value is expected to be non-null.
- Using Ref/RefPtr here achieves nothing and causes ref churn. I think what you really want here is 'const CSSPrimitiveValue& value'.

> Source/WebCore/css/MediaQueryExpression.cpp:98
> +static inline bool featureWithPositiveInteger(const String& mediaFeature, const RefPtr<CSSPrimitiveValue> value)

Same here.

> Source/WebCore/css/MediaQueryExpression.cpp:105
> +static inline bool featureWithPositiveNumber(const String& mediaFeature, const RefPtr<CSSPrimitiveValue> value)

Same here.

> Source/WebCore/css/MediaQueryExpression.cpp:119
> +static inline bool featureWithZeroOrOne(const String& mediaFeature, const RefPtr<CSSPrimitiveValue> value)

Same here.

> Source/WebCore/css/MediaQueryExpression.cpp:173
> +    RefPtr<CSSPrimitiveValue> value = CSSPropertyParserHelpers::consumeInteger(range, 0);

A more descriptive name than 'value' would be better, especially since this function also deals with m_value member, firstValue or something

> Source/WebCore/css/MediaQueryExpression.cpp:180
> +    RefPtr<CSSPrimitiveValue> ident;
> +    if (!value && !featureExpectingPositiveInteger(m_mediaFeature) && !isAspectRatioFeature(m_mediaFeature))
> +        value = CSSPropertyParserHelpers::consumeNumber(range, ValueRangeNonNegative);
> +    if (!value)
> +        value = CSSPropertyParserHelpers::consumeLength(range, HTMLStandardMode, ValueRangeNonNegative);
> +    if (!value)
> +        value = CSSPropertyParserHelpers::consumeResolution(range);

This might read better as a helper function that does early returns:

RefPtr<CSSPrimitiveValue> value = consumeFirstValue(...)

> Source/WebCore/css/MediaQueryExpression.cpp:202
> +            m_value = CSSPrimitiveValue::create(value->doubleValue(), (CSSPrimitiveValue::UnitType) value->primitiveType());

Isn't this the same as just assigning m_value = value?

> Source/WebCore/css/MediaQueryExpression.cpp:211
> +        } else if (featureWithPositiveInteger(m_mediaFeature, value) || featureWithPositiveNumber(m_mediaFeature, value)
> +            || featureWithZeroOrOne(m_mediaFeature, value)) {
> +            // Media features that must have non-negative integer value,
> +            // or media features that must have non-negative number value,
> +            // or media features that must have (0|1) value.
> +            m_value = CSSPrimitiveValue::create(value->doubleValue(), CSSPrimitiveValue::UnitType::CSS_NUMBER);
> +            m_isValid = true;
> +        } else

WebKit prefers early return style. All these else ifs can be replaced with plain ifs and returns at the end:

if (something) {
   ...
   return;
}
if (somethingelse) {
   ...
   return;
}

The code will read better

> Source/WebCore/css/MediaQueryExpression.cpp:223
> +    } else {
> +        return;
>      }

This does nothing.

> Source/WebCore/css/parser/MediaQueryParser.cpp:240
> +void MediaQueryParser::processToken(const CSSParserToken& token, CSSParserTokenRange& range)

token is just range.peek() so providing it separately seems unnecessary. 

Since this function now consumes tokens maybe it should be renamed consumeToken? Actually, it can now consume multiple tokens...

> Source/WebCore/css/parser/MediaQueryParser.cpp:248
> +    if (m_state != ReadFeatureValue || type == WhitespaceToken) {
> +        handleBlocks(token);
> +        m_blockWatcher.handleToken(token);
> +        range.consume();
> +    }

It is pretty awkward that this knows readFeatureValue does its own consuming and has special case for it. Can we avoid special case, for example by making all these state functions do their own consuming?

> Source/WebCore/css/parser/MediaQueryParser.cpp:263
>      // FIXME: Can we get rid of this special case?
>      if (m_parserType == MediaQuerySetParser)
> -        processToken(CSSParserToken(EOFToken));
> +        processToken(CSSParserToken(EOFToken), range);

There is this special case where token is not range.peek(). Maybe this could be handled better?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180119/8509c4fe/attachment-0001.html>


More information about the webkit-unassigned mailing list