Rework MediaQueryMatcher to batch up listener notification

Rework MediaQueryMatcher to batch up listener notification

MediaQueryMatcher currently runs script synchronously inside layout and
recalc style which is bad. Instead we should collect all the listeners
that have changed and notify them asynchronously. Incidentally this is
what the spec says to do too, and also what Firefox and the polyfills
do.

This is the first step where we collect all the listeners that changed
in a Vector and then notify them as a group. This also fixes our behavior
where adding a listener in the middle of notification would get that one
called too, which is not what the spec says.

This also changes the notification order such that listeners are notified
in groups based on the creation time of their MediaQueryList which also
matches Firefox (and the polyfill) instead of in the order of global
calls to addListener.

Based on https://codereview.chromium.org/214383008/ by esprehn@

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

[cbiesinger, 2014-06-26 04:05:31]

This is now working. And Elliott added a test for this function in
https://codereview.chromium.org/347423010/

[cbiesinger, 2014-06-26 04:28:04]

Mads - could you check that this patch handles Oilpan correctly?

[Yoav Weiss, 2014-06-26 04:43:19]

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

https://codereview.chromium.org/348893004/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryMatcher.cpp:80: return evaluator->eval(media);
Why aren't you updating m_evaluator here?

https://codereview.chromium.org/348893004/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryMatcher.cpp:124: m_evaluator = nullptr;
Why create and destroy this evaluator on each change? I believe you can keep
this evaluator around, and since it contains a reference to the LocaLFrame, it'd
get updated automagically if the media conditions change.

[cbiesinger, 2014-06-26 04:56:51]

On 2014/06/26 04:43:19, Yoav Weiss wrote:
>
https://codereview.chromium.org/348893004/diff/80001/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryMatcher.cpp (right):
> 
>
https://codereview.chromium.org/348893004/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryMatcher.cpp:80: return evaluator->eval(media);
> Why aren't you updating m_evaluator here?
> 
>
https://codereview.chromium.org/348893004/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryMatcher.cpp:124: m_evaluator = nullptr;
> Why create and destroy this evaluator on each change? I believe you can keep
> this evaluator around, and since it contains a reference to the LocaLFrame,
it'd
> get updated automagically if the media conditions change.

It's what the old code (and esprehn's patch) did. I'm happy to change it to keep
the evaluator around, though. Will update tomorrow.

[esprehn, 2014-06-26 05:01:46]

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

https://codereview.chromium.org/348893004/diff/80001/Source/core/css/MediaQue...
Source/core/css/MediaQueryMatcher.cpp:124: m_evaluator = nullptr;
On 2014/06/26 04:43:19, Yoav Weiss wrote:
> Why create and destroy this evaluator on each change? I believe you can keep
> this evaluator around, and since it contains a reference to the LocaLFrame,
it'd
> get updated automagically if the media conditions change.

The mediaType() is stored in the evaluator and won't get updated. It's not clear
why the thing wants both the frame and the mediaType() since the frame is there
the mediaType() comes from... we should fix the Evaulator to not need both and
just always ask the frame and then you can probably keep it around.

[esprehn, 2014-06-26 05:43:24]

