Call media query change listeners asynchronously.

Call media query change listeners asynchronously.

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

[esprehn, 2014-07-01 00:01:07]

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

https://codereview.chromium.org/337883003/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryList.h:42: class Listener : public
RefCountedWillBeGarbageCollectedFinalized<Listener> {
This virtual abstraction seems like it could be a separate patch from the timing
change.

https://codereview.chromium.org/337883003/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryList.h:46: virtual bool equals(Listener* other) {
return this == other; }
This shouldn't need to be virtual.

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

https://codereview.chromium.org/337883003/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryListListener.h:51: MediaQueryListListener*
otherListener = other->isMediaQueryListListener();
"is" methods shouldn't return a pointer.

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

https://codereview.chromium.org/337883003/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryMatcher.cpp:116:
m_document->ensureScriptedAnimationController().scheduleAnimationIfNeeded();
scheduleAnimationIfNeeded() should be private, I think you want
scheduleVisualUpdate().

https://codereview.chromium.org/337883003/diff/60001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/337883003/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.h:1049: ScriptedAnimationController&
ensureScriptedAnimationController();
This should not be public.

https://codereview.chromium.org/337883003/diff/60001/Source/core/dom/Scripted...
File Source/core/dom/ScriptedAnimationController.cpp (right):

https://codereview.chromium.org/337883003/diff/60001/Source/core/dom/Scripted...
Source/core/dom/ScriptedAnimationController.cpp:212: if (!m_callbacks.size() &&
!m_eventQueue.size() && !m_document->mediaQueryMatcher().hasDelayedListeners())
ScriptedAnimationController should not be reaching backwards into the
MediaQueryMatcher like this.

[cbiesinger, 2014-07-01 00:12:09]

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

https://codereview.chromium.org/337883003/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryList.h:42: class Listener : public
RefCountedWillBeGarbageCollectedFinalized<Listener> {
On 2014/07/01 00:01:06, esprehn wrote:
> This virtual abstraction seems like it could be a separate patch from the
timing
> change.

It could. Want me to do that?

https://codereview.chromium.org/337883003/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryList.h:46: virtual bool equals(Listener* other) {
return this == other; }
On 2014/07/01 00:01:06, esprehn wrote:
> This shouldn't need to be virtual.

How so? The implementation for JS listeners needs to use
ScriptValue::operator==, whereas the C++ implementation needs pointer equality.

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

https://codereview.chromium.org/337883003/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryListListener.h:51: MediaQueryListListener*
otherListener = other->isMediaQueryListListener();
On 2014/07/01 00:01:06, esprehn wrote:
> "is" methods shouldn't return a pointer.

Can I rename it "to" even without adding an assert that the type is right? Or is
the preferred approach to add "is" and "to"?

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

https://codereview.chromium.org/337883003/diff/60001/Source/core/css/MediaQue...
Source/core/css/MediaQueryMatcher.cpp:116:
m_document->ensureScriptedAnimationController().scheduleAnimationIfNeeded();
On 2014/07/01 00:01:06, esprehn wrote:
> scheduleAnimationIfNeeded() should be private, I think you want
> scheduleVisualUpdate().

On PageAnimator? Hm... I'm not positive but I don't believe that will call
ensureScriptedAnimationController(), but that does need to be called.

https://codereview.chromium.org/337883003/diff/60001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/337883003/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.h:1049: ScriptedAnimationController&
ensureScriptedAnimationController();
On 2014/07/01 00:01:06, esprehn wrote:
> This should not be public.

(see above)

https://codereview.chromium.org/337883003/diff/60001/Source/core/dom/Scripted...
File Source/core/dom/ScriptedAnimationController.cpp (right):

https://codereview.chromium.org/337883003/diff/60001/Source/core/dom/Scripted...
Source/core/dom/ScriptedAnimationController.cpp:212: if (!m_callbacks.size() &&
!m_eventQueue.size() && !m_document->mediaQueryMatcher().hasDelayedListeners())
On 2014/07/01 00:01:06, esprehn wrote:
> ScriptedAnimationController should not be reaching backwards into the
> MediaQueryMatcher like this.

So, what should I do instead?

[esprehn, 2014-07-01 00:28:09]

Your layering is wrong if calling ensureScriptedAnimationController() has side
effects for the future frame. I think you want something like
Document::scheduleMediaQueryChange(Vector<...>) that takes the vector, creates
the animation controller, and calls a method on it. The animation controller
should not be public, you're reaching far across the system to cause a side
effect.

Also please do make the C++ change in a separate patch, those don't need to
happen at the same time and this patch is already pretty big.

[cbiesinger, 2014-07-01 01:19:41]

