Make media query feature values more strict

Make media query feature values more strict

Before this patch MediaQueryData::addParserValue would
accept any kind of css token, including url() and '#' types.
With this patch addParserValue becomes more restrictive and
only allows number, percentage, dimension, delimiter and
ident token types. To detect when some other token type
is used, make it return a bool and reflect this by renaming
to tryAddParserValue.

Behavior matches Firefox.

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

[rwlbuis, 2015-01-07 22:15:49]

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

[rwlbuis, 2015-01-07 22:15:49]

PTAL

[Timothy Loh, 2015-01-07 23:05:26]

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

[Timothy Loh, 2015-01-07 23:05:27]

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

https://codereview.chromium.org/836373002/diff/20001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:143: m_state = ReadFeatureEnd;
I'm not sure this works; doesn't this mean that if we have (say) an at-keyword
token here, we'd just skip over it and not do any state transition? So
(orientation: @junk portrait) would be accepted?

[rwlbuis, 2015-01-08 01:03:08]

On 2015/01/07 23:05:27, Timothy Loh wrote:
>
https://codereview.chromium.org/836373002/diff/20001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryParser.cpp (right):
> 
>
https://codereview.chromium.org/836373002/diff/20001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.cpp:143: m_state = ReadFeatureEnd;
> I'm not sure this works; doesn't this mean that if we have (say) an at-keyword
> token here, we'd just skip over it and not do any state transition? So
> (orientation: @junk portrait) would be accepted?

You are right, I think I fixed it now in patch set 3. But it would also be good
if Yoav can have a look.

[Yoav Weiss, 2015-01-08 08:06:20]

On 2015/01/08 01:03:08, rwlbuis wrote:
> On 2015/01/07 23:05:27, Timothy Loh wrote:
> >
>
https://codereview.chromium.org/836373002/diff/20001/Source/core/css/parser/M...
> > File Source/core/css/parser/MediaQueryParser.cpp (right):
> > 
> >
>
https://codereview.chromium.org/836373002/diff/20001/Source/core/css/parser/M...
> > Source/core/css/parser/MediaQueryParser.cpp:143: m_state = ReadFeatureEnd;
> > I'm not sure this works; doesn't this mean that if we have (say) an
at-keyword
> > token here, we'd just skip over it and not do any state transition? So
> > (orientation: @junk portrait) would be accepted?
> 
> You are right, I think I fixed it now in patch set 3. But it would also be
good
> if Yoav can have a look.

Looks good! :)

I'd love to see some more tests, e.g:
screen and (orientation: @portrait) and (max-width: 1000px) => false
screen and (orientation: @portrait), (max-width: 1000px) => true

[rwlbuis, 2015-01-08 16:32:51]

On 2015/01/08 08:06:20, Yoav Weiss wrote:
> On 2015/01/08 01:03:08, rwlbuis wrote:
> > On 2015/01/07 23:05:27, Timothy Loh wrote:
> > >
> >
>
https://codereview.chromium.org/836373002/diff/20001/Source/core/css/parser/M...
> > > File Source/core/css/parser/MediaQueryParser.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/836373002/diff/20001/Source/core/css/parser/M...
> > > Source/core/css/parser/MediaQueryParser.cpp:143: m_state = ReadFeatureEnd;
> > > I'm not sure this works; doesn't this mean that if we have (say) an
> at-keyword
> > > token here, we'd just skip over it and not do any state transition? So
> > > (orientation: @junk portrait) would be accepted?
> > 
> > You are right, I think I fixed it now in patch set 3. But it would also be
> good
> > if Yoav can have a look.
> 
> Looks good! :)

Thanks!

> I'd love to see some more tests, e.g:
> screen and (orientation: @portrait) and (max-width: 1000px) => false
> screen and (orientation: @portrait), (max-width: 1000px) => true

I added these tests. I think it is ready for proper review now.

[Timothy Loh, 2015-01-08 22:16:29]

lgtm, this looks correct to me

[Yoav Weiss, 2015-01-09 08:48:34]

On 2015/01/08 22:16:29, Timothy Loh wrote:
> lgtm, this looks correct to me

LGTM2 :)

[rwlbuis, 2015-01-09 13:50:23]

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

[commit-bot: I haz the power, 2015-01-09 13:51:23]

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

[commit-bot: I haz the power, 2015-01-09 15:45:42]

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