[webkit-reviews] review denied: [Bug 172906] CSS.supports(one_string) should accept paren-less declaration : [Attachment 311964] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Emilio Cobos Álvarez
<ecobos at igalia.com>'s request for review:
Bug 172906: CSS.supports(one_string) should accept paren-less declaration
https://bugs.webkit.org/show_bug.cgi?id=172906

Attachment 311964: Patch

https://bugs.webkit.org/attachment.cgi?id=311964&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 311964
  --> https://bugs.webkit.org/attachment.cgi?id=311964
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.