On 2014/07/01 00:28:09, esprehn wrote:
> Your layering is wrong if calling ensureScriptedAnimationController() has side
> effects for the future frame. I think you want something like
> Document::scheduleMediaQueryChange(Vector<...>) that takes the vector, creates
> the animation controller, and calls a method on it. The animation controller
> should not be public, you're reaching far across the system to cause a side
> effect.
> 
> Also please do make the C++ change in a separate patch, those don't need to
> happen at the same time and this patch is already pretty big.

Alright, I made those changes. Please take another look.

[esprehn, 2014-07-01 01:21:27]

lgtm, but I think you want a ListHashSet instead of manually combining them
which is n^2,

https://codereview.chromium.org/337883003/diff/100001/Source/core/dom/Scripte...
File Source/core/dom/ScriptedAnimationController.cpp (right):

https://codereview.chromium.org/337883003/diff/100001/Source/core/dom/Scripte...
Source/core/dom/ScriptedAnimationController.cpp:219:
m_mediaQueryListListeners.add(listeners[i]);
You want a ListHashSet for this.

[cbiesinger, 2014-07-01 01:34:57]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2014-07-01 01:35:35]

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

[commit-bot: I haz the power, 2014-07-01 02:40:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13686)

[commit-bot: I haz the power, 2014-07-01 03:21:26]

Change committed as 177267

[lushnikov, 2014-07-01 08:12:48]

A revert of this CL has been created in
https://codereview.chromium.org/368533003/ by lushnikov@chromium.org.

The reason for reverting is: The patch broke
inspector-protocol/css/media-query-listener-exception.html on Mac

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....

[lushnikov, 2014-07-01 08:23:02]

On 2014/07/01 08:12:48, lushnikov wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/368533003/ by mailto:lushnikov@chromium.org.
> 
> The reason for reverting is: The patch broke
> inspector-protocol/css/media-query-listener-exception.html on Mac
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....

Revert wasn't committed - sorry for the noize.

[yurys, 2014-07-11 09:30:22]

https://codereview.chromium.org/337883003/diff/150001/LayoutTests/inspector-p...
File LayoutTests/inspector-protocol/css/media-query-listener-exception.html
(right):

https://codereview.chromium.org/337883003/diff/150001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/css/media-query-listener-exception.html:23:
setTimeout("InspectorTest.completeTest();", 20);
This is bound to make the the test flaky and the flakiness dashboard confirms
this:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....

Сan we revert this change and reland it later with a proper fix for the test?

[Yoav Weiss, 2014-07-11 14:05:33]

On 2014/07/11 09:30:22, yurys wrote:
>
https://codereview.chromium.org/337883003/diff/150001/LayoutTests/inspector-p...
> File LayoutTests/inspector-protocol/css/media-query-listener-exception.html
> (right):
> 
>
https://codereview.chromium.org/337883003/diff/150001/LayoutTests/inspector-p...
> LayoutTests/inspector-protocol/css/media-query-listener-exception.html:23:
> setTimeout("InspectorTest.completeTest();", 20);
> This is bound to make the the test flaky and the flakiness dashboard confirms
> this:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....
> 
> Сan we revert this change and reland it later with a proper fix for the test?

Since I believe there are now other patches that rely on this one, it'd be
better (and simpler) if we could avoid reverting it.
Perhaps we can mark the test as flaky/skip until a fix is available?

cbiesinger - can you please take a look at fixing this flakiness issue? I'd take
a stab at it, but I'm on the road ATM

[yurys, 2014-07-11 14:43:06]

On 2014/07/11 14:05:33, Yoav Weiss wrote:
> On 2014/07/11 09:30:22, yurys wrote:
> >
>
https://codereview.chromium.org/337883003/diff/150001/LayoutTests/inspector-p...
> > File LayoutTests/inspector-protocol/css/media-query-listener-exception.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/337883003/diff/150001/LayoutTests/inspector-p...
> > LayoutTests/inspector-protocol/css/media-query-listener-exception.html:23:
> > setTimeout("InspectorTest.completeTest();", 20);
> > This is bound to make the the test flaky and the flakiness dashboard
confirms
> > this:
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....
> > 
> > Сan we revert this change and reland it later with a proper fix for the
test?
> 
> Since I believe there are now other patches that rely on this one, it'd be
> better (and simpler) if we could avoid reverting it.
> Perhaps we can mark the test as flaky/skip until a fix is available?
> 
> cbiesinger - can you please take a look at fixing this flakiness issue? I'd
take
> a stab at it, but I'm on the road ATM
The test has already been marked as [Failure Pass] which basically makes it
useless. I don't think it is a good practice to land patches that introduce test
flakiness.

Since there were other commits on top of this change it's probably better to fix
the test and remove custom expectations rather than reverting the CL. In general
we shouldn't commit such patches unless problems with the tests are resolved.

[cbiesinger, 2014-07-11 22:18:58]

