| Summary: | Parse calc() in CSS media queries | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Nardi <cnardi> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, dino, ews-watchlist, koivisto, rniwa, simon.fraser, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Chris Nardi
2018-01-16 21:10:58 PST
Created attachment 331461 [details]
Patch
Created attachment 331466 [details]
Patch
Created attachment 331467 [details]
Patch
Created attachment 331497 [details]
Patch
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.
Created attachment 331505 [details]
Patch
Created attachment 331522 [details]
Patch
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 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 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 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
Created attachment 331549 [details]
Patch
Created attachment 331550 [details]
Patch
Created attachment 331558 [details]
Patch
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. Created attachment 331666 [details]
Patch
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 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
Created attachment 331671 [details]
Patch
Created attachment 331672 [details]
Patch
Created attachment 331681 [details]
Patch
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 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
I think these new test failures are pre-existing; I only touched CSS code and nothing to do with timings. 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? Created attachment 331726 [details]
Patch
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.
Created attachment 331727 [details]
Patch
Created attachment 331728 [details]
Patch
Created attachment 331731 [details]
Patch
Created attachment 331732 [details]
Patch
Created attachment 331733 [details]
Patch
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. Created attachment 331736 [details]
Patch
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. Created attachment 331872 [details]
Patch
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 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 Created attachment 331911 [details]
Patch
(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. Created attachment 331913 [details]
Patch
Created attachment 331915 [details]
Patch
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 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 on attachment 331915 [details] Patch Clearing flags on attachment: 331915 Committed r227295: <https://trac.webkit.org/changeset/227295> All reviewed patches have been landed. Closing bug. |