Avoid use of CSSValue in MediaQueryExp and MediaQueryEvaluator

Avoid use of CSSValue in MediaQueryExp and MediaQueryEvaluator

Since the new MediaQueryParser should be thread-safe and CSSPrimitiveValue may not be, MediaQueryExp should not generate a CSSValue when created by MediaQueryParser and SizesAttributeParser. 
This CL fixes that.

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

[eseidel, 2014-04-22 16:22:23]

https://codereview.chromium.org/240453010/diff/80002/Source/core/css/CSSPrimi...
File Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/240453010/diff/80002/Source/core/css/CSSPrimi...
Source/core/css/CSSPrimitiveValue.h:216: static bool isDotsPerInch(unsigned
short type) { return type == CSS_DPI; }
I wonder if we should make an internal enum mapped to the external (CSSOM)
unsigned short values.  Would allow the compiler to do typechecking. :)

https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluator.cpp (right):

https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:253: if (value.set())
What is .set?  Normally that's a setter.  foo.set(bar);

https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
Source/core/css/MediaQueryExp.cpp:320: static String unitToString(unsigned short
unit)
I wonder if this doesn't belong on CSSPrimitiveValue itself?

https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
File Source/core/css/MediaQueryExp.h (right):

https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
Source/core/css/MediaQueryExp.h:54: MediaQueryExpValue()
Do you ever need to create one of these from a CSSPrimitiveValue?  Or create a
CSSPrimitiveValue from one (such that you mgiht want an operator?)

[Yoav Weiss, 2014-04-22 16:54:06]

On 2014/04/22 16:22:23, eseidel wrote:
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/CSSPrimi...
> File Source/core/css/CSSPrimitiveValue.h (right):
> 
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/CSSPrimi...
> Source/core/css/CSSPrimitiveValue.h:216: static bool isDotsPerInch(unsigned
> short type) { return type == CSS_DPI; }
> I wonder if we should make an internal enum mapped to the external (CSSOM)
> unsigned short values.  Would allow the compiler to do typechecking. :)

I'm not sure I get what you mean by that. Example?

> 
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryEvaluator.cpp (right):
> 
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:253: if (value.set())
> What is .set?  Normally that's a setter.  foo.set(bar);

Yeah, the name choice is not great. It's indicating that the value was
initialized and set with appropriate internal values.
Maybe isInit?

> 
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryExp.cpp (right):
> 
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> Source/core/css/MediaQueryExp.cpp:320: static String unitToString(unsigned
short
> unit)
> I wonder if this doesn't belong on CSSPrimitiveValue itself?

CSSPrimitiveValue::customCSSText contains a slightly similar logic. I'll try to
break it up, and reuse that logic from here.

> 
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryExp.h (right):
> 
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> Source/core/css/MediaQueryExp.h:54: MediaQueryExpValue()
> Do you ever need to create one of these from a CSSPrimitiveValue?  Or create a
> CSSPrimitiveValue from one (such that you mgiht want an operator?)

For now I don't need to create one from the other

[Yoav Weiss, 2014-04-23 08:22:28]

On 2014/04/22 16:54:06, Yoav Weiss wrote:
> On 2014/04/22 16:22:23, eseidel wrote:
> >
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/CSSPrimi...
> > File Source/core/css/CSSPrimitiveValue.h (right):
> > 
> >
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/CSSPrimi...
> > Source/core/css/CSSPrimitiveValue.h:216: static bool isDotsPerInch(unsigned
> > short type) { return type == CSS_DPI; }
> > I wonder if we should make an internal enum mapped to the external (CSSOM)
> > unsigned short values.  Would allow the compiler to do typechecking. :)
> 
> I'm not sure I get what you mean by that. Example?
> 
> > 
> >
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> > File Source/core/css/MediaQueryEvaluator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> > Source/core/css/MediaQueryEvaluator.cpp:253: if (value.set())
> > What is .set?  Normally that's a setter.  foo.set(bar);
> 
> Yeah, the name choice is not great. It's indicating that the value was
> initialized and set with appropriate internal values.
> Maybe isInit?
> 
> > 
> >
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> > File Source/core/css/MediaQueryExp.cpp (right):
> > 
> >
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> > Source/core/css/MediaQueryExp.cpp:320: static String unitToString(unsigned
> short
> > unit)
> > I wonder if this doesn't belong on CSSPrimitiveValue itself?
> 
> CSSPrimitiveValue::customCSSText contains a slightly similar logic. I'll try
to
> break it up, and reuse that logic from here.
> 
> > 
> >
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> > File Source/core/css/MediaQueryExp.h (right):
> > 
> >
>
https://codereview.chromium.org/240453010/diff/80002/Source/core/css/MediaQue...
> > Source/core/css/MediaQueryExp.h:54: MediaQueryExpValue()
> > Do you ever need to create one of these from a CSSPrimitiveValue?  Or create
a
> > CSSPrimitiveValue from one (such that you mgiht want an operator?)
> 
> For now I don't need to create one from the other

