Bug 181716 – Parse calc() in CSS media queries

• Component: CSS
• Status: RESOLVED FIXED

[Chris Nardi]

Parse calc() in CSS media queries

[Chris Nardi]

Created attachment 331461 [details]
Patch

[Chris Nardi]

Created attachment 331466 [details]
Patch

[Chris Nardi]

Created attachment 331467 [details]
Patch

[Chris Nardi]

Created attachment 331497 [details]
Patch

[EWS Watchlist]

Attachment 331497 [details] did not pass style-queue:


ERROR: Source/WebCore/css/MediaQueryExpression.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Nardi]

Created attachment 331505 [details]
Patch

[Chris Nardi]

Created attachment 331522 [details]
Patch

[EWS Watchlist]

Comment on attachment 331522 [details]
Patch

Attachment 331522 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6109327

New failing tests:
imported/w3c/web-platform-tests/css/mediaqueries/test_media_queries.html
fast/dom/HTMLImageElement/sizes/image-sizes-w3c-4.html
fast/dom/HTMLImageElement/sizes/image-sizes-w3c-2.html
fast/media/mq-hover-invalid.html
fast/media/mq-pointer-invalid.html
fast/media/mq-any-pointer-invalid.html
fast/media/mq-any-hover-invalid.html
fast/dom/HTMLImageElement/sizes/image-sizes-w3c-3.html
fast/dom/HTMLImageElement/sizes/image-sizes-w3c-1.html

[EWS Watchlist]

Created attachment 331531 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 331522 [details]
Patch

Attachment 331522 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6109320

New failing tests:
imported/w3c/web-platform-tests/css/mediaqueries/test_media_queries.html
fast/dom/HTMLImageElement/sizes/image-sizes-w3c-4.html
fast/dom/HTMLImageElement/sizes/image-sizes-w3c-2.html
fast/media/mq-hover-invalid.html
fast/media/mq-pointer-invalid.html
fast/media/mq-any-pointer-invalid.html
fast/media/mq-any-hover-invalid.html
fast/dom/HTMLImageElement/sizes/image-sizes-w3c-3.html
fast/dom/HTMLImageElement/sizes/image-sizes-w3c-1.html

[EWS Watchlist]

Created attachment 331532 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Chris Nardi]

Created attachment 331549 [details]
Patch

[Chris Nardi]

Created attachment 331550 [details]
Patch

[Chris Nardi]

Created attachment 331558 [details]
Patch

[Chris Nardi]

All tests are now passing. This is an adaptation of my Chromium patch at https://chromium-review.googlesource.com/c/chromium/src/+/866152. I also imported web-platform-tests/css/mediaqueries in order to test my changes.

[Chris Nardi]

Created attachment 331666 [details]
Patch

[EWS Watchlist]

Comment on attachment 331666 [details]
Patch

Attachment 331666 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6126716

New failing tests:
imported/w3c/web-platform-tests/css/mediaqueries/test_media_queries.html

[EWS Watchlist]

Created attachment 331669 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Nardi]

Created attachment 331671 [details]
Patch

[Chris Nardi]

Created attachment 331672 [details]
Patch

[Chris Nardi]

Created attachment 331681 [details]
Patch

[EWS Watchlist]

Comment on attachment 331681 [details]
Patch

Attachment 331681 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6128700

New failing tests:
http/tests/misc/resource-timing-resolution.html

[EWS Watchlist]

Created attachment 331696 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Nardi]

I think these new test failures are pre-existing; I only touched CSS code and nothing to do with timings.

[Antti Koivisto]

Comment on attachment 331681 [details]
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
> [phone]  Chris Nardi  <[e-mail]>
> +
> +        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?

[Chris Nardi]

Created attachment 331726 [details]
Patch

[EWS Watchlist]

Attachment 331726 [details] did not pass style-queue:


ERROR: Source/WebCore/css/MediaQueryExpression.cpp:177:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/MediaQueryExpression.cpp:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 78 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Nardi]

Created attachment 331727 [details]
Patch

[Chris Nardi]

Created attachment 331728 [details]
Patch

[Chris Nardi]

Created attachment 331731 [details]
Patch

[Chris Nardi]

Created attachment 331732 [details]
Patch

[Chris Nardi]

Created attachment 331733 [details]
Patch

[Chris Nardi]

Comment on attachment 331681 [details]
Patch

Apologies for the mess of emails; I really need to fix my local build (bug 181639).

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

>> LayoutTests/ChangeLog:14
>> +        * 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)

Done.

>> Source/WebCore/ChangeLog:19
>> +        (WebCore::featureWithZeroOrOne):
> 
> You should add some function level ChangeLog comments explaining the code changes you are making.

Done.

>> 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'.

Thanks, I fixed that.

>> 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

Changed to firstValue.

