Bug 181716 - Parse calc() in CSS media queries
Summary: Parse calc() in CSS media queries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-16 21:10 PST by Chris Nardi
Modified: 2018-01-22 08:09 PST (History)
7 users (show)

See Also:


Attachments
Patch (125.16 KB, patch)
2018-01-16 21:14 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (127.79 KB, patch)
2018-01-16 21:41 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (127.79 KB, patch)
2018-01-16 21:53 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (127.95 KB, patch)
2018-01-17 08:46 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (127.99 KB, patch)
2018-01-17 10:00 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (186.23 KB, patch)
2018-01-17 11:18 PST, Chris Nardi
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.27 MB, application/zip)
2018-01-17 12:33 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.98 MB, application/zip)
2018-01-17 12:36 PST, Build Bot
no flags Details
Patch (183.96 KB, patch)
2018-01-17 15:45 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (184.00 KB, patch)
2018-01-17 15:51 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (188.72 KB, patch)
2018-01-17 16:35 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (188.18 KB, patch)
2018-01-18 15:36 PST, Chris Nardi
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.36 MB, application/zip)
2018-01-18 16:35 PST, Build Bot
no flags Details
Patch (188.53 KB, patch)
2018-01-18 16:45 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (187.85 KB, patch)
2018-01-18 16:46 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (188.03 KB, patch)
2018-01-18 17:46 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.15 MB, application/zip)
2018-01-18 19:24 PST, Build Bot
no flags Details
Patch (189.00 KB, patch)
2018-01-19 07:26 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (189.00 KB, patch)
2018-01-19 07:33 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (189.01 KB, patch)
2018-01-19 07:44 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (189.00 KB, patch)
2018-01-19 07:52 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (189.03 KB, patch)
2018-01-19 08:00 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (189.05 KB, patch)
2018-01-19 08:18 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (189.16 KB, patch)
2018-01-19 08:32 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (189.02 KB, patch)
2018-01-21 09:49 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (188.99 KB, patch)
2018-01-22 03:43 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (188.98 KB, patch)
2018-01-22 03:53 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Patch (188.98 KB, patch)
2018-01-22 04:07 PST, Chris Nardi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.12 MB, application/zip)
2018-01-22 05:38 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Nardi 2018-01-16 21:10:58 PST
Parse calc() in CSS media queries
Comment 1 Chris Nardi 2018-01-16 21:14:17 PST
Created attachment 331461 [details]
Patch
Comment 2 Chris Nardi 2018-01-16 21:41:08 PST
Created attachment 331466 [details]
Patch
Comment 3 Chris Nardi 2018-01-16 21:53:01 PST
Created attachment 331467 [details]
Patch
Comment 4 Chris Nardi 2018-01-17 08:46:56 PST
Created attachment 331497 [details]
Patch
Comment 5 Build Bot 2018-01-17 08:51:44 PST
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.
Comment 6 Chris Nardi 2018-01-17 10:00:59 PST
Created attachment 331505 [details]
Patch
Comment 7 Chris Nardi 2018-01-17 11:18:12 PST
Created attachment 331522 [details]
Patch
Comment 8 Build Bot 2018-01-17 12:33:27 PST
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
Comment 9 Build Bot 2018-01-17 12:33:28 PST
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
Comment 10 Build Bot 2018-01-17 12:36:12 PST
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
Comment 11 Build Bot 2018-01-17 12:36:13 PST
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
Comment 12 Chris Nardi 2018-01-17 15:45:29 PST
Created attachment 331549 [details]
Patch
Comment 13 Chris Nardi 2018-01-17 15:51:43 PST
Created attachment 331550 [details]
Patch
Comment 14 Chris Nardi 2018-01-17 16:35:57 PST
Created attachment 331558 [details]
Patch
Comment 15 Chris Nardi 2018-01-17 18:14:36 PST
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.
Comment 16 Chris Nardi 2018-01-18 15:36:01 PST
Created attachment 331666 [details]
Patch
Comment 17 Build Bot 2018-01-18 16:35:32 PST
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
Comment 18 Build Bot 2018-01-18 16:35:33 PST
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
Comment 19 Chris Nardi 2018-01-18 16:45:12 PST
Created attachment 331671 [details]
Patch
Comment 20 Chris Nardi 2018-01-18 16:46:38 PST
Created attachment 331672 [details]
Patch
Comment 21 Chris Nardi 2018-01-18 17:46:06 PST
Created attachment 331681 [details]
Patch
Comment 22 Build Bot 2018-01-18 19:24:08 PST
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
Comment 23 Build Bot 2018-01-18 19:24:09 PST
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
Comment 24 Chris Nardi 2018-01-18 19:27:14 PST
I think these new test failures are pre-existing; I only touched CSS code and nothing to do with timings.
Comment 25 Antti Koivisto 2018-01-19 02:36:14 PST
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
> +2018-01-17  Chris Nardi  <csnardi1@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?
Comment 26 Chris Nardi 2018-01-19 07:26:35 PST
Created attachment 331726 [details]
Patch
Comment 27 Build Bot 2018-01-19 07:29:57 PST
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.
Comment 28 Chris Nardi 2018-01-19 07:33:58 PST
Created attachment 331727 [details]
Patch
Comment 29 Chris Nardi 2018-01-19 07:44:08 PST
Created attachment 331728 [details]
Patch
Comment 30 Chris Nardi 2018-01-19 07:52:20 PST
Created attachment 331731 [details]
Patch
Comment 31 Chris Nardi 2018-01-19 08:00:58 PST
Created attachment 331732 [details]
Patch
Comment 32 Chris Nardi 2018-01-19 08:18:44 PST
Created attachment 331733 [details]
Patch
Comment 33 Chris Nardi 2018-01-19 08:24:17 PST
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.
Comment 34 Chris Nardi 2018-01-19 08:32:45 PST
Created attachment 331736 [details]
Patch
Comment 35 Antti Koivisto 2018-01-21 09:38:39 PST
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.
Comment 36 Chris Nardi 2018-01-21 09:49:20 PST
Created attachment 331872 [details]
Patch
Comment 37 Chris Nardi 2018-01-21 09:53:09 PST
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.
Comment 38 Antti Koivisto 2018-01-22 00:09:39 PST
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
Comment 39 Chris Nardi 2018-01-22 03:43:19 PST
Created attachment 331911 [details]
Patch
Comment 40 Chris Nardi 2018-01-22 03:45:07 PST
(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.
Comment 41 Chris Nardi 2018-01-22 03:53:30 PST
Created attachment 331913 [details]
Patch
Comment 42 Chris Nardi 2018-01-22 04:07:25 PST
Created attachment 331915 [details]
Patch
Comment 43 Build Bot 2018-01-22 05:38:01 PST
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
Comment 44 Build Bot 2018-01-22 05:38:03 PST
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
Comment 45 WebKit Commit Bot 2018-01-22 08:07:19 PST
Comment on attachment 331915 [details]
Patch

Clearing flags on attachment: 331915

Committed r227295: <https://trac.webkit.org/changeset/227295>
Comment 46 WebKit Commit Bot 2018-01-22 08:07:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 47 Radar WebKit Bug Importer 2018-01-22 08:09:56 PST
<rdar://problem/36733894>