A sizes attribute parser

A sizes attribute parser

This CL implements a parser for HTMLImageElement's sizes attribute.
It also hooks that parser to HTMLImageElement, but doesn't yet do anything with the output.

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

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

[Yoav Weiss, 2014-04-05 20:05:52]

This CL start adding the sizes parser implementation. It is not yet very useful
(critical parts are not yet implemented).
It is used by HTMLImageElement when the sizes attribute is present (and the
runtime flag is on), but it doesn't yet have any impact on resource loading.

[eseidel, 2014-04-08 06:02:14]

In general seems fine.

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/MediaVal...
File Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/MediaVal...
Source/core/css/MediaValues.cpp:198: const Document* executingDocument =
document.import() ? document.import()->master() : &document;
const pointers to ref-counted objects rarely make sense given that ref() is
non-const.

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
File Source/core/css/parser/SizesAttributeParser.cpp (right):

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.cpp:13: enum {
EffectiveSizeDefaultValue = 300 };
I know you were encouraged to do these as Enums before.  I'm not really sure
why.  I probably would have used const int, but this is also OK just feels odd
to me.  I wish we had the reasoning one way or the other documented in our style
guide...

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.cpp:21: if
(!parser.parseImpl(tokens))
Why parseImpl and not just parse?

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.cpp:32: static void
goBackWhileWhitespaceToken(TokenIterator& token, TokenIterator startToken)
Other parser have used:

template<type UntilFunction>
goBackUntil<UntilFunction>()...