>> Source/WebCore/css/MediaQueryExpression.cpp:180
>> +        value = CSSPropertyParserHelpers::consumeResolution(range);
> 
> This might read better as a helper function that does early returns:
> 
> RefPtr<CSSPrimitiveValue> value = consumeFirstValue(...)

I added a helper function in this format.

>> 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?

Yep; I was getting a test failure but I realize now that's just due to the rem unit being miscalculated which is preexisting. Somehow this way must avoid that bug but I'm not sure why... I'll just use what you suggest though since that makes more sense.

>> Source/WebCore/css/MediaQueryExpression.cpp:211
>> +        } 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

I changed it around; it does look a lot better.

>> Source/WebCore/css/MediaQueryExpression.cpp:223
>>      }
> 
> This does nothing.

Changed per above.

>> 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...

Yeah, I thought this was awkward but I wasn't sure the best way to go about it.

>> Source/WebCore/css/parser/MediaQueryParser.cpp:248
>> +    }
> 
> 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?

True, although we would still need a special case for handleBlocks/blockWatcher as calc functions would appear to be another block and fail out due to this check. Also, all other functions do their own consuming so we'd just be adding ~10 range.consume() statements total... I'm not sure what would be cleanest.

>> Source/WebCore/css/parser/MediaQueryParser.cpp:263
>> +        processToken(CSSParserToken(EOFToken), range);
> 
> There is this special case where token is not range.peek(). Maybe this could be handled better?

Yeah, I'll try to play around with this to make it better.

[Chris Nardi]

Created attachment 331736 [details]
Patch

[Antti Koivisto]

Comment on attachment 331736 [details]
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.

[Chris Nardi]

Created attachment 331872 [details]
Patch

[Chris Nardi]

Comment on attachment 331736 [details]
Patch

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

>> Source/WebCore/css/MediaQueryExpression.cpp:185
>> +        return firstValue;
> 
> These could be written somewhat more compactly as
> 
> if (auto value = CSSPropertyParserHelpers::consumeLength(range, HTMLStandardMode, ValueRangeNonNegative)) 
>     return value;

Done.

>> Source/WebCore/css/parser/MediaQueryParser.cpp:252
>> +        ((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.

Yeah, makes sense; I can try for a follow-up patch with this.

[Antti Koivisto]

Comment on attachment 331872 [details]
Patch

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

> Source/WebCore/css/MediaQueryExpression.cpp:40
> +static inline bool featureWithValidIdent(const AtomicString& mediaFeature, const CSSPrimitiveValue* value)

'const CSSPrimitiveValue&' would be better since the value is non-null. RefPtr has operator* so invoking with 'featureWithValidIdent(m_mediaFeature, *firstValue)' works.

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

Also here.

> Source/WebCore/css/MediaQueryExpression.cpp:68
> +static inline bool featureWithValidPositiveLength(const String& mediaFeature, const CSSPrimitiveValue* value)

And here

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

And here

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

And here

[Chris Nardi]

Created attachment 331911 [details]
Patch

[Chris Nardi]

(In reply to Antti Koivisto from comment #38)
> Comment on attachment 331872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331872&action=review
> 
> > Source/WebCore/css/MediaQueryExpression.cpp:40
> > +static inline bool featureWithValidIdent(const AtomicString& mediaFeature, const CSSPrimitiveValue* value)
> 
> 'const CSSPrimitiveValue&' would be better since the value is non-null.
> RefPtr has operator* so invoking with 'featureWithValidIdent(m_mediaFeature,
> *firstValue)' works.
> 
> > Source/WebCore/css/MediaQueryExpression.cpp:58
> > +static inline bool featureWithValidDensity(const String& mediaFeature, const CSSPrimitiveValue* value)
> 
> Also here.
> 
> > Source/WebCore/css/MediaQueryExpression.cpp:68
> > +static inline bool featureWithValidPositiveLength(const String& mediaFeature, const CSSPrimitiveValue* value)
> 
> And here
> 
> > Source/WebCore/css/MediaQueryExpression.cpp:100
> > +static inline bool featureWithPositiveInteger(const String& mediaFeature, const CSSPrimitiveValue* value)
> 
> And here
> 
> > Source/WebCore/css/MediaQueryExpression.cpp:121
> > +static inline bool featureWithZeroOrOne(const String& mediaFeature, const CSSPrimitiveValue* value)
> 
> And here

Done for all of those changes.

[Chris Nardi]

Created attachment 331913 [details]
Patch

[Chris Nardi]

Created attachment 331915 [details]
Patch

[EWS Watchlist]

Comment on attachment 331915 [details]
Patch

Attachment 331915 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6168164

New failing tests:
compositing/debug-borders-dynamic.html

[EWS Watchlist]

Created attachment 331920 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[WebKit Commit Bot]

Comment on attachment 331915 [details]
Patch

Clearing flags on attachment: 331915

Committed r227295: <https://trac.webkit.org/changeset/227295>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36733894>