lgtm, lots of random nits but this looks great.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:91: if (updateMatches()) {
early return.

if (!updateMatches())
  return;
for (...)
  toNotify->append(*i);

then you don't need any braces at all.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.h (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:64: typedef
HeapListHashSet<RefPtrWillBePersistent<MediaQueryListListener> > ListenerList;
I think this should be a Member, you only need to use Persistent when you're
storing things in a container off the Oilpan heap.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryMatcher.cpp (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryMatcher.cpp:46: // Take a ref to each MediaQuerylist
as removing the listeners in documentDetached
typo, MediaQueryList

[Mads Ager (chromium), 2014-06-26 06:06:23]

This is pretty close on the oilpan side, thanks! :)

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:47: // FIXME: Figure out how to deal with
this in oilpan.
Right, this will not work for the oilpan build since m_matcher can be dead in
the same GC cycle and could therefore already be destructed. If we cannot avoid
tie together the destruction and unregistration of the object we should use
Oilpan weakness.

In order to do that: guard this with #if !ENABLE(OILPAN) and update the
MediaQueryMatcher collection to use RawPtrWillBeWeakMember instead of
RawPtrWillBeMember. Then oilpan will remove the list from the collection when it
dies automatically.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.h (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:40: class MediaQueryList FINAL : public
RefCountedWillBeGarbageCollected<MediaQueryList> {
Since the class now has a non-trivial destructor it needs to be
...WillBeGarbageCollectedFinalized. (The plugin will complaint about this if you
compile with enable_oilpan=1, so this will be easy for us to catch and fix when
you land.)

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:64: typedef
HeapListHashSet<RefPtrWillBePersistent<MediaQueryListListener> > ListenerList;
MediaQueryList, MediaQueryListListener and HeapListHashSet are all on the oilpan
heap and pointers between them should be Members. Therefore, this should be
RefPtrWillBeMember. Also, you might want to use WillBeHeapListHashSet to only
use a heap allocated collection when oilpan is enabled.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryMatcher.h (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryMatcher.h:41: class MediaQueryMatcher FINAL : public
RefCountedWillBeGarbageCollected<MediaQueryMatcher> {
This now has an OwnPtr so it needs to be GarbageCollectedFinalized. Again, this
will be caught at compile-time with the clang plugin so it is easy to fix the
compile-error once compiling with enable_oilpan=1.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryMatcher.h:67: typedef
HeapListHashSet<RawPtrWillBeMember<MediaQueryList> > MediaQueryListSet;
This is where we should then use weak processing and have

WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<MediaQueryList> >

Please note that this needs to change from a ListHashSet to a LinkedHashSet. The
reason is that a ListHashSet does not support weakness (but supports limited
modification during iteration) and a LinkedHashSet supports weakness (not not
modification during iteration.) Other than that they have the same properties.

[Yoav Weiss, 2014-06-26 08:36:04]

On 2014/06/26 05:01:46, esprehn wrote:
>
https://codereview.chromium.org/348893004/diff/80001/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryMatcher.cpp (right):
> 
>
https://codereview.chromium.org/348893004/diff/80001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryMatcher.cpp:124: m_evaluator = nullptr;
> On 2014/06/26 04:43:19, Yoav Weiss wrote:
> > Why create and destroy this evaluator on each change? I believe you can keep
> > this evaluator around, and since it contains a reference to the LocaLFrame,
> it'd
> > get updated automagically if the media conditions change.
> 
> The mediaType() is stored in the evaluator and won't get updated. It's not
clear
> why the thing wants both the frame and the mediaType() since the frame is
there
> the mediaType() comes from... we should fix the Evaulator to not need both and
> just always ask the frame and then you can probably keep it around.

Oh, right. That doesn't make much sense. I'll take a stab at fixing it.

[cbiesinger, 2014-06-26 20:27:59]

Review comments fixed! Thanks for so much for the Oilpan advice.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:47: // FIXME: Figure out how to deal with
this in oilpan.
On 2014/06/26 06:06:22, Mads Ager (chromium) wrote:
> Right, this will not work for the oilpan build since m_matcher can be dead in
> the same GC cycle and could therefore already be destructed. If we cannot
avoid
> tie together the destruction and unregistration of the object we should use
> Oilpan weakness.
> 
> In order to do that: guard this with #if !ENABLE(OILPAN) and update the
> MediaQueryMatcher collection to use RawPtrWillBeWeakMember instead of
> RawPtrWillBeMember. Then oilpan will remove the list from the collection when
it
> dies automatically.

Done.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:91: if (updateMatches()) {
On 2014/06/26 05:43:24, esprehn wrote:
> early return.
> 
> if (!updateMatches())
>   return;
> for (...)
>   toNotify->append(*i);
> 
> then you don't need any braces at all.

Done.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.h (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:40: class MediaQueryList FINAL : public
RefCountedWillBeGarbageCollected<MediaQueryList> {
On 2014/06/26 06:06:22, Mads Ager (chromium) wrote:
> Since the class now has a non-trivial destructor it needs to be
> ...WillBeGarbageCollectedFinalized. (The plugin will complaint about this if
you
> compile with enable_oilpan=1, so this will be easy for us to catch and fix
when
> you land.)

Done.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.h:64: typedef
HeapListHashSet<RefPtrWillBePersistent<MediaQueryListListener> > ListenerList;
On 2014/06/26 06:06:22, Mads Ager (chromium) wrote:
> MediaQueryList, MediaQueryListListener and HeapListHashSet are all on the
oilpan
> heap and pointers between them should be Members. Therefore, this should be
> RefPtrWillBeMember. Also, you might want to use WillBeHeapListHashSet to only
> use a heap allocated collection when oilpan is enabled.

Done.

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryMatcher.cpp (right):

https://codereview.chromium.org/348893004/diff/110001/Source/core/css/MediaQu...
Source/core/css/MediaQueryMatcher.cpp:46: // Take a ref to each MediaQuerylist
as removing the listeners in documentDetached
On 2014/06/26 05:43:24, esprehn wrote:
> typo, MediaQueryList

Done.

[cbiesinger, 2014-06-26 21:32:02]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2014-06-26 21:33:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/348893004/150001

[esprehn, 2014-06-26 21:35:54]

https://codereview.chromium.org/348893004/diff/150001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryList.cpp (right):

https://codereview.chromium.org/348893004/diff/150001/Source/core/css/MediaQu...
Source/core/css/MediaQueryList.cpp:94: for (ListenerList::const_iterator i =
m_listeners.begin(), end = m_listeners.end(); i != end; ++i) {
"it", single letter variable names are usually only for int indexes.

[cbiesinger, 2014-06-26 21:37:04]

The CQ bit was unchecked by cbiesinger@chromium.org

[cbiesinger, 2014-06-26 23:15:30]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2014-06-26 23:17:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/348893004/130002

[commit-bot: I haz the power, 2014-06-27 01:31:32]

Change committed as 177038