On 2014/07/11 14:43:06, yurys wrote:
> On 2014/07/11 14:05:33, Yoav Weiss wrote:
> > On 2014/07/11 09:30:22, yurys wrote:
> > >
> >
>
https://codereview.chromium.org/337883003/diff/150001/LayoutTests/inspector-p...
> > > File
LayoutTests/inspector-protocol/css/media-query-listener-exception.html
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/337883003/diff/150001/LayoutTests/inspector-p...
> > > LayoutTests/inspector-protocol/css/media-query-listener-exception.html:23:
> > > setTimeout("InspectorTest.completeTest();", 20);
> > > This is bound to make the the test flaky and the flakiness dashboard
> confirms
> > > this:
> > >
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....
> > > 
> > > Сan we revert this change and reland it later with a proper fix for the
> test?
> > 
> > Since I believe there are now other patches that rely on this one, it'd be
> > better (and simpler) if we could avoid reverting it.
> > Perhaps we can mark the test as flaky/skip until a fix is available?
> > 
> > cbiesinger - can you please take a look at fixing this flakiness issue? I'd
> take
> > a stab at it, but I'm on the road ATM
> The test has already been marked as [Failure Pass] which basically makes it
> useless. I don't think it is a good practice to land patches that introduce
test
> flakiness.
> 
> Since there were other commits on top of this change it's probably better to
fix
> the test and remove custom expectations rather than reverting the CL. In
general
> we shouldn't commit such patches unless problems with the tests are resolved.

I certainly did not make the test flaky on purpose. Since we dispatch media list
events once per frame (ie. every 16 ms), 20 ms ought to be sufficient. However,
that does not seem to be true, so I will change the test to use
requestAnimationFrame.

[esprehn, 2014-07-11 22:32:00]

On 2014/07/11 at 22:18:58, cbiesinger wrote:
> ...
> > 
> > Since there were other commits on top of this change it's probably better to
fix
> > the test and remove custom expectations rather than reverting the CL. In
general
> > we shouldn't commit such patches unless problems with the tests are
resolved.
> 
> I certainly did not make the test flaky on purpose. Since we dispatch media
list events once per frame (ie. every 16 ms), 20 ms ought to be sufficient.
However, that does not seem to be true, so I will change the test to use
requestAnimationFrame.

DRT doesn't have real RAF unfortunately, so the timing of rafs are not as
predictable, they just get scheduled with setTimeout(0).

[cbiesinger, 2014-07-12 00:57:37]

On 2014/07/11 22:32:00, esprehn wrote:
> On 2014/07/11 at 22:18:58, cbiesinger wrote:
> > ...
> > > 
> > > Since there were other commits on top of this change it's probably better
to
> fix
> > > the test and remove custom expectations rather than reverting the CL. In
> general
> > > we shouldn't commit such patches unless problems with the tests are
> resolved.
> > 
> > I certainly did not make the test flaky on purpose. Since we dispatch media
> list events once per frame (ie. every 16 ms), 20 ms ought to be sufficient.
> However, that does not seem to be true, so I will change the test to use
> requestAnimationFrame.
> 
> DRT doesn't have real RAF unfortunately, so the timing of rafs are not as
> predictable, they just get scheduled with setTimeout(0).

oddly, requestAnimationFrame does not fix the test. I'll have to investigate
further.

[yurys, 2014-07-17 07:00:51]

On 2014/07/12 00:57:37, cbiesinger wrote:
> oddly, requestAnimationFrame does not fix the test. I'll have to investigate
> further.

Any update on this?

[Yoav Weiss, 2014-07-17 07:06:15]

On 2014/07/17 07:00:51, yurys wrote:
> On 2014/07/12 00:57:37, cbiesinger wrote:
> > oddly, requestAnimationFrame does not fix the test. I'll have to investigate
> > further.
> 
> Any update on this?

FWIW, I resolved a similar flake at
https://codereview.chromium.org/369423002/patch/180001/190003 by waiting for 2
RAFs to end (so RAF->RAF->setTimeout(0)).

[cbiesinger, 2014-07-17 19:13:47]

I finally figured out how these inspector tests work (fwiw, very unintuitively -
test.toString()?!), and posted a solution to
https://codereview.chromium.org/398413002/

At least, that should fix it - I was never able to reproduce the flakiness
locally. Let's see what tryservers think.

[yurys, 2014-07-18 08:54:37]

On 2014/07/17 19:13:47, cbiesinger wrote:
> I finally figured out how these inspector tests work (fwiw, very unintuitively
-
> test.toString()?!),
Yeah, that's a bit counter-intuitive. The reason for it is that we need to
inject some testing code into DevTools front-end so we pass it as a text to be
evaled in the front-end context.

> and posted a solution to
> https://codereview.chromium.org/398413002/
> 
> At least, that should fix it - I was never able to reproduce the flakiness
> locally. Let's see what tryservers think.
Thanks for fixing this!