Length MQs invalidate non zero numbers

Length MQs invalidate non zero numbers

According to the specification, certain media queries accept a CSS length as their value (e.g. http://www.w3.org/TR/css3-mediaqueries/#width).
A CSS length (http://www.w3.org/TR/css3-values/#lengths) must have a known unit, unless the value is zero.
This was not properly represented in the MediaQueryExp code, where any number was accepted.
So such media queries (e.g. "(max-width: 1)") were considered valid, but evaluated to false.
This CL fixes that, making such MQs invalid.

BUG=

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

[Yoav Weiss, 2014-03-04 12:26:55]

Following a chat with Tab Atkins RE https://codereview.chromium.org/171383002/
we noticed that some MQs are not handled according the spec, and are considered
valid even though they should not be.

This CL should fix that.

[kenneth.r.christiansen, 2014-03-04 12:27:58]

On 2014/03/04 12:26:55, Yoav Weiss wrote:
> Following a chat with Tab Atkins RE https://codereview.chromium.org/171383002/
> we noticed that some MQs are not handled according the spec, and are
considered
> valid even though they should not be.
> 
> This CL should fix that.

This would need a test!

[Yoav Weiss, 2014-03-04 15:10:34]

On 2014/03/04 12:27:58, kenneth.r.christiansen wrote:
> On 2014/03/04 12:26:55, Yoav Weiss wrote:
> > Following a chat with Tab Atkins RE
https://codereview.chromium.org/171383002/
> > we noticed that some MQs are not handled according the spec, and are
> considered
> > valid even though they should not be.
> > 
> > This CL should fix that.
> 
> This would need a test!

Added the tests from https://codereview.chromium.org/171383002/ at a higher
level (testing MediaQuerySet instead of the parser itself).

[kenneth.r.christiansen, 2014-03-04 15:59:29]

https://codereview.chromium.org/186603002/diff/20001/Source/core/css/MediaQue...
File Source/core/css/MediaQuerySetTest.cpp (right):

https://codereview.chromium.org/186603002/diff/20001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:22: int get8BitString(String str, char*
output)
you write the same way if it is a 16bit string

https://codereview.chromium.org/186603002/diff/20001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:115: {"", ""} // That's here as a
terminator for the loop. Do not remove!
why not 0, 0?

[Yoav Weiss, 2014-03-04 17:40:26]

On 2014/03/04 15:59:29, kenneth.r.christiansen wrote:
>
https://codereview.chromium.org/186603002/diff/20001/Source/core/css/MediaQue...
> File Source/core/css/MediaQuerySetTest.cpp (right):
> 
>
https://codereview.chromium.org/186603002/diff/20001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:22: int get8BitString(String str, char*
> output)
> you write the same way if it is a 16bit string

This function converts the strings content to char array, so that the assert
would accept them as input.
I've renamed it to better indicate that

> 
>
https://codereview.chromium.org/186603002/diff/20001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:115: {"", ""} // That's here as a
> terminator for the loop. Do not remove!
> why not 0, 0?
Changed.

I also commented out an MQ test that fails with the Bison parser. It passes with
my spec compliant MQ parser (from which these tests originally came).

[kenneth.r.christiansen, 2014-03-04 18:18:45]

lgtm with nits... maybe abarth will have a look

https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
File Source/core/css/MediaQuerySetTest.cpp (right):

https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:24: if (!str.impl()) {
that normally wouldnt need braces

https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:42: // These are the test cases.
kind of unneeded comment

https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:45: // If the second string is "", the
output string is identical to the first string.
An output string of "" represent identical input and output.

but why not just 0 instead? that is more obvious.

https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:119: {0, 0} // That's here as a terminator
for the loop. Do not remove!
That should be obvious to most people now you use 0, 0. I would add the comment,
or just add // Do not remove the terminator line.

https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:143: ASSERT_EQ((unsigned)0,
output.length());
static_cast instaed of C cast?

[Yoav Weiss, 2014-03-04 19:07:19]

On 2014/03/04 18:18:45, kenneth.r.christiansen wrote:
> lgtm with nits... maybe abarth will have a look
> 
>
https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
> File Source/core/css/MediaQuerySetTest.cpp (right):
> 
>
https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:24: if (!str.impl()) {
> that normally wouldnt need braces

Fixed.

> 
>
https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:42: // These are the test cases.
> kind of unneeded comment

Removed.

> 
>
https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:45: // If the second string is "", the
> output string is identical to the first string.
> An output string of "" represent identical input and output.
> 
> but why not just 0 instead? that is more obvious.

You're right. 0 is better.

> 
>
https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:119: {0, 0} // That's here as a
terminator
> for the loop. Do not remove!
> That should be obvious to most people now you use 0, 0. I would add the
comment,
> or just add // Do not remove the terminator line.

Done.

> 
>
https://codereview.chromium.org/186603002/diff/60001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:143: ASSERT_EQ((unsigned)0,
> output.length());
> static_cast instaed of C cast?

Removed that line entirely. (Using 0 as second string made it unnecessary)

[abarth-chromium, 2014-03-05 18:01:42]

https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
File Source/core/css/MediaQuerySetTest.cpp (right):

https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:13: #define OUTPUT_CHAR_ARRAY_LEN 256
Please use a constant instead of a #define.

https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:24: if (!str.impl())
if (str.isNull())

https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:128: output.append(queryText);
Please don't call String#append, even in tests.  We're going to remove that API
someday.  Instead, use StringBuilder.

https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQuerySetTest.cpp:141: ++i;
Why not use a for loop?

[Yoav Weiss, 2014-03-05 19:56:33]

On 2014/03/05 18:01:42, abarth wrote:
>
https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
> File Source/core/css/MediaQuerySetTest.cpp (right):
> 
>
https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:13: #define OUTPUT_CHAR_ARRAY_LEN 256
> Please use a constant instead of a #define.
> 
>
https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:24: if (!str.impl())
> if (str.isNull())
> 
>
https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:128: output.append(queryText);
> Please don't call String#append, even in tests.  We're going to remove that
API
> someday.  Instead, use StringBuilder.
> 
>
https://codereview.chromium.org/186603002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQuerySetTest.cpp:141: ++i;
> Why not use a for loop?

Fixed above comments and rebased

[Yoav Weiss, 2014-03-05 22:30:04]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-03-05 22:30:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/186603002/120001

[commit-bot: I haz the power, 2014-03-06 03:59:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 03:59:00]

Try jobs failed on following builders: win_blink_rel

[Yoav Weiss, 2014-03-06 07:51:12]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-03-06 07:52:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/186603002/120001

[commit-bot: I haz the power, 2014-03-06 09:28:45]

Change committed as 168606