Some invalid media queries are parsed as valid

Some invalid media queries are parsed as valid

Make media query parsing more strict by not allowing
restrictors right after after we already parsed one.

BUG=451263
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189353

[rwlbuis, 2015-01-23 22:51:12]

rob.buis@samsung.com changed reviewers:
+ timloh@chromium.org

[rwlbuis, 2015-01-23 22:51:12]

@timloh I need some advice on this one. As you can see
there are many fails, but I think most (all?) are due
to the new CSS parser and not the media query part. Is
there some practical way to test my changes (note I also
had to enable the runtime and comment out some unit tests)?

[Timothy Loh, 2015-01-27 03:57:46]

On 2015/01/23 22:51:12, rwlbuis wrote:
> @timloh I need some advice on this one. As you can see
> there are many fails, but I think most (all?) are due
> to the new CSS parser and not the media query part. Is
> there some practical way to test my changes (note I also
> had to enable the runtime and comment out some unit tests)?

Yeah, the new CSS Parser still fails a whole bunch of tests (the list fits on my
screen though, so it isn't that far off!). You can add unit tests in
MediaConditionTest and/or extend the layout tests to use window.matchMedia
(which uses the new MQ parser).

[rwlbuis, 2015-01-27 22:21:01]

On 2015/01/27 03:57:46, Timothy Loh wrote:
> On 2015/01/23 22:51:12, rwlbuis wrote:
> > @timloh I need some advice on this one. As you can see
> > there are many fails, but I think most (all?) are due
> > to the new CSS parser and not the media query part. Is
> > there some practical way to test my changes (note I also
> > had to enable the runtime and comment out some unit tests)?
> 
> Yeah, the new CSS Parser still fails a whole bunch of tests (the list fits on
my
> screen though, so it isn't that far off!). You can add unit tests in
> MediaConditionTest and/or extend the layout tests to use window.matchMedia
> (which uses the new MQ parser).

Thanks for pointing that out. I added mq-invalid-media-type-matchMedia.html on
the basis of mq-invalid-media-type.html. However the w3c one is trickier, I
guess it is not run in the default test runs? I ended up copying the two
subtests for now.

[Timothy Loh, 2015-01-29 22:55:17]

timloh@chromium.org changed reviewers:
+ yoav@yoav.ws

[Timothy Loh, 2015-01-29 22:55:18]

+yoav, could you take a look? :)

[Yoav Weiss, 2015-01-30 08:31:51]

Sure :)

For parser-related tests, I strongly prefer the use of unit tests, rather than
the use of tiny, bug specific layout tests. Also, if we're writing LayoutTests,
it's better to use testharness.js and make them shareable with other browsers.

For the MediaQueryParser, its unit tests are located in
core/css/MediaQuerySetTest.cpp (I know).
You could easily add your tests there by adding the following to the test
structure:
          {"not not", "not all", true},
          {"not and", "not all", true},
          {"not only", "not all", true},
          {"not or", "not all", true},
          {"not (orientation)", "not all", true},
          {"only (orientation)", "not all", true},

https://codereview.chromium.org/867363002/diff/40001/Source/core/css/parser/M...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/867363002/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:91: } else {
Nit: Can you make that an "else if"?

https://codereview.chromium.org/867363002/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:95: ||
equalIgnoringCase(token.value(), "only"))) {
Can you break out the "equqlIgnoringCase"s into their own helper function?
"isRestrictorOrLogicalOperator" (or a better name :) )?

Also - I don't know if this is a performance bottleneck in any way, but we're
starting to lowercase that token a few times too many... Can you add a FIXME in
that spirit, so we'd later move the lowercasing up the chain.

[rwlbuis, 2015-01-30 15:20:42]

On 2015/01/30 08:31:51, Yoav Weiss wrote:
> Sure :)
> 
> For parser-related tests, I strongly prefer the use of unit tests, rather than
> the use of tiny, bug specific layout tests. Also, if we're writing
LayoutTests,
> it's better to use testharness.js and make them shareable with other browsers.

Good to know.

> For the MediaQueryParser, its unit tests are located in
> core/css/MediaQuerySetTest.cpp (I know).
> You could easily add your tests there by adding the following to the test
> structure:
>           {"not not", "not all", true},
>           {"not and", "not all", true},
>           {"not only", "not all", true},
>           {"not or", "not all", true},
>           {"not (orientation)", "not all", true},
>           {"only (orientation)", "not all", true},

Yeah that works well. I also added some tests when starting with "only".

>
https://codereview.chromium.org/867363002/diff/40001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryParser.cpp (right):
> 
>
https://codereview.chromium.org/867363002/diff/40001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.cpp:91: } else {
> Nit: Can you make that an "else if"?

Fixed. Usually I prefer else if too :) In this case I missed it.

>
https://codereview.chromium.org/867363002/diff/40001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.cpp:95: ||
> equalIgnoringCase(token.value(), "only"))) {
> Can you break out the "equqlIgnoringCase"s into their own helper function?
> "isRestrictorOrLogicalOperator" (or a better name :) )?

Done, keeping that name for now.

> Also - I don't know if this is a performance bottleneck in any way, but we're
> starting to lowercase that token a few times too many... Can you add a FIXME
in
> that spirit, so we'd later move the lowercasing up the chain.

I added a FIXME.

[Yoav Weiss, 2015-02-02 17:16:53]

LGTM. Thanks :)

[rwlbuis, 2015-02-02 17:17:27]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-02-02 17:18:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/867363002/60001

[commit-bot: I haz the power, 2015-02-02 18:36:57]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189353