Remove usage of CSSParserValue in MediaQueryParser

Remove usage of CSSParserValue in MediaQueryParser

Remove usage of CSSParserValue in MediaQueryParser by using
CSSParserToken everywhere. Also prefer to deal with const ref
rather than pointers.

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

[rwlbuis, 2015-06-15 17:53:50]

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

[rwlbuis, 2015-06-15 17:53:51]

PTAL.

- the static_assert is optional, but I think handy since we
don't want to increase CSSParserToken size while phasing
out CSSParserValue.
- only tricky thing was testing the token type before
calling numericValue to avoid ASSERTs.

[Yoav Weiss, 2015-06-15 21:46:26]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-06-15 21:46:27]

https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQu...
Source/core/css/MediaQueryExp.cpp:228: } else if (token.type() == NumberToken ||
token.type() == PercentageToken || token.type() == DimensionToken) {
Does this add a functional change?

[rwlbuis, 2015-06-15 22:00:53]

https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQu...
Source/core/css/MediaQueryExp.cpp:228: } else if (token.type() == NumberToken ||
token.type() == PercentageToken || token.type() == DimensionToken) {
On 2015/06/15 21:46:27, Yoav Weiss wrote:
> Does this add a functional change?

I think I need this since calling numericValue() ASSERTs for the above three
types.

[Yoav Weiss, 2015-06-15 22:15:18]

On 2015/06/15 22:00:53, rwlbuis wrote:
>
https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryExp.cpp (right):
> 
>
https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryExp.cpp:228: } else if (token.type() == NumberToken
||
> token.type() == PercentageToken || token.type() == DimensionToken) {
> On 2015/06/15 21:46:27, Yoav Weiss wrote:
> > Does this add a functional change?
> 
> I think I need this since calling numericValue() ASSERTs for the above three
> types.

OK. I think it'd be helpful to add a comment that explains that.
Do we have tests that exercise that code? Are you sure that all valid concerned
media features pass this test?

[rwlbuis, 2015-06-15 22:40:37]

On 2015/06/15 22:15:18, Yoav Weiss wrote:
>
https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQu...
> > Source/core/css/MediaQueryExp.cpp:228: } else if (token.type() ==
NumberToken
> ||
> > token.type() == PercentageToken || token.type() == DimensionToken) {
> > On 2015/06/15 21:46:27, Yoav Weiss wrote:
> > > Does this add a functional change?
> > 
> > I think I need this since calling numericValue() ASSERTs for the above three
> > types.
> 
> OK. I think it'd be helpful to add a comment that explains that.

Sure, I can add a comment.

> Do we have tests that exercise that code? Are you sure that all valid
concerned
> media features pass this test?

Yes, this code was needed when I noticed that MediaQueryEvaluaterTest.Dynamic
failed in the previous patch set.

[Timothy Loh, 2015-06-16 00:05:36]

https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/MediaQu...
Source/core/css/MediaQueryExp.cpp:230: if (token.type() == NumberToken ||
token.type() == PercentageToken || token.type() == DimensionToken) {
else if?

https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSParserToken.cpp:17: void* fields[3];
IIRC we're 20B on 32-bit and 24B on 64-bit.

[rwlbuis, 2015-06-16 02:34:15]

PTAL

https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/MediaQu...
Source/core/css/MediaQueryExp.cpp:230: if (token.type() == NumberToken ||
token.type() == PercentageToken || token.type() == DimensionToken) {
On 2015/06/16 00:05:35, Timothy Loh wrote:
> else if?

I was making some room for the comment :) Hope the new way is ok.

https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/parser/...
Source/core/css/parser/CSSParserToken.cpp:17: void* fields[3];
On 2015/06/16 00:05:36, Timothy Loh wrote:
> IIRC we're 20B on 32-bit and 24B on 64-bit.

Done.

[Timothy Loh, 2015-06-16 06:20:36]

lgtm

https://codereview.chromium.org/1176393005/diff/100001/Source/core/css/parser...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1176393005/diff/100001/Source/core/css/parser...
Source/core/css/parser/CSSParserToken.cpp:23:
static_assert(sizeof(CSSParserToken) == sizeof(SameSizeAsCSSParserToken),
"CSSParserToken should stay small");
Actually, can we do:

#if CPU(32BIT)
static_assert(.. == 20, ..)
#else
static_assert(.. == 24, ..)
#endif

Also I don't think this makes sense in this patch, just do this separately.

[rwlbuis, 2015-06-16 14:09:27]

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

[rwlbuis, 2015-06-16 14:09:27]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1176393005/#ps120001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-06-16 14:09:44]

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

[commit-bot: I haz the power, 2015-06-16 16:39:32]

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

[commit-bot: I haz the power, 2015-06-16 16:39:33]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66809)

[rwlbuis, 2015-06-16 16:54:18]

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

[commit-bot: I haz the power, 2015-06-16 16:54:44]

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

[commit-bot: I haz the power, 2015-06-16 19:16:01]

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

[commit-bot: I haz the power, 2015-06-16 19:16:02]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66833)

[rwlbuis, 2015-06-16 19:20:24]

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

[commit-bot: I haz the power, 2015-06-16 19:21:14]

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

[commit-bot: I haz the power, 2015-06-16 22:10:18]

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

[commit-bot: I haz the power, 2015-06-16 22:10:19]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66864)

[rwlbuis, 2015-06-16 22:17:26]

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

[commit-bot: I haz the power, 2015-06-16 22:18:08]

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

[commit-bot: I haz the power, 2015-06-17 00:58:31]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197219