Enable Media query evaluation in the preload scanner

Enable Media query evaluation in the preload scanner

This CL enables Media Query evaluation inside the preload scanner, without joining the main thread.
That is to enable resource fetching based on the media attribute, without hampering future resource fetching improvements.

Major changes:
* Added a MediaValues class that "freezes" the various media values needed for MQE when it is initialized.
* Initialize MediaValues and provide them to the PreloadScanner during its initialization.
* Run MQE inside the PreloadScanner, according to provided MediaValues.
* Removed mediaAttribute passing to the HTMLResourcePreloader, that runs on the main thread.

BUG=

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

[abarth-chromium, 2014-03-17 19:00:01]

https://codereview.chromium.org/201813002/diff/1/Source/core/css/MediaQueryEv...
File Source/core/css/MediaQueryEvaluator.h (right):

https://codereview.chromium.org/201813002/diff/1/Source/core/css/MediaQueryEv...
Source/core/css/MediaQueryEvaluator.h:85: ,
m_mediaType(mediaType.isolatedCopy())
These should be indented four more spaces

https://codereview.chromium.org/201813002/diff/1/Source/core/css/MediaQueryEv...
Source/core/css/MediaQueryEvaluator.h:151: RefPtrWillBeMember<RenderStyle>
m_style;
Why did this type change?

[Yoav Weiss, 2014-03-17 22:18:44]

On 2014/03/17 19:00:01, abarth wrote:
>
https://codereview.chromium.org/201813002/diff/1/Source/core/css/MediaQueryEv...
> File Source/core/css/MediaQueryEvaluator.h (right):
> 
>
https://codereview.chromium.org/201813002/diff/1/Source/core/css/MediaQueryEv...
> Source/core/css/MediaQueryEvaluator.h:85: ,
> m_mediaType(mediaType.isolatedCopy())
> These should be indented four more spaces

OK.

> 
>
https://codereview.chromium.org/201813002/diff/1/Source/core/css/MediaQueryEv...
> Source/core/css/MediaQueryEvaluator.h:151: RefPtrWillBeMember<RenderStyle>
> m_style;
> Why did this type change?

I've tried to follow ager's instructions, but looking at them again, I believe i
was mistaken. I'll revert that.

[Yoav Weiss, 2014-03-21 17:30:11]

ping

[Mads Ager (chromium), 2014-03-22 09:33:54]

https://codereview.chromium.org/201813002/diff/60001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluator.h (right):

https://codereview.chromium.org/201813002/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:45: class MediaValues : public
RefCountedWillBeGarbageCollected<MediaValues> {
As far as I can tell, there is no need to worry about Oilpan transition types in
this code yet. I would just make this RefCounted. I don't think any of the
objects that hold on to MediaValues are GarbageCollected yet and therefore the
oilpan build should continue to work without worrying about it for this one and
we will take care of it when we get there. :)

There are multiple things that would have to change in this code if you want to
keep this garbage collected. I don't want to spam this code review line by line
(since this is not needed right now) so I will just list a couple of things
here:

1. This needs to be GarbageCollectedFinalized because it needs the destructor of
the object to be called because it has a String field (which derefs a string
impl in its destructor).

2. For something that is GarbageCollected(Finalized) the create methods should
return raw pointers. So the create methods should use
PassRefPtrWillBeRawPtr<MediaValues> and the body should use adoptRefWillBeNoop.

3. The places where you hold on to a MediaValues pointer you are using a RefPtr.
With oilpan that translates to either a Persistent or a Member. Use a Persistent
if the object holding the pointer is *not* in the oilpan heap. A Persistent is a
root for the garbage collection and will keep the object pointed to alive. Use a
Member if the object holding the pointer *is* in the oilpan heap. You then need
to tell the garbage collector about the Member in the trace method of the object
containing the Member.

There is a very brief cheat sheet about how to transition here:
http://www.chromium.org/blink/blink-gc