I guess it depends on if you'll need others of these.

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.cpp:48: MediaQueryEvaluator
mediaQueryEvaluator("screen", m_mediaValues);
IIRC you have some StaticStrings for media types, like screen?

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.cpp:57:
goBackWhileWhitespaceToken(endToken, startToken);
I see, so these would be "reverseFind" in a string, but we have a token stream
instead, right?

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
File Source/core/css/parser/SizesAttributeParser.h (right):

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.h:19:
SizesAttributeParser(PassRefPtr<MediaValues> mediaValues)
Feels a bit odd for this to be private, but OK.  I guess you don't want to make
it possible for folks to call functions on it twice?

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.h:26: bool
parseMediaConditionAndLength(TokenIterator startToken, TokenIterator endToken);
Should these be const&?

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.h:33: int m_length;
Do you want these to be ints or unsigned?

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
File Source/core/css/parser/SizesAttributeParserTest.cpp (right):

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParserTest.cpp:31: RefPtr<MediaValues>
mediaValues = MediaValues::create(MediaValues::CachingMode,
Wow, this is bad.  Instead we should use a struct:

MediaValuesArgs args;
args.viewportWidth = 500;
args.viewportHeight = 500;
args.deviceWidth = 500;

or maybe just MediaValues itself should be a struct?

https://codereview.chromium.org/224733011/diff/40001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/224733011/diff/40001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.h:133: int m_effectiveSize;
int or unsigned?

[rune, 2014-04-08 10:34:41]

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
File Source/core/css/parser/SizesAttributeParser.cpp (right):

https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
Source/core/css/parser/SizesAttributeParser.cpp:13: enum {
EffectiveSizeDefaultValue = 300 };
On 2014/04/08 06:02:14, eseidel wrote:
> I know you were encouraged to do these as Enums before.  I'm not really sure
> why.  I probably would have used const int, but this is also OK just feels odd
> to me.  I wish we had the reasoning one way or the other documented in our
style
> guide...

That would have been me. I based it on how it's mostly done elsewhere in Blink.
const int members use memory, or?

[rune, 2014-04-08 10:42:10]

> That would have been me. I based it on how it's mostly done elsewhere in
Blink.
> const int members use memory, or?

I'm probably wrong about "mostly". It's a good mix, at least.

[Yoav Weiss, 2014-04-08 11:18:26]

On second thought, I think it'd be better to land this CL only after
https://codereview.chromium.org/227043007/ and
https://codereview.chromium.org/225293006/ have landed, and make it more feature
complete.

On 2014/04/08 06:02:14, eseidel wrote:
> In general seems fine.
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/MediaVal...
> File Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/MediaVal...
> Source/core/css/MediaValues.cpp:198: const Document* executingDocument =
> document.import() ? document.import()->master() : &document;
> const pointers to ref-counted objects rarely make sense given that ref() is
> non-const.

Fixed (locally)

> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> File Source/core/css/parser/SizesAttributeParser.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParser.cpp:13: enum {
> EffectiveSizeDefaultValue = 300 };
> I know you were encouraged to do these as Enums before.  I'm not really sure
> why.  I probably would have used const int, but this is also OK just feels odd
> to me.  I wish we had the reasoning one way or the other documented in our
style
> guide...
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParser.cpp:21: if
> (!parser.parseImpl(tokens))
> Why parseImpl and not just parse?

Fixed (locally).
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParser.cpp:32: static void
> goBackWhileWhitespaceToken(TokenIterator& token, TokenIterator startToken)
> Other parser have used:
> 
> template<type UntilFunction>
> goBackUntil<UntilFunction>()...
> 
> I guess it depends on if you'll need others of these.
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParser.cpp:48: MediaQueryEvaluator
> mediaQueryEvaluator("screen", m_mediaValues);
> IIRC you have some StaticStrings for media types, like screen?

Fixed (locally).
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParser.cpp:57:
> goBackWhileWhitespaceToken(endToken, startToken);
> I see, so these would be "reverseFind" in a string, but we have a token stream
> instead, right?

Yes. I need to fix the logic here, since the Whitespace based algorithm I used
here is flawed. The spec is being amended to include a step based algorithm
here, and I'd use that algorithm instead of this one.

> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> File Source/core/css/parser/SizesAttributeParser.h (right):
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParser.h:19:
> SizesAttributeParser(PassRefPtr<MediaValues> mediaValues)
> Feels a bit odd for this to be private, but OK.  I guess you don't want to
make
> it possible for folks to call functions on it twice?
Yeah, calling parse on it twice is not something I planned for.

> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParser.h:26: bool
> parseMediaConditionAndLength(TokenIterator startToken, TokenIterator
endToken);
> Should these be const&?
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParser.h:33: int m_length;
> Do you want these to be ints or unsigned?

I'll change these to unsigned.
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> File Source/core/css/parser/SizesAttributeParserTest.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/css/parser/S...
> Source/core/css/parser/SizesAttributeParserTest.cpp:31: RefPtr<MediaValues>
> mediaValues = MediaValues::create(MediaValues::CachingMode,
> Wow, this is bad.  Instead we should use a struct:
> 
> MediaValuesArgs args;
> args.viewportWidth = 500;
> args.viewportHeight = 500;
> args.deviceWidth = 500;
> 
> or maybe just MediaValues itself should be a struct?

This is only used in tests, but I'll look into ways of making MediaValues
initialization less painful. It'd probably be a part of a separate CL.
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/html/HTMLIma...
> File Source/core/html/HTMLImageElement.h (right):
> 
>
https://codereview.chromium.org/224733011/diff/40001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.h:133: int m_effectiveSize;
> int or unsigned?

I'll upload a new patchset once the above CLs have landed, and once I've done
modifying the "reverse find" algorithm.

[Yoav Weiss, 2014-04-08 16:00:00]

On 2014/04/08 11:18:26, Yoav Weiss wrote:
> On second thought, I think it'd be better to land this CL only after
> https://codereview.chromium.org/227043007/ and
> https://codereview.chromium.org/225293006/ have landed, and make it more
feature
> complete.
> 
[snip]
> 
> I'll upload a new patchset once the above CLs have landed, and once I've done
> modifying the "reverse find" algorithm.

I've uploaded a patchset that relies on all these CLs, and is fully functional
(modulo the "calc()" implementation).
It might be easier to review once the CLs it depends on have landed, so it'd be
better to review and land them first.

[Yoav Weiss, 2014-04-11 13:23:45]

On 2014/04/08 16:00:00, Yoav Weiss wrote:
> On 2014/04/08 11:18:26, Yoav Weiss wrote:
> > On second thought, I think it'd be better to land this CL only after
> > https://codereview.chromium.org/227043007/ and
> > https://codereview.chromium.org/225293006/ have landed, and make it more
> feature
> > complete.
> > 
> [snip]
> > 
> > I'll upload a new patchset once the above CLs have landed, and once I've
done
> > modifying the "reverse find" algorithm.
> 
> I've uploaded a patchset that relies on all these CLs, and is fully functional
> (modulo the "calc()" implementation).
> It might be easier to review once the CLs it depends on have landed, so it'd
be
> better to review and land them first.

I've refactored the MediaValues class according to comment on this CL and
others.
I also hooked up the SizesParser to length calculations based on the cached
values in mediaValues, and extended the length calculations to cover many the
units that CSSSPrimitiveValue covers.

[eseidel, 2014-04-14 22:55:31]

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

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValue.cpp:598: // The logic in this function is
duplicated in MediaValues::computeLength for performance reasons.
Is MediaValues performance sensitive?

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryEvaluatorTest.cpp (right):

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaQu...
Source/core/css/MediaQueryEvaluatorTest.cpp:73:
MediaValues::MediaValuesInitializer initializer;
This is sooooo much easier to read /less error prone, thank you.

[eseidel, 2014-04-14 23:10:12]

Overall this looks good!

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
File Source/core/css/MediaValues.h (right):

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValues.h:55: static PassRefPtr<MediaValues>
create(PassRefPtr<LocalFrame>, PassRefPtr<RenderStyle>, MediaValuesMode);
Should this be on the base class or the subclass?

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValues.h:56: static PassRefPtr<MediaValues>
createCached(MediaValuesInitializer&);
Maybe this should move to MediaValuesCached::create?

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValues.h:59: virtual PassRefPtr<MediaValues> copy() const =
0;
Does this create a cached copy or a dynamic one?

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValues.h:60: virtual bool isSafeToSendToAnotherThread()
const = 0;
This is always false for MediaValuesDynamic, no?

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
File Source/core/css/MediaValuesCached.cpp (right):

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValuesCached.cpp:100: // The logic here is duplicated from
CSSPrimitiveValue::computeLengthDouble for performance reasons.
Why?  Can't we share code?  Have them both call a shared function?

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
File Source/core/css/MediaValuesCached.h (right):

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValuesCached.h:12: class MediaValuesCached : public
MediaValues {
Please mark the class as FINAL.

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValuesCached.h:17: virtual bool
isSafeToSendToAnotherThread() const;
These are all OVERRIDE.

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
File Source/core/css/MediaValuesDynamic.h (right):

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValuesDynamic.h:14: class MediaValuesDynamic : public
MediaValues {
You should mark this class as FINAL.

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
Source/core/css/MediaValuesDynamic.h:21: virtual int viewportWidth() const;
These are all OVERRIDE.

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParser.cpp (right):

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.cpp:28: if (type == NumberToken ||
type == PercentageToken || type == DimensionToken) {
Would this be clearer returning early instead of a long if block?

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.cpp:29: int length;
Can this be negative?

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.cpp:30: RefPtr<CSSPrimitiveValue>
primitiveValue = CSSPrimitiveValue::create(startToken->numericValue(),
startToken->unitType());
Presumably this only ever runs on the main thread?  I'm not sure if
CSSPrimitiveValue is safe to use from multipel threads.

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParser.h (right):

https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.h:33: RefPtr<MediaValues>
m_mediaValues;
Are these always cached or dynamic?  Can this be a more specific type?

[Yoav Weiss, 2014-04-15 05:53:04]

On 2014/04/14 22:55:31, eseidel wrote:
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/CSSPrim...
> File Source/core/css/CSSPrimitiveValue.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/CSSPrim...
> Source/core/css/CSSPrimitiveValue.cpp:598: // The logic in this function is
> duplicated in MediaValues::computeLength for performance reasons.
> Is MediaValues performance sensitive?

MediaValues is not performance sensitive, but feedback from esprehn said that
CSSPrimitiveValue::computeLength is extremely hot, and adding virtual function
calls to it (which was the alternative approach to duplicating the logic), is
very likely to have a negative performance impact. 
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryEvaluatorTest.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryEvaluatorTest.cpp:73:
> MediaValues::MediaValuesInitializer initializer;
> This is sooooo much easier to read /less error prone, thank you.

[Yoav Weiss, 2014-04-15 06:11:26]

On 2014/04/14 23:10:12, eseidel wrote:
> Overall this looks good!
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> File Source/core/css/MediaValues.h (right):
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.h:55: static PassRefPtr<MediaValues>
> create(PassRefPtr<LocalFrame>, PassRefPtr<RenderStyle>, MediaValuesMode);
> Should this be on the base class or the subclass?
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.h:56: static PassRefPtr<MediaValues>
> createCached(MediaValuesInitializer&);
> Maybe this should move to MediaValuesCached::create?

Yeah, I'll move the create() functions to the sub-classes.
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.h:59: virtual PassRefPtr<MediaValues> copy() const
=
> 0;
> Does this create a cached copy or a dynamic one?
It creates a cached copy for the Cached variant and a dynamic copy for the
Dynamic one.

> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.h:60: virtual bool isSafeToSendToAnotherThread()
> const = 0;
> This is always false for MediaValuesDynamic, no?

Yeah.

> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> File Source/core/css/MediaValuesCached.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValuesCached.cpp:100: // The logic here is duplicated
from
> CSSPrimitiveValue::computeLengthDouble for performance reasons.
> Why?  Can't we share code?  Have them both call a shared function?

According to esprehn adding virtual calls here will be harmful, and making this
function templated is complicated because of virtual functions for CSSCalc.
Maybe we can try to re-unify that logic into a single function in a separate CL?

> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> File Source/core/css/MediaValuesCached.h (right):
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValuesCached.h:12: class MediaValuesCached : public
> MediaValues {
> Please mark the class as FINAL.
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValuesCached.h:17: virtual bool
> isSafeToSendToAnotherThread() const;
> These are all OVERRIDE.
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> File Source/core/css/MediaValuesDynamic.h (right):
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValuesDynamic.h:14: class MediaValuesDynamic : public
> MediaValues {
> You should mark this class as FINAL.
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> Source/core/css/MediaValuesDynamic.h:21: virtual int viewportWidth() const;
> These are all OVERRIDE.

Will do.

> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> File Source/core/css/parser/SizesAttributeParser.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.cpp:28: if (type == NumberToken ||
> type == PercentageToken || type == DimensionToken) {
> Would this be clearer returning early instead of a long if block?

Probably.

> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.cpp:29: int length;
> Can this be negative?

computeLength can return a negative length.

> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.cpp:30: RefPtr<CSSPrimitiveValue>
> primitiveValue = CSSPrimitiveValue::create(startToken->numericValue(),
> startToken->unitType());
> Presumably this only ever runs on the main thread?  I'm not sure if
> CSSPrimitiveValue is safe to use from multipel threads.

This also runs off main thread. I didn't see something particular about
CSSPrimitiveValue's creation that isn't thread safe, but I probably missed it.
I can avoid creating CSSPrimitiveValue here by calculating isLength in some
alternative way.

> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> File Source/core/css/parser/SizesAttributeParser.h (right):
> 
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.h:33: RefPtr<MediaValues>
> m_mediaValues;
> Are these always cached or dynamic?  Can this be a more specific type?
These can be both cached (when called from HTMLPreloadScanner) and dynamic (when
called from HTMLImageElement)

[Yoav Weiss, 2014-04-15 09:34:35]

On 2014/04/15 06:11:26, Yoav Weiss wrote:
> On 2014/04/14 23:10:12, eseidel wrote:
> > Overall this looks good!
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > File Source/core/css/MediaValues.h (right):
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValues.h:55: static PassRefPtr<MediaValues>
> > create(PassRefPtr<LocalFrame>, PassRefPtr<RenderStyle>, MediaValuesMode);
> > Should this be on the base class or the subclass?
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValues.h:56: static PassRefPtr<MediaValues>
> > createCached(MediaValuesInitializer&);
> > Maybe this should move to MediaValuesCached::create?
> 
> Yeah, I'll move the create() functions to the sub-classes.
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValues.h:59: virtual PassRefPtr<MediaValues> copy()
const
> =
> > 0;
> > Does this create a cached copy or a dynamic one?
> It creates a cached copy for the Cached variant and a dynamic copy for the
> Dynamic one.
> 
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValues.h:60: virtual bool isSafeToSendToAnotherThread()
> > const = 0;
> > This is always false for MediaValuesDynamic, no?
> 
> Yeah.
> 
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > File Source/core/css/MediaValuesCached.cpp (right):
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValuesCached.cpp:100: // The logic here is duplicated
> from
> > CSSPrimitiveValue::computeLengthDouble for performance reasons.
> > Why?  Can't we share code?  Have them both call a shared function?
> 
> According to esprehn adding virtual calls here will be harmful, and making
this
> function templated is complicated because of virtual functions for CSSCalc.
> Maybe we can try to re-unify that logic into a single function in a separate
CL?
> 
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > File Source/core/css/MediaValuesCached.h (right):
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValuesCached.h:12: class MediaValuesCached : public
> > MediaValues {
> > Please mark the class as FINAL.
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValuesCached.h:17: virtual bool
> > isSafeToSendToAnotherThread() const;
> > These are all OVERRIDE.
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > File Source/core/css/MediaValuesDynamic.h (right):
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValuesDynamic.h:14: class MediaValuesDynamic : public
> > MediaValues {
> > You should mark this class as FINAL.
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/MediaVa...
> > Source/core/css/MediaValuesDynamic.h:21: virtual int viewportWidth() const;
> > These are all OVERRIDE.
> 
> Will do.
> 
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> > File Source/core/css/parser/SizesAttributeParser.cpp (right):
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> > Source/core/css/parser/SizesAttributeParser.cpp:28: if (type == NumberToken
||
> > type == PercentageToken || type == DimensionToken) {
> > Would this be clearer returning early instead of a long if block?
> 
> Probably.
> 
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> > Source/core/css/parser/SizesAttributeParser.cpp:29: int length;
> > Can this be negative?
> 
> computeLength can return a negative length.
> 
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> > Source/core/css/parser/SizesAttributeParser.cpp:30:
RefPtr<CSSPrimitiveValue>
> > primitiveValue = CSSPrimitiveValue::create(startToken->numericValue(),
> > startToken->unitType());
> > Presumably this only ever runs on the main thread?  I'm not sure if
> > CSSPrimitiveValue is safe to use from multipel threads.
> 
> This also runs off main thread. I didn't see something particular about
> CSSPrimitiveValue's creation that isn't thread safe, but I probably missed it.
> I can avoid creating CSSPrimitiveValue here by calculating isLength in some
> alternative way.
> 
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> > File Source/core/css/parser/SizesAttributeParser.h (right):
> > 
> >
>
https://codereview.chromium.org/224733011/diff/210001/Source/core/css/parser/...
> > Source/core/css/parser/SizesAttributeParser.h:33: RefPtr<MediaValues>
> > m_mediaValues;
> > Are these always cached or dynamic?  Can this be a more specific type?
> These can be both cached (when called from HTMLPreloadScanner) and dynamic
(when
> called from HTMLImageElement)

I've fixed the above comments.
I didn't try to further unify the computeLength logic between MediaValues and
CSSPrimitiveValue. I'll try to do that in a follow-up CL, if that's OK. (It'd be
easier to add reviewers, test for perf regressions, etc). I've added a fixme.
One more thing, if CSSPrimitiveValue is not thread safe, its creation should be
avoided in MediaQueryExp as well. Currently it is not the case. I've added a
fixme here, and will address this in a separate CL.

[eseidel, 2014-04-15 18:47:02]

lgtm

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

https://codereview.chromium.org/224733011/diff/240001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValue.cpp:598: // The logic in this function is
duplicated in MediaValues::computeLength for performance reasons.
"for performance reasons" doesn't give the reader any actionable knowledge. 
Better would be something like:
// The logic in this function is duplicated in MediaValues::computeLength
because MediaValues::computeLength
// needs nearly identical logic, but we haven't found a way to make
CSSPrimitiveValue::computeLengthDouble
// more generic (to solve both cases) without hurting performance.

i don't know if what I wrote above is true. :)  But hopefully you do.

https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryEvaluatorTest.cpp (right):

https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaQu...
Source/core/css/MediaQueryEvaluatorTest.cpp:73:
MediaValues::MediaValuesInitializer initializer;
Basically this is the storage for MediaValuesCached, and MediaValuesCached is
just the wrapper around this storage.  I might even consider making
MediaValuesCache just use one of these as m_data, woudl certainly simplify your
construction. :)

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

https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaQu...
Source/core/css/MediaQueryExp.cpp:218: // FIXME - Creation of CSSValue here may
not be thread safe.
I guess we can't turn this on until we fix this?

https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaVa...
File Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaVa...
Source/core/css/MediaValues.cpp:93: int
MediaValues::calculateDefaultFontSize(RenderStyle* style) const
A bit odd that these are in MediaValues instead of MediaValuesDynamic, but OK.

https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaVa...
File Source/core/css/MediaValuesDynamic.h (right):

https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaVa...
Source/core/css/MediaValuesDynamic.h:41: RefPtr<RenderStyle> m_style;
I suspect we could have answered Elliot's concern with a slightly simpler
design.

MediaValues would have two OwnPtrs.  One which points to MediaValuesCached, and
the other which points to MediaValuesDyanmic.  It can never have both at the
same time (RELEASE_ASSERT) and MediaValuesDynamic has
RELEASE_ASSERT(isMainThread) in its' constructor.  Then all your accessors look
like this:

if (m_dynamic)
   reutrn m_dynamic->foo();
return m_cached->foo();

All the calculate logic, etc. would just live on MediaValuesDynamic, etc.

But I think this design, making the virtual dispatch explicit is also fine. 
This is what virtual dispatch is for after all. :)

[Yoav Weiss, 2014-04-15 19:45:11]

On 2014/04/15 18:47:02, eseidel wrote:
> lgtm
> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/CSSPrim...
> File Source/core/css/CSSPrimitiveValue.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/CSSPrim...
> Source/core/css/CSSPrimitiveValue.cpp:598: // The logic in this function is
> duplicated in MediaValues::computeLength for performance reasons.
> "for performance reasons" doesn't give the reader any actionable knowledge. 
> Better would be something like:
> // The logic in this function is duplicated in MediaValues::computeLength
> because MediaValues::computeLength
> // needs nearly identical logic, but we haven't found a way to make
> CSSPrimitiveValue::computeLengthDouble
> // more generic (to solve both cases) without hurting performance.
> 
> i don't know if what I wrote above is true. :)  But hopefully you do.
It is true :)

> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryEvaluatorTest.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryEvaluatorTest.cpp:73:
> MediaValues::MediaValuesInitializer initializer;
> Basically this is the storage for MediaValuesCached, and MediaValuesCached is
> just the wrapper around this storage.  I might even consider making
> MediaValuesCache just use one of these as m_data, woudl certainly simplify
your
> construction. :)
Changed it. Much nicer. Thanks! :)
> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryExp.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryExp.cpp:218: // FIXME - Creation of CSSValue here
may
> not be thread safe.
> I guess we can't turn this on until we fix this?
It's not related to this CL, but it does impact the MQ parser.
I hope to have a fix for that tonight/tomorrow.

> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaVa...
> File Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.cpp:93: int
> MediaValues::calculateDefaultFontSize(RenderStyle* style) const
> A bit odd that these are in MediaValues instead of MediaValuesDynamic, but OK.
They're used inside MediaValuesCached when it's initialized from a style/frame
(to get the values and cached them).
> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaVa...
> File Source/core/css/MediaValuesDynamic.h (right):
> 
>
https://codereview.chromium.org/224733011/diff/240001/Source/core/css/MediaVa...
> Source/core/css/MediaValuesDynamic.h:41: RefPtr<RenderStyle> m_style;
> I suspect we could have answered Elliot's concern with a slightly simpler
> design.
> 
> MediaValues would have two OwnPtrs.  One which points to MediaValuesCached,
and
> the other which points to MediaValuesDyanmic.  It can never have both at the
> same time (RELEASE_ASSERT) and MediaValuesDynamic has
> RELEASE_ASSERT(isMainThread) in its' constructor.  Then all your accessors
look
> like this:
> 
> if (m_dynamic)
>    reutrn m_dynamic->foo();
> return m_cached->foo();
> 
> All the calculate logic, etc. would just live on MediaValuesDynamic, etc.
> 
> But I think this design, making the virtual dispatch explicit is also fine. 
> This is what virtual dispatch is for after all. :)

[Yoav Weiss, 2014-04-15 20:06:44]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-15 20:06:59]

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

[commit-bot: I haz the power, 2014-04-15 20:47:01]

Change committed as 171701

[hayato, 2014-04-16 05:24:30]

[blink gardening]
SystemMemoryApiTest.Memory started to fail.

http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/35970

I manually bisected in range 171674: 171705.
After r171701, SystemMemoryApiTest.Memory started to fail.

I suspect this patch might be also the cause of the failures of other tests,
such as Storage, StorageAttachment and so on.

Let me revert this.

[Yoav Weiss, 2014-04-16 05:47:14]

On 2014/04/16 05:24:30, hayato wrote:
> [blink gardening]
> SystemMemoryApiTest.Memory started to fail.
> 
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/35970
> 
> I manually bisected in range 171674: 171705.
> After r171701, SystemMemoryApiTest.Memory started to fail.
> 
> I suspect this patch might be also the cause of the failures of other tests,
> such as Storage, StorageAttachment and so on.
> 
> Let me revert this.

I'm not familiar with these test bots. What can I do to fix it? How can I test
for possible issues?

[hayato, 2014-04-16 07:24:10]

I reverted at:
https://codereview.chromium.org/239863003/
https://codereview.chromium.org/240063003/


In my local linux machine, these reverts fixed the following tests, which
started to fail as reported in
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/35970.

browser_tests
- Memory
- Storage
- StorageAttachment

interactive_ui_tests:
- Referrer
- SearchReusesInstantTab
- TypedSearchURLDoesntReuseInstantTab

(These list might not be complete).


I tested these in my local linux machine using:
 ./out/Release/browser_tests --gtest_filter='SystemMemoryApiTest.Memory'

[esprehn, 2014-04-16 19:35:36]

This patch was so big, can we try to land it in parts next time? :)

[Yoav Weiss, 2014-04-16 21:53:21]

On 2014/04/16 19:35:36, esprehn wrote:
> This patch was so big, can we try to land it in parts next time? :)

Breaking this patch into different pieces now would be fairly complex. I prefer
to land it as is (without the fontMetrics parts that caused the test issues), if
that's OK. I'll try to keep my CLs small in the future :)

To keep a track of my discoveries from investigating this issue:
Trying to get the fontMetrics from the HTMLDocumentParser (which
MediaValuesCached used to do when initialized), results in accessing the
fontCache, then the FontPlatformData, and eventually it calls
getRenderStyleForStrike no the chrome impl side.
I'm not sure what on the chrome side is triggering the "webthread_impl.cc(40)]
Check failed: isCurrentThread()." error seen on the SystemMemoryApiTest, though.

I've ignored this issue in the re-uploaded CL by avoiding reading these values,
and dropping support for accurate length computation of 'ch' and 'ex' units, as
a result.
I'll further investigate it when trying to activate accurate 'ex' and 'ch'
calculations.

[Yoav Weiss, 2014-04-17 04:20:07]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-17 04:20:17]

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

