Patch incoming.
Created attachment 311962 [details] Patch
Hi Dean, David, looking at the blame log you've reviewed/written most of the CSS parser changes, mind taking a look at this patch? Thanks!
Comment on attachment 311962 [details] Patch Attachment 311962 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3871480 New failing tests: imported/w3c/web-platform-tests/cssom/CSS.html
Created attachment 311963 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 311964 [details] Patch
Comment on attachment 311964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311964&action=review > Source/WebCore/css/parser/CSSSupportsParser.cpp:39 > + bool implicitParentheses = mode == ForWindowCSS; It’s not so great to name a local variable "implicit parentheses" when it really means "add implicit parentheses" or "allow implicit parentheses" or something like that. > Source/WebCore/css/parser/CSSSupportsParser.cpp:46 > + return range.peek().type() == IdentToken && parser.supportsDeclaration(range) ? Supported : Unsupported; Assuming that this were correct, it’s annoying that this code matches the last line of consumeConditionInParenthesis but doesn’t share a helper function. Even though it’s a one liner, I think maybe there should be a function so we can share this. But it doesn’t look correct to me. There are quite a few checks missing here that the other functions have, such as a check for atEnd, check for trailing whitespace, etc. I think we need lots of invalid test cases without parentheses, with things like trailing garbage to show us that this is not correct. Here are a few test cases we should add. I think these are the correct expected results: shouldBeFalse('CSS.supports("display: deadbeef")'); shouldBeTrue('CSS.supports("(display: none) and (display: block) or (display: inline)")'); shouldBeTrue('CSS.supports("not (display: deadbeef) and (display: block)")'); shouldBeFalse('CSS.supports("!important")'); shouldBeTrue('CSS.supports("top: -webkit-calc(80% - 20px)")'); shouldBeTrue('CSS.supports("background-color: rgb(0, 128, 0)")'); shouldBeTrue('CSS.supports("background: url(\'/blah\')"'); shouldBeFalse('CSS.supports("background: invalid(\'/blah\')")'); shouldBeFalse('CSS.supports("display: none;")'); shouldBeFalse('CSS.supports("display: none; garbage")'); In addition to these, generally speaking I don’t think there are enough tests cases for trailing garbage after an otherwise correct supports query.
(In reply to Darin Adler from comment #6) > Comment on attachment 311964 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=311964&action=review > > > Source/WebCore/css/parser/CSSSupportsParser.cpp:39 > > + bool implicitParentheses = mode == ForWindowCSS; > > It’s not so great to name a local variable "implicit parentheses" when it > really means "add implicit parentheses" or "allow implicit parentheses" or > something like that. Fair enough, I just inlined the check. > > Source/WebCore/css/parser/CSSSupportsParser.cpp:46 > > + return range.peek().type() == IdentToken && parser.supportsDeclaration(range) ? Supported : Unsupported; > > Assuming that this were correct, it’s annoying that this code matches the > last line of consumeConditionInParenthesis but doesn’t share a helper > function. Even though it’s a one liner, I think maybe there should be a > function so we can share this. > > But it doesn’t look correct to me. There are quite a few checks missing here > that the other functions have, such as a check for atEnd, check for trailing > whitespace, etc. I think we need lots of invalid test cases without > parentheses, with things like trailing garbage to show us that this is not > correct. How not? Trailing whitespace is ignored regardless, and junk is handled by CSSParserImpl::supportsDeclaration. > Here are a few test cases we should add. I think these are the correct > expected results: > > shouldBeFalse('CSS.supports("display: deadbeef")'); > shouldBeTrue('CSS.supports("(display: none) and (display: block) or > (display: inline)")'); This one is invalid (need parenthesis to disambiguate between and and or). > shouldBeTrue('CSS.supports("not (display: deadbeef) and (display: block)")'); Ditto (to disambiguate between not and and). > shouldBeFalse('CSS.supports("!important")'); > shouldBeTrue('CSS.supports("top: -webkit-calc(80% - 20px)")'); > shouldBeTrue('CSS.supports("background-color: rgb(0, 128, 0)")'); > shouldBeTrue('CSS.supports("background: url(\'/blah\')"'); Missing the closing paren, so this throws, but with it all these tests pass with this patch. > shouldBeFalse('CSS.supports("background: invalid(\'/blah\')")'); > > shouldBeFalse('CSS.supports("display: none;")'); > shouldBeFalse('CSS.supports("display: none; garbage")'); Ditto about this, these pass with this patch, I'll add it. In particular, this works because the only thing that parses in parens and doesn't in a normal condition per the grammar is the declaration condition: https://drafts.csswg.org/css-conditional-3/#supports_declaration_condition Which is what we check here. If you're worried about the function syntax (which currently always evaluates to false) evaluating to something else in the future, I can put both conditions in a helper function. I can also write something more complex like https://gist.github.com/emilio/812eed04fa62e1d696c7ba1f0223f16c, but I think it's unnecessary.
(In reply to Emilio Cobos Álvarez from comment #7) > (In reply to Darin Adler from comment #6) > > Comment on attachment 311964 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=311964&action=review > > > > > Source/WebCore/css/parser/CSSSupportsParser.cpp:39 > > > + bool implicitParentheses = mode == ForWindowCSS; > > > > It’s not so great to name a local variable "implicit parentheses" when it > > really means "add implicit parentheses" or "allow implicit parentheses" or > > something like that. > > Fair enough, I just inlined the check. > > > > Source/WebCore/css/parser/CSSSupportsParser.cpp:46 > > > + return range.peek().type() == IdentToken && parser.supportsDeclaration(range) ? Supported : Unsupported; > > > > Assuming that this were correct, it’s annoying that this code matches the > > last line of consumeConditionInParenthesis but doesn’t share a helper > > function. Even though it’s a one liner, I think maybe there should be a > > function so we can share this. > > > > But it doesn’t look correct to me. There are quite a few checks missing here > > that the other functions have, such as a check for atEnd, check for trailing > > whitespace, etc. I think we need lots of invalid test cases without > > parentheses, with things like trailing garbage to show us that this is not > > correct. > > How not? Trailing whitespace is ignored regardless, and junk is handled by > CSSParserImpl::supportsDeclaration. > > > Here are a few test cases we should add. I think these are the correct > > expected results: > > > > shouldBeFalse('CSS.supports("display: deadbeef")'); > > shouldBeTrue('CSS.supports("(display: none) and (display: block) or > > (display: inline)")'); > > This one is invalid (need parenthesis to disambiguate between and and or). > > > shouldBeTrue('CSS.supports("not (display: deadbeef) and (display: block)")'); > > Ditto (to disambiguate between not and and). > > > shouldBeFalse('CSS.supports("!important")'); > > shouldBeTrue('CSS.supports("top: -webkit-calc(80% - 20px)")'); > > shouldBeTrue('CSS.supports("background-color: rgb(0, 128, 0)")'); > > shouldBeTrue('CSS.supports("background: url(\'/blah\')"'); > > Missing the closing paren, so this throws, but with it all these tests pass > with this patch. > > > shouldBeFalse('CSS.supports("background: invalid(\'/blah\')")'); > > > > shouldBeFalse('CSS.supports("display: none;")'); > > shouldBeFalse('CSS.supports("display: none; garbage")'); > > Ditto about this, these pass with this patch, I'll add it. > > In particular, this works because the only thing that parses in parens and > doesn't in a normal condition per the grammar is the declaration condition: > https://drafts.csswg.org/css-conditional-3/#supports_declaration_condition > > Which is what we check here. If you're worried about the function syntax > (which currently always evaluates to false) evaluating to something else in > the future, I can put both conditions in a helper function. Note that this would have made a difference if the function syntax would start returning Supported values, in which case something like: CSS.supports("function(foo) random garbage"); would return the function(foo) result independently of the random garbage. I guess I'll add the atEnd() check, just so the result doesn't lie and tell Unsupported instead of Invalid.
Created attachment 311973 [details] Patch
(In reply to Emilio Cobos Álvarez from comment #8) > I guess I'll add the atEnd() check, just so the result doesn't lie and tell > Unsupported instead of Invalid. I couldn't do this in the end because CSSParserImpl::consumeDeclaration actually takes the range by value, so it isn't consumed. Anyway, I don't think it's observable per my above comments, and we already don't check whether innerRange is consumed, so if you think it deserves a fix, I'd prefer to move it to another bug.
Comment on attachment 311973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311973&action=review > LayoutTests/css3/supports-dom-api.html:47 > shouldBeFalse('CSS.supports("(display: none) and (display: block) or (display: inline)")'); > shouldBeFalse('CSS.supports("not (display: deadbeef) and (display: block)")'); > + shouldBeFalse('CSS.supports("(display: none) and (display: block) or (display: inline)")'); > + shouldBeFalse('CSS.supports("not (display: deadbeef) and (display: block)")'); Looks like these got doubled accidentally.
Created attachment 311975 [details] Patch
(In reply to Darin Adler from comment #11) > Comment on attachment 311973 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=311973&action=review > > > LayoutTests/css3/supports-dom-api.html:47 > > shouldBeFalse('CSS.supports("(display: none) and (display: block) or (display: inline)")'); > > shouldBeFalse('CSS.supports("not (display: deadbeef) and (display: block)")'); > > + shouldBeFalse('CSS.supports("(display: none) and (display: block) or (display: inline)")'); > > + shouldBeFalse('CSS.supports("not (display: deadbeef) and (display: block)")'); > > Looks like these got doubled accidentally. Thanks, good catch :)
Comment on attachment 311975 [details] Patch Clearing flags on attachment: 311975 Committed r217844: <http://trac.webkit.org/changeset/217844>
All reviewed patches have been landed. Closing bug.