All of the above would make the oilpan build fail. Most of them because of
normal compilation errors. And the rest will fail the verification of our clang
plugin.

https://codereview.chromium.org/201813002/diff/60001/Source/core/html/parser/...
File Source/core/html/parser/HTMLResourcePreloader.cpp (right):

https://codereview.chromium.org/201813002/diff/60001/Source/core/html/parser/...
Source/core/html/parser/HTMLResourcePreloader.cpp:75: /*
Code in comments.

[abarth-chromium, 2014-03-24 19:43:30]

Neat!  We'll probably need to iterate a few times on this CL, but it's off to a
good start.

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

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:106: , m_style(nullptr)
You can omit this line.  m_style will initialize itself to zero.

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:210: return expectedValue;
Can this case actually occur?  I would have thought it would be an error to call
this function without a Frame and without mediaValues.

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:331: }
It's kind of lame that each of these functions has to have this branch...  Can
we always use the MediaValues codepath?  Maybe instead of storing a LocalFrame*,
we should instead extract the media values for the frame?

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:885: int viewportHeight =
viewportSize(view).height();
Maybe we should call viewportSize once instead of once for the width and once
for the height?

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:886: FloatRect sg =
screenRect(frame->page()->mainFrame()->view());
sg <--- Please use complete words in variable names.  It's hard to tell from
reading this code what "sg" means.

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:933: mediaValues->m_mediaType));
Notice that we're sharing the buffer for m_mediaType.  Do we need to call
isolatedCopy here?

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluator.h (right):

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:45: class MediaValues : public
RefCounted<MediaValues> {
We prefer to have one class per file.  This class should go in a new
MediaValues.h file.  That makes it easier for people to find the definition of a
class.

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:47: // Should return a pointer that auto
destructs when copied
I'm not sure what this comment means...  Is this a FIXME about what this code
should do in the future, or does it describe the current behavior of the code?

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:49: static PassRefPtr<MediaValues>
copy(const MediaValues*);
Why isn't this a regular member function?

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:61: String getMediaType() { return
m_mediaType; }
We generally omit the "get" prefix on getters.  For example:

int viewportWidth() const { return m_viewportWidth; }

Also, getters should be marked const.

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:134: */
Can you convert these comments to C++ style comments (e.g., starting with //
rather than /*   )

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:135: MediaQueryEvaluator(const String&
acceptedMediaType, const MediaValues* , bool mediaFeatureResult);
You've got an extra space between the * and the ,

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:150: LocalFrame* m_frame; // Not owned.
I'd remove this comment.  The frame obviously isn't owned.

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:499:
startBackgroundParser(MediaValues::create(document()));
This doesn't seem correct.  We have a Config object that contains all the
configuration information for the background parser.  We should do this work
when we create the Config object.

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.cpp:575: m_preloadScanner =
adoptPtr(new HTMLPreloadScanner(m_options, document()->url(),
document()->devicePixelRatio(), MediaValues::create(document())));
Isn't devicePixelRatio contained in MediaValues?  Maybe we should teach
HTMLPreloadScanner to read the value out of the media values instead of
supplying it twice.

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLPreloadScanner.cpp:100: return ret;
I'd skip |ret| and just return the result of the function directly.

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLPreloadScanner.cpp:108: , m_mediaMatch(true)
We can probably come up with a slightly better name for this variable.  The
trick for boolean member variables is to read the follow sentence aloud:

"if <name-of-class> <name-of-bool-member>, ..."

If that makes sense, then we've got a good name.  Otherwise, there's room for
improvement.  For example,

"if the token preload scanner encountered img@src, ..."

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLPreloadScanner.cpp:163: 
This change looks spurious

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLPreloadScanner.cpp:306: 
ditto

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLPreloadScanner.h:36: 
This change looks spurious.

https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLPreloadScanner.h:62: bool
isSafeToSendToAnotherThread()
We need to teach this function about m_mediaValues.  For example, m_mediaValues
contains a String, which means we'll need to call
m_mediaValues->isSafeToSendToAnotherThread(), which will need to call
isSafeToSendToAnotherThread() on m_mediaType.

[Yoav Weiss, 2014-03-25 12:54:00]

Thanks for reviewing. I'll work on fixing the issues raised. A few answers &
questions below.

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

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:106: , m_style(nullptr)
On 2014/03/24 19:43:31, abarth wrote:
> You can omit this line.  m_style will initialize itself to zero.

Does that mean that I can also skip m_mediaValues(nullptr) in previous
constructors?

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:210: return expectedValue;
On 2014/03/24 19:43:31, abarth wrote:
> Can this case actually occur?  I would have thought it would be an error to
call
> this function without a Frame and without mediaValues.

You're right. This case is taken care of at eval().

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:331: }
On 2014/03/24 19:43:31, abarth wrote:
> It's kind of lame that each of these functions has to have this branch...  Can
> we always use the MediaValues codepath?  Maybe instead of storing a
LocalFrame*,
> we should instead extract the media values for the frame?

That makes sense. I'll do that.

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:933: mediaValues->m_mediaType));
On 2014/03/24 19:43:31, abarth wrote:
> Notice that we're sharing the buffer for m_mediaType.  Do we need to call
> isolatedCopy here?

It's called inside the MediaValues constructor.

[eseidel, 2014-03-25 13:15:18]

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluator.h (right):

https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.h:64: MediaValues(int viewportWidth,
I don't quite understand what's special about this class yet.  Should it just be
a struct instead of a class?  Will it have methods?

I guess it's read-only?  Then again that can be accomplished by passing it
around const...

[Yoav Weiss, 2014-03-25 13:23:07]

On 2014/03/25 13:15:18, eseidel wrote:
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryEvaluator.h (right):
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.h:64: MediaValues(int viewportWidth,
> I don't quite understand what's special about this class yet.  Should it just
be
> a struct instead of a class?  Will it have methods?
> 
> I guess it's read-only?  Then again that can be accomplished by passing it
> around const...

It has a create() static method and a copy() method. Nothing beyond that.
I can get easily replace it with a struct and eliminate all the getters, if
that's preferred.

[eseidel, 2014-03-25 13:24:21]

In general classes are nicer than structs, so you've probably made the right
pick.   I trust your judgement here, I haven't stared at this CL in enough depth
to say yet.

[eseidel, 2014-03-25 13:33:49]

(structs and classes are the same thing in c++, just class defaults to private
and struct defaults to public, but what I meant to imply in my previous comment
was that the pattern of getters/setters associated with classes is generally the
preferred pattern and what code tends towards.)

[abarth-chromium, 2014-03-25 15:31:43]

On 2014/03/25 12:54:00, Yoav Weiss wrote:
> Does that mean that I can also skip m_mediaValues(nullptr) in previous
> constructors?

Very likely.

>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:933: mediaValues->m_mediaType));
> On 2014/03/24 19:43:31, abarth wrote:
> > Notice that we're sharing the buffer for m_mediaType.  Do we need to call
> > isolatedCopy here?
> 
> It's called inside the MediaValues constructor.

There's no need to call isolatedCopy if the MediaValue always stays on the main
thread.  The typical pattern we use is to create an isolated copy of an object
immediately before moving it to another thread.  There some some cases where we
don't do that (e.g., SQLValue), but those classes end up hammering the allocator
and wasting tons of memory.  (/me shakes a stick at SQLValue.)

[Yoav Weiss, 2014-03-25 22:26:03]

On 2014/03/25 12:54:00, Yoav Weiss wrote:
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:331: }
> On 2014/03/24 19:43:31, abarth wrote:
> > It's kind of lame that each of these functions has to have this branch... 
Can
> > we always use the MediaValues codepath?  Maybe instead of storing a
> LocalFrame*,
> > we should instead extract the media values for the frame?
> 
> That makes sense. I'll do that.
> 

That proves to be trickier than it looks. I've moved everything to use
MediaValues on a local branch, but now LayoutTests that rely on the dynamic
nature of MQ eval are (rightfully) failing... Should I create, own and update a
MediaValues object outside of the MediaQueryEvaluator with the current media
values?

[abarth-chromium, 2014-03-25 22:39:15]

On 2014/03/25 22:26:03, Yoav Weiss wrote:
> That proves to be trickier than it looks. I've moved everything to use
> MediaValues on a local branch, but now LayoutTests that rely on the dynamic
> nature of MQ eval are (rightfully) failing... Should I create, own and update
a
> MediaValues object outside of the MediaQueryEvaluator with the current media
> values?

I'm not sure.  I'd have to study the code to see what would make the most sense.

[Yoav Weiss, 2014-03-26 15:41:52]

On 2014/03/25 22:39:15, abarth wrote:
> On 2014/03/25 22:26:03, Yoav Weiss wrote:
> > That proves to be trickier than it looks. I've moved everything to use
> > MediaValues on a local branch, but now LayoutTests that rely on the dynamic
> > nature of MQ eval are (rightfully) failing... Should I create, own and
update
> a
> > MediaValues object outside of the MediaQueryEvaluator with the current media
> > values?
> 
> I'm not sure.  I'd have to study the code to see what would make the most
sense.

Would it be possible to use both (LocalFrame, RenderStyle) and MediaValues in
this CL (despite the code branching in every Eval function, which I agree is not
ideal), and try to refactor that in a later one?

[abarth-chromium, 2014-03-26 16:48:45]

On 2014/03/26 15:41:52, Yoav Weiss wrote:
> Would it be possible to use both (LocalFrame, RenderStyle) and MediaValues in
> this CL (despite the code branching in every Eval function, which I agree is
not
> ideal), and try to refactor that in a later one?

Is there some reason it would be easier to do later?  That just seems like
taking on technical debt.

[Yoav Weiss, 2014-03-26 20:29:35]

On 2014/03/26 16:48:45, abarth wrote:
> On 2014/03/26 15:41:52, Yoav Weiss wrote:
> > Would it be possible to use both (LocalFrame, RenderStyle) and MediaValues
in
> > this CL (despite the code branching in every Eval function, which I agree is
> not
> > ideal), and try to refactor that in a later one?
> 
> Is there some reason it would be easier to do later?  That just seems like
> taking on technical debt.

Fair enough.

[abarth-chromium, 2014-03-27 03:33:39]

Maybe the solution is some sort of invalidation system whereby the media query
evaluator is notified if any of the values change?  I'm not that familiar with
this part of the system, so I'm not sure what the best approach is.  Another
approach might be to use a virtual interface with two subclasses, one of which
gets the values from a LocalFrame and another that gets them from a MediaValue
object.

[Yoav Weiss, 2014-03-27 06:50:24]

On 2014/03/27 03:33:39, abarth wrote:
> Maybe the solution is some sort of invalidation system whereby the media query
> evaluator is notified if any of the values change?  I'm not that familiar with
> this part of the system, so I'm not sure what the best approach is.  Another
> approach might be to use a virtual interface with two subclasses, one of which
> gets the values from a LocalFrame and another that gets them from a MediaValue
> object.

I like the second approach. If that's OK, I think I'll just start out by growing
MediaValues to take on both roles, with a flag indicating if it's in "caching"
mode or in "dynamic calculation" mode.
That way, the calculations from Frame/Style to the actual values happen in a
single place.
If I see that MediaValues becomes too complex that way, I'll split it to an
interface class, two sub-classes for dynamic values and cached values, and a
class that does the calculations for both.

[Yoav Weiss, 2014-03-27 15:11:14]

On 2014/03/24 19:43:30, abarth wrote:
> Neat!  We'll probably need to iterate a few times on this CL, but it's off to
a
> good start.
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryEvaluator.cpp (right):
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:106: , m_style(nullptr)
> You can omit this line.  m_style will initialize itself to zero.

Omitted.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:210: return expectedValue;
> Can this case actually occur?  I would have thought it would be an error to
call
> this function without a Frame and without mediaValues.

Dropped.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:331: }
> It's kind of lame that each of these functions has to have this branch...  Can
> we always use the MediaValues codepath?  Maybe instead of storing a
LocalFrame*,
> we should instead extract the media values for the frame?

Now MediaValues have two modes. In one it is caching the values and doesn't keep
LocalFrame & RenderStyle around, and in the other it is just an interface for
the above two.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:885: int viewportHeight =
> viewportSize(view).height();
> Maybe we should call viewportSize once instead of once for the width and once
> for the height?
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:886: FloatRect sg =
> screenRect(frame->page()->mainFrame()->view());
> sg <--- Please use complete words in variable names.  It's hard to tell from
> reading this code what "sg" means.
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:933: mediaValues->m_mediaType));
> Notice that we're sharing the buffer for m_mediaType.  Do we need to call
> isolatedCopy here?

I've dropped m_mediaType in favor of 3 booleans, which give us all the info we
need from it.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryEvaluator.h (right):
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.h:45: class MediaValues : public
> RefCounted<MediaValues> {
> We prefer to have one class per file.  This class should go in a new
> MediaValues.h file.  That makes it easier for people to find the definition of
a
> class.

Moved into its own class.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.h:47: // Should return a pointer that auto
> destructs when copied
> I'm not sure what this comment means...  Is this a FIXME about what this code
> should do in the future, or does it describe the current behavior of the code?
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.h:49: static PassRefPtr<MediaValues>
> copy(const MediaValues*);
> Why isn't this a regular member function?

Moved to be a member.
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.h:61: String getMediaType() { return
> m_mediaType; }
> We generally omit the "get" prefix on getters.  For example:
> 
> int viewportWidth() const { return m_viewportWidth; }
> 
> Also, getters should be marked const.

Done.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.h:134: */
> Can you convert these comments to C++ style comments (e.g., starting with //
> rather than /*   )

Done.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.h:135: MediaQueryEvaluator(const String&
> acceptedMediaType, const MediaValues* , bool mediaFeatureResult);
> You've got an extra space between the * and the ,

Fixed.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.h:150: LocalFrame* m_frame; // Not owned.
> I'd remove this comment.  The frame obviously isn't owned.

Removed.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLDocumentParser.cpp (right):
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLDocumentParser.cpp:499:
> startBackgroundParser(MediaValues::create(document()));
> This doesn't seem correct.  We have a Config object that contains all the
> configuration information for the background parser.  We should do this work
> when we create the Config object.

Moved the MediaValues creation to the config object initialization, inside
startBackgroundParser.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLDocumentParser.cpp:575: m_preloadScanner =
> adoptPtr(new HTMLPreloadScanner(m_options, document()->url(),
> document()->devicePixelRatio(), MediaValues::create(document())));
> Isn't devicePixelRatio contained in MediaValues?  Maybe we should teach
> HTMLPreloadScanner to read the value out of the media values instead of
> supplying it twice.

Done.
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLPreloadScanner.cpp (right):
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLPreloadScanner.cpp:100: return ret;
> I'd skip |ret| and just return the result of the function directly.

Skipped.
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLPreloadScanner.cpp:108: , m_mediaMatch(true)
> We can probably come up with a slightly better name for this variable.  The
> trick for boolean member variables is to read the follow sentence aloud:
> 
> "if <name-of-class> <name-of-bool-member>, ..."
> 
> If that makes sense, then we've got a good name.  Otherwise, there's room for
> improvement.  For example,
> 
> "if the token preload scanner encountered img@src, ..."

Renamed to matchedMediaAttribute.

> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLPreloadScanner.cpp:163: 
> This change looks spurious
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLPreloadScanner.cpp:306: 
> ditto
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLPreloadScanner.h (right):
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLPreloadScanner.h:36: 
> This change looks spurious.
> 
>
https://codereview.chromium.org/201813002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLPreloadScanner.h:62: bool
> isSafeToSendToAnotherThread()
> We need to teach this function about m_mediaValues.  For example,
m_mediaValues
> contains a String, which means we'll need to call
> m_mediaValues->isSafeToSendToAnotherThread(), which will need to call
> isSafeToSendToAnotherThread() on m_mediaType.

Since I dropped m_mediaType, this is no longer relevant.

[abarth-chromium, 2014-03-27 21:01:54]

LGTM

This looks great.  Thanks for the patch!

https://codereview.chromium.org/201813002/diff/250001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/201813002/diff/250001/Source/core/core.gypi#n...
Source/core/core.gypi:798: 'css/MediaValues.cpp',
Please keep this list in alphabetical order.  :)

https://codereview.chromium.org/201813002/diff/250001/Source/core/core.gypi#n...
Source/core/core.gypi:3301: 'css/MediaQueryEvaluatorTest.cpp',
ditto

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

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaQu...
Source/core/css/MediaQueryEvaluator.cpp:368: result =
roundForImpreciseConversion<int>(primitiveValue->getDoubleValue()*factor);
Should we extract this bit into a separate function?

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

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.cpp:3: // found in the LICENSE file.
Please add a blank line after this line.

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.cpp:23: inline static int
calculateViewportWidth(LocalFrame* frame, RenderStyle* style)
The inline keyword is redundant with the static keyword here.  You can just use
static for the same effect.

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.cpp:135: return adoptRef(new MediaValues(style,
frame, mode));
It's odd that the constructor uses a different order of arguments than the
create function.  Can you change them to be the same?

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.cpp:166: strictMode));
Another option is to skip this constructor with the long list of arguments.  You
can create the object on line 136 and then store the calculated values directly
into the object instead of using local variables.  Because this function is part
of the MediaValues class, it can write the private fields on the object you're
about to return.

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.cpp:198: m_strictMode));
Again, you can do away with the large constructor call here by assigning the
fields individually.

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

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.h:21: enum MediaValuesMode { CachingMode,
DynamicMode };
These should all be on separate lines.

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.h:27: enum PointerDeviceType { TouchPointer,
MousePointer, NoPointer, UnknownPointer };
These too.

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.h:48: MediaValuesMode mode)
You can merge these three lines though.  :)

https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
Source/core/css/MediaValues.h:110: int m_viewportWidth;
I'd add a comment here explaining that the members below this point need to be
threadsafe because they're copied to another thread.

I'd also add an isSafeToSendToAnotherThread function that checks !m_style,
!m_frame, m_mode == CachingMode, and hasOneRef().  You'll also need to call
isSafeToSendToAnotherThread in the appropriate place in the parser.

[Yoav Weiss, 2014-03-28 00:32:39]

On 2014/03/27 21:01:54, abarth wrote:
> LGTM
> 
> This looks great.  Thanks for the patch!
Thanks for reviewing! :)

> 
> https://codereview.chromium.org/201813002/diff/250001/Source/core/core.gypi
> File Source/core/core.gypi (right):
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/core.gypi#n...
> Source/core/core.gypi:798: 'css/MediaValues.cpp',
> Please keep this list in alphabetical order.  :)
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/core.gypi#n...
> Source/core/core.gypi:3301: 'css/MediaQueryEvaluatorTest.cpp',
> ditto

Fixed.
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryEvaluator.cpp (right):
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryEvaluator.cpp:368: result =
> roundForImpreciseConversion<int>(primitiveValue->getDoubleValue()*factor);
> Should we extract this bit into a separate function?

Done.
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> File Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.cpp:3: // found in the LICENSE file.
> Please add a blank line after this line.

Added.
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.cpp:23: inline static int
> calculateViewportWidth(LocalFrame* frame, RenderStyle* style)
> The inline keyword is redundant with the static keyword here.  You can just
use
> static for the same effect.

Removed.
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.cpp:135: return adoptRef(new MediaValues(style,
> frame, mode));
> It's odd that the constructor uses a different order of arguments than the
> create function.  Can you change them to be the same?

Changed.
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.cpp:166: strictMode));
> Another option is to skip this constructor with the long list of arguments. 
You
> can create the object on line 136 and then store the calculated values
directly
> into the object instead of using local variables.  Because this function is
part
> of the MediaValues class, it can write the private fields on the object you're
> about to return.
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.cpp:198: m_strictMode));
> Again, you can do away with the large constructor call here by assigning the
> fields individually.

I need that constructor for testing purposes (Testing MQEvaluator without
creating real Frames and RenderStyles).
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> File Source/core/css/MediaValues.h (right):
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.h:21: enum MediaValuesMode { CachingMode,
> DynamicMode };
> These should all be on separate lines.
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.h:27: enum PointerDeviceType { TouchPointer,
> MousePointer, NoPointer, UnknownPointer };
> These too.
> 

Separated.

>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.h:48: MediaValuesMode mode)
> You can merge these three lines though.  :)

Merged.
> 
>
https://codereview.chromium.org/201813002/diff/250001/Source/core/css/MediaVa...
> Source/core/css/MediaValues.h:110: int m_viewportWidth;
> I'd add a comment here explaining that the members below this point need to be
> threadsafe because they're copied to another thread.
> 
> I'd also add an isSafeToSendToAnotherThread function that checks !m_style,
> !m_frame, m_mode == CachingMode, and hasOneRef().  You'll also need to call
> isSafeToSendToAnotherThread in the appropriate place in the parser.

Added an isSafeToSendToAnotherThread function, and added a static function in
HTMLDocumentParser that creates a MediaValues object and asserts that it is safe
to send.

[Yoav Weiss, 2014-03-28 01:08:41]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-03-28 01:09:08]

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

[commit-bot: I haz the power, 2014-03-28 02:57:00]

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

[commit-bot: I haz the power, 2014-03-28 02:57:00]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[abarth-chromium, 2014-03-28 04:00:23]

Looks related to your CL:

#0 0x7fe806829ea1 base::debug::StackTrace::StackTrace()
#1 0x7fe8068297cd base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fe7fc1e9cb0 <unknown>
#3 0x7fe801fcb87e WTF::RefCountedBase::~RefCountedBase()
#4 0x7fe8021c1a60 WTF::RefCounted<>::~RefCounted()
#5 0x7fe8021c1c22 WebCore::MediaValues::~MediaValues()
#6 0x7fe8021c1905 WebCore::MediaQueryEvaluatorTest_Basic_Test::TestBody()

[Yoav Weiss, 2014-03-28 09:21:40]

On 2014/03/28 04:00:23, abarth wrote:
> Looks related to your CL:
> 
> #0 0x7fe806829ea1 base::debug::StackTrace::StackTrace()
> #1 0x7fe8068297cd base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #2 0x7fe7fc1e9cb0 <unknown>
> #3 0x7fe801fcb87e WTF::RefCountedBase::~RefCountedBase()
> #4 0x7fe8021c1a60 WTF::RefCounted<>::~RefCounted()
> #5 0x7fe8021c1c22 WebCore::MediaValues::~MediaValues()
> #6 0x7fe8021c1905 WebCore::MediaQueryEvaluatorTest_Basic_Test::TestBody()

Indeed. The initialization of MediaValues inside MediaQueryEvaluatorTest was to
a local variable, rather than a RefPtr.

I also eliminated the value based constructor (as you suggested in the review),
and created a "create()" function that does that instead (and used only in the
tests at the moment). It seemed cleaner than having public constructors.

[Yoav Weiss, 2014-03-28 09:56:28]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-03-28 09:56:35]

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

[commit-bot: I haz the power, 2014-03-28 11:47:00]

Change committed as 170314