[commit-bot: I haz the power, 2014-04-17 04:20:29]

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

[commit-bot: I haz the power, 2014-04-17 04:20:29]

Failed to apply patch for Source/core/css/MediaValues.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/css/MediaValues.cpp
  Hunk #2 FAILED at 23.
  Hunk #8 FAILED at 136.
  2 out of 8 hunks FAILED -- saving rejects to file
Source/core/css/MediaValues.cpp.rej

Patch:       Source/core/css/MediaValues.cpp
Index: Source/core/css/MediaValues.cpp
diff --git a/Source/core/css/MediaValues.cpp b/Source/core/css/MediaValues.cpp
index
1291d4e51a34e70d3dd607b0a9a9f28d9ae40516..999c260319e30ef370a48b4ec213d75af2284fee
100644
--- a/Source/core/css/MediaValues.cpp
+++ b/Source/core/css/MediaValues.cpp
@@ -5,6 +5,8 @@
 #include "config.h"
 #include "core/css/MediaValues.h"
 
+#include "core/css/MediaValuesCached.h"
+#include "core/css/MediaValuesDynamic.h"
 #include "core/dom/Document.h"
 #include "core/dom/Element.h"
 #include "core/frame/FrameHost.h"
@@ -21,21 +23,29 @@
 
 namespace WebCore {
 
-static int calculateViewportWidth(LocalFrame* frame, RenderStyle* style)
+Document* MediaValues::getExecutingDocument(Document& document)
+{
+    Document* executingDocument = document.importsController() ?
document.importsController()->master() : &document;
+    ASSERT(executingDocument);
+    ASSERT(executingDocument->renderer());
+    return executingDocument;
+}
+
+int MediaValues::calculateViewportWidth(LocalFrame* frame, RenderStyle* style)
const
 {
     ASSERT(frame && frame->view() && style);
     int viewportWidth = frame->view()->layoutSize(IncludeScrollbars).width();
     return adjustForAbsoluteZoom(viewportWidth, style);
 }
 
-static int calculateViewportHeight(LocalFrame* frame, RenderStyle* style)
+int MediaValues::calculateViewportHeight(LocalFrame* frame, RenderStyle* style)
const
 {
     ASSERT(frame && frame->view() && style);
     int viewportHeight = frame->view()->layoutSize(IncludeScrollbars).height();
     return adjustForAbsoluteZoom(viewportHeight, style);
 }
 
-static int calculateDeviceWidth(LocalFrame* frame)
+int MediaValues::calculateDeviceWidth(LocalFrame* frame) const
 {
     ASSERT(frame && frame->view() && frame->settings() && frame->host());
     int deviceWidth = static_cast<int>(screenRect(frame->view()).width());
@@ -44,7 +54,7 @@ static int calculateDeviceWidth(LocalFrame* frame)
     return deviceWidth;
 }
 
-static int calculateDeviceHeight(LocalFrame* frame)
+int MediaValues::calculateDeviceHeight(LocalFrame* frame) const
 {
     ASSERT(frame && frame->view() && frame->settings() && frame->host());
     int deviceHeight = static_cast<int>(screenRect(frame->view()).height());
@@ -53,18 +63,18 @@ static int calculateDeviceHeight(LocalFrame* frame)
     return deviceHeight;
 }
 
-static bool calculateStrictMode(LocalFrame* frame)
+bool MediaValues::calculateStrictMode(LocalFrame* frame) const
 {
     ASSERT(frame && frame->document());
     return !frame->document()->inQuirksMode();
 }
 
-static float calculateDevicePixelRatio(LocalFrame* frame)
+float MediaValues::calculateDevicePixelRatio(LocalFrame* frame) const
 {
     return frame->devicePixelRatio();
 }
 
-static int calculateColorBitsPerComponent(LocalFrame* frame)
+int MediaValues::calculateColorBitsPerComponent(LocalFrame* frame) const
 {
     ASSERT(frame && frame->page() && frame->page()->mainFrame());
     if (screenIsMonochrome(frame->page()->mainFrame()->view()))
@@ -72,7 +82,7 @@ static int calculateColorBitsPerComponent(LocalFrame* frame)
     return screenDepthPerComponent(frame->view());
 }
 
-static int calculateMonochromeBitsPerComponent(LocalFrame* frame)
+int MediaValues::calculateMonochromeBitsPerComponent(LocalFrame* frame) const
 {
     ASSERT(frame && frame->page() && frame->page()->mainFrame());
     if (screenIsMonochrome(frame->page()->mainFrame()->view()))
@@ -80,31 +90,36 @@ static int calculateMonochromeBitsPerComponent(LocalFrame*
frame)
     return 0;
 }
 
-static int calculateDefaultFontSize(RenderStyle* style)
+int MediaValues::calculateDefaultFontSize(RenderStyle* style) const
 {
     return style->fontDescription().specifiedSize();
 }
 
-static bool calculateScanMediaType(LocalFrame* frame)
+int MediaValues::calculateComputedFontSize(RenderStyle* style) const
+{
+    return style->fontDescription().computedSize();
+}
+
+bool MediaValues::calculateScanMediaType(LocalFrame* frame) const
 {
     ASSERT(frame && frame->view());
     // Scan only applies to 'tv' media.
     return equalIgnoringCase(frame->view()->mediaType(), "tv");
 }
 
-static bool calculateScreenMediaType(LocalFrame* frame)
+bool MediaValues::calculateScreenMediaType(LocalFrame* frame) const
 {
     ASSERT(frame && frame->view());
     return equalIgnoringCase(frame->view()->mediaType(), "screen");
 }
 
-static bool calculatePrintMediaType(LocalFrame* frame)
+bool MediaValues::calculatePrintMediaType(LocalFrame* frame) const
 {
     ASSERT(frame && frame->view());
     return equalIgnoringCase(frame->view()->mediaType(), "print");
 }
 
-static bool calculateThreeDEnabled(LocalFrame* frame)
+bool MediaValues::calculateThreeDEnabled(LocalFrame* frame) const
 {
     ASSERT(frame && frame->contentRenderer() &&
frame->contentRenderer()->compositor());
     bool threeDEnabled = false;
@@ -113,7 +128,7 @@ static bool calculateThreeDEnabled(LocalFrame* frame)
     return threeDEnabled;
 }
 
-static MediaValues::PointerDeviceType
calculateLeastCapablePrimaryPointerDeviceType(LocalFrame* frame)
+MediaValues::PointerDeviceType
MediaValues::calculateLeastCapablePrimaryPointerDeviceType(LocalFrame* frame)
const
 {
     ASSERT(frame && frame->settings());
     if (frame->settings()->deviceSupportsTouch())
@@ -129,213 +144,4 @@ static MediaValues::PointerDeviceType
calculateLeastCapablePrimaryPointerDeviceT
     return MediaValues::UnknownPointer;
 }
 
-PassRefPtr<MediaValues> MediaValues::create(MediaValuesMode mode,
-    int viewportWidth,
-    int viewportHeight,
-    int deviceWidth,
-    int deviceHeight,
-    float devicePixelRatio,
-    int colorBitsPerComponent,
-    int monochromeBitsPerComponent,
-    PointerDeviceType pointer,
-    int defaultFontSize,
-    bool threeDEnabled,
-    bool scanMediaType,
-    bool screenMediaType,
-    bool printMediaType,
-    bool strictMode)
-{
-    ASSERT(mode == CachingMode);
-    RefPtr<MediaValues> mediaValues = adoptRef(new MediaValues(0, nullptr,
mode));
-    mediaValues->m_viewportWidth = viewportWidth;
-    mediaValues->m_viewportHeight = viewportHeight;
-    mediaValues->m_deviceWidth = deviceWidth;
-    mediaValues->m_deviceHeight = deviceHeight;
-    mediaValues->m_devicePixelRatio = devicePixelRatio;
-    mediaValues->m_colorBitsPerComponent = colorBitsPerComponent;
-    mediaValues->m_monochromeBitsPerComponent = monochromeBitsPerComponent;
-    mediaValues->m_pointer = pointer;
-    mediaValues->m_defaultFontSize = defaultFontSize;
-    mediaValues->m_threeDEnabled = threeDEnabled;
-    mediaValues->m_scanMediaType = scanMediaType;
-    mediaValues->m_screenMediaType = screenMediaType;
-    mediaValues->m_printMediaType = printMediaType;
-    mediaValues->m_strictMode = strictMode;
-
-    return mediaValues;
-}
-
-PassRefPtr<MediaValues> MediaValues::create(LocalFrame* frame, RenderStyle*
style, MediaValuesMode mode)
-{
-    ASSERT(frame && style);
-    RefPtr<MediaValues> mediaValues;
-    mediaValues = adoptRef(new MediaValues(frame, style, mode));
-    if (mode == CachingMode) {
-        mediaValues->m_viewportWidth = calculateViewportWidth(frame, style);
-        mediaValues->m_viewportHeight = calculateViewportHeight(frame, style),
-        mediaValues->m_deviceWidth = calculateDeviceWidth(frame),
-        mediaValues->m_deviceHeight = calculateDeviceHeight(frame),
-        mediaValues->m_devicePixelRatio = calculateDevicePixelRatio(frame),
-        mediaValues->m_colorBitsPerComponent =
calculateColorBitsPerComponent(frame),
-        mediaValues->m_monochromeBitsPerComponent =
calculateMonochromeBitsPerComponent(frame),
-        mediaValues->m_pointer =
calculateLeastCapablePrimaryPointerDeviceType(frame),
-        mediaValues->m_defaultFontSize = calculateDefaultFontSize(style),
-        mediaValues->m_threeDEnabled = calculateThreeDEnabled(frame),
-        mediaValues->m_scanMediaType = calculateScanMediaType(frame),
-        mediaValues->m_screenMediaType = calculateScreenMediaType(frame),
-        mediaValues->m_printMediaType = calculatePrintMediaType(frame),
-        mediaValues->m_strictMode = calculateStrictMode(frame);
-
-        mediaValues->m_style.clear();
-        mediaValues->m_frame = 0;
-    }
-
-    return mediaValues;
-}
-
-PassRefPtr<MediaValues> MediaValues::create(Document* document, MediaValuesMode
mode)
-{
-    ASSERT(document);
-    Document* executingDocument = document->importsController() ?
document->importsController()->master() : document;
-    ASSERT(executingDocument->frame());
-    ASSERT(executingDocument->renderer());
-    ASSERT(executingDocument->renderer()->style());
-    LocalFrame* frame = executingDocument->frame();
-    RenderStyle* style = executingDocument->renderer()->style();
-
-    return MediaValues::create(frame, style, mode);
-}
-
-PassRefPtr<MediaValues> MediaValues::copy() const
-{
-    ASSERT(m_mode == CachingMode && !m_style.get() && !m_frame);
-    RefPtr<MediaValues> mediaValues = adoptRef(new MediaValues(0, nullptr,
m_mode));
-    mediaValues->m_viewportWidth = m_viewportWidth;
-    mediaValues->m_viewportHeight = m_viewportHeight;
-    mediaValues->m_deviceWidth = m_deviceWidth;
-    mediaValues->m_deviceHeight = m_deviceHeight;
-    mediaValues->m_devicePixelRatio = m_devicePixelRatio;
-    mediaValues->m_colorBitsPerComponent = m_colorBitsPerComponent;
-    mediaValues->m_monochromeBitsPerComponent = m_monochromeBitsPerComponent;
-    mediaValues->m_pointer = m_pointer;
-    mediaValues->m_defaultFontSize = m_defaultFontSize;
-    mediaValues->m_threeDEnabled = m_threeDEnabled;
-    mediaValues->m_scanMediaType = m_scanMediaType;
-    mediaValues->m_screenMed…
(message too large)

[Yoav Weiss, 2014-04-17 06:01:47]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-17 06:01:58]

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

[commit-bot: I haz the power, 2014-04-17 06:18:52]

Change committed as 171843