Fixed above comments

[eseidel, 2014-04-23 16:51:04]

https://codereview.chromium.org/240453010/diff/140001/Source/core/css/CSSPrim...
File Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/240453010/diff/140001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValue.cpp:1117: default:
Are you sure you want a default?  That will defeat the compiler warning about
missing cases for the enum.

https://codereview.chromium.org/240453010/diff/140001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryEvaluator.cpp (right):

https://codereview.chromium.org/240453010/diff/140001/Source/core/css/MediaQu...
Source/core/css/MediaQueryEvaluator.cpp:195: if (value.isInit())
What does isInit mean?

[Yoav Weiss, 2014-04-23 20:44:23]

On 2014/04/23 16:51:04, eseidel wrote:
>
https://codereview.chromium.org/240453010/diff/140001/Source/core/css/CSSPrim...
> File Source/core/css/CSSPrimitiveValue.cpp (right):
> 
>
https://codereview.chromium.org/240453010/diff/140001/Source/core/css/CSSPrim...
> Source/core/css/CSSPrimitiveValue.cpp:1117: default:
> Are you sure you want a default?  That will defeat the compiler warning about
> missing cases for the enum.
OK, I'll add all other cases, with an ASSERT_NOT_REACHED (since they're not
really supposed to happen) 
> 
>
https://codereview.chromium.org/240453010/diff/140001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryEvaluator.cpp (right):
> 
>
https://codereview.chromium.org/240453010/diff/140001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryEvaluator.cpp:195: if (value.isInit())
> What does isInit mean?

It means that the value was initialized with some values (either a number
value,an ID or a ratio).
I'll be happy to change it to a different name.

[Yoav Weiss, 2014-04-23 22:19:52]

On 2014/04/23 20:44:23, Yoav Weiss wrote:
> On 2014/04/23 16:51:04, eseidel wrote:
> >
>
https://codereview.chromium.org/240453010/diff/140001/Source/core/css/CSSPrim...
> > File Source/core/css/CSSPrimitiveValue.cpp (right):
> > 
> >
>
https://codereview.chromium.org/240453010/diff/140001/Source/core/css/CSSPrim...
> > Source/core/css/CSSPrimitiveValue.cpp:1117: default:
> > Are you sure you want a default?  That will defeat the compiler warning
about
> > missing cases for the enum.
> OK, I'll add all other cases, with an ASSERT_NOT_REACHED (since they're not
> really supposed to happen) 

Done
> > 
> >
>
https://codereview.chromium.org/240453010/diff/140001/Source/core/css/MediaQu...
> > File Source/core/css/MediaQueryEvaluator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/240453010/diff/140001/Source/core/css/MediaQu...
> > Source/core/css/MediaQueryEvaluator.cpp:195: if (value.isInit())
> > What does isInit mean?
> 
> It means that the value was initialized with some values (either a number
> value,an ID or a ratio).
> I'll be happy to change it to a different name.

[eseidel, 2014-04-24 00:23:59]

https://codereview.chromium.org/240453010/diff/160001/Source/core/css/CSSPrim...
File Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/240453010/diff/160001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValue.cpp:1076: return String("em");
You should just be able to write "em", i believe String has an implicit
constructor.  Then again, you could also have this function return const char*
as these are compiler constants.  Then you would avoid the malloc.

https://codereview.chromium.org/240453010/diff/160001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryExp.h (right):

https://codereview.chromium.org/240453010/diff/160001/Source/core/css/MediaQu...
Source/core/css/MediaQueryExp.h:67: bool isInit() const { return (isID ||
isValue || isRatio); }
isValid?

[Yoav Weiss, 2014-04-24 05:05:30]

On 2014/04/24 00:23:59, eseidel wrote:
>
https://codereview.chromium.org/240453010/diff/160001/Source/core/css/CSSPrim...
> File Source/core/css/CSSPrimitiveValue.cpp (right):
> 
>
https://codereview.chromium.org/240453010/diff/160001/Source/core/css/CSSPrim...
> Source/core/css/CSSPrimitiveValue.cpp:1076: return String("em");
> You should just be able to write "em", i believe String has an implicit
> constructor.  Then again, you could also have this function return const char*
> as these are compiler constants.  Then you would avoid the malloc.

Returned as const char* to avoid a malloc. Couldn't avoid an strlen though.
(The previous template that concluded the const char[]'s number of characters
can't conclude it from a const char*)
> 
>
https://codereview.chromium.org/240453010/diff/160001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryExp.h (right):
> 
>
https://codereview.chromium.org/240453010/diff/160001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryExp.h:67: bool isInit() const { return (isID ||
> isValue || isRatio); }
> isValid?
isValid it is! :)

[eseidel, 2014-04-24 05:32:22]

lgtm

[commit-bot: I haz the power, 2014-04-24 05:32:37]

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

[commit-bot: I haz the power, 2014-04-24 06:13:23]

Change committed as 172457