Hiroyuki Ikezoe (:hiro) Assignee
	Description • 7 years ago

It seems to start failing on Android.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=821ea90bc82a42f6df93b3aeded41b1dfe163a4f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=165108651

running on the compositor or not: assert_equals: Animation reports that it is running on the compositor during playback at 0ms expected true but got false

I will disable it on Android now. And will look at the failure reason after landing bug 1193394.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 2 • 7 years ago

Comment on attachment 8955037 [details] [diff] [review]
Disable it on Android

arai, please use this patch when landing the microtask change. Thanks!

	Tooru Fujisawa [:arai]
	Comment 3 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0f18ca10ec79c11a91091c501fd56e8d551f30
Bug 1442150 - Skip test_running_on_compositor.html on Android. r=arai

	Cristina Coroiu [:ccoroiu]
	Comment 4 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5a0f18ca10ec

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 5 • 7 years ago

Oops, I didn't set leave-open.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 6 • 7 years ago

The test has been disabled.  Reuse this bug for enabling it.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 7 • 7 years ago

I have to check why the test failed on Android.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 8 • 7 years ago

Checking the animation started at the current time fixes the failure [1].  I hope it needs only in the first test case.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1cdd0e9d9a1deb6f9d4040914160cb8275aadae

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 13 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdd2e331ef291144ce295f354692244ea48ad765

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8956005 [details]
Bug 1442150 - Rewrite test_running_on_compositor.html with async/await.

https://reviewboard.mozilla.org/r/224948/#review231098

::: dom/animation/test/chrome/test_running_on_compositor.html:187
(Diff revision 2)
>    var div = addDiv(t, { style: 'animation: anim 100s' });
>    var animation = div.getAnimations()[0];
>  
> -  return waitForPaints().then(() => {
> -    return new Promise(resolve => {
> +  await waitForPaints();
> +
> +  await new Promise(resolve => {

Note that I did ask arai on IRC that whether we should do 'await new Promise' or 'return new Promise' if it's the final Promise in promise_test, the answer was 'await', so I did use it here and there.

	Brian Birtles (:birtles)
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8956005 [details]
Bug 1442150 - Rewrite test_running_on_compositor.html with async/await.

https://reviewboard.mozilla.org/r/224948/#review231130

Thank you!

	Brian Birtles (:birtles)
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8956006 [details]
Bug 1442150 - Move startsRightNow into testcommon.js and rename it to animationStartsRightNow.

https://reviewboard.mozilla.org/r/224950/#review231132

	Brian Birtles (:birtles)
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8956007 [details]
Bug 1442150 - Wait for one more frame if the animation starts at the current timeline time in the first test case.

https://reviewboard.mozilla.org/r/224952/#review231138

::: dom/animation/test/chrome/test_running_on_compositor.html:71
(Diff revision 2)
> +  await animation.ready;
> +
> +  // If the animation starts at the current timeline time, we need to wait for
> +  // one more frame.
> +  if (animationStartsRightNow(animation)) {
> +    await waitForFrame();
> +  }
> +
>    await waitForPaints();
>  
>    assert_animation_is_running_on_compositor(animation,
>       'Animation reports that it is running on the compositor'
>       + ' during playback');

I don't quite understand this.

We send animations to the compositor as part of painting so it shouldn't matter whether or not the main thread is doing its vsync-fixup step or not, right?

Could you explain the failure case a bit more?

Does assert_animation_is_running_on_compositor look at the main thread state? Or does the compositor also suffer from the "vsync makes the time go backwards" thing?

	Brian Birtles (:birtles)
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8956008 [details]
Bug 1442150 - Re-enable test_running_on_compositor.html on Android.

https://reviewboard.mozilla.org/r/224954/#review231140

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 23 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #21)
> Comment on attachment 8956007 [details]
> Bug 1442150 - Wait for one more frame if the animation starts at the current
> timeline time in the first test case.
> 
> https://reviewboard.mozilla.org/r/224952/#review231138
> 
> ::: dom/animation/test/chrome/test_running_on_compositor.html:71
> (Diff revision 2)
> > +  await animation.ready;
> > +
> > +  // If the animation starts at the current timeline time, we need to wait for
> > +  // one more frame.
> > +  if (animationStartsRightNow(animation)) {
> > +    await waitForFrame();
> > +  }
> > +
> >    await waitForPaints();
> >  
> >    assert_animation_is_running_on_compositor(animation,
> >       'Animation reports that it is running on the compositor'
> >       + ' during playback');
> 
> I don't quite understand this.
> 
> We send animations to the compositor as part of painting so it shouldn't
> matter whether or not the main thread is doing its vsync-fixup step or not,
> right?

Yes, that's right.  In this failure case I think it's somewhat related to the fake MozAfterPaint (bug 1439875).
What I am thinking is that when the animation started at the current timeline time, we skiped styling for the animation, at that time, we received the fake MozAfterPaint so that we don't yet send the animation to the compositor yet.

Anyway, as far as I can tell a proper solution is to land bug 1439875.

	Brian Birtles (:birtles)
	Comment 24 • 7 years ago

Ah, ok. If we're pretty sure that's the issue, could we add a comment to the patch to explain that?

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 25 • 7 years ago

Sure will do.

	Brian Birtles (:birtles)
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8956007 [details]
Bug 1442150 - Wait for one more frame if the animation starts at the current timeline time in the first test case.

https://reviewboard.mozilla.org/r/224952/#review231152

::: dom/animation/test/chrome/test_running_on_compositor.html:71
(Diff revision 3)
> +  await animation.ready;
> +
> +  // If the animation starts at the current timeline time, we need to wait for
> +  // one more frame to avoid receiving the fake timer-based MozAfterPaint event.
> +  // FIXME: Bug 1419226: Drop this 'waitForFrame'.  Once MozAfterPaint is fired
> +  // reliably, we just need to wait for a MozAfterPaint here.

Shouldn't this comment go before the `await animation.ready` too?

i.e. normally we only need to wait for paints to finish, right?

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 31 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #30)
> Comment on attachment 8956007 [details]
> Bug 1442150 - Wait for one more frame if the animation starts at the current
> timeline time in the first test case.
> 
> https://reviewboard.mozilla.org/r/224952/#review231152
> 
> ::: dom/animation/test/chrome/test_running_on_compositor.html:71
> (Diff revision 3)
> > +  await animation.ready;
> > +
> > +  // If the animation starts at the current timeline time, we need to wait for
> > +  // one more frame to avoid receiving the fake timer-based MozAfterPaint event.
> > +  // FIXME: Bug 1419226: Drop this 'waitForFrame'.  Once MozAfterPaint is fired
> > +  // reliably, we just need to wait for a MozAfterPaint here.
> 
> Shouldn't this comment go before the `await animation.ready` too?
> 
> i.e. normally we only need to wait for paints to finish, right?

Oh right. Good catch!  Thanks!

	Pulsebot
	Comment 34 • 7 years ago

Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e277cff709f
Rewrite test_running_on_compositor.html with async/await. r=birtles
https://hg.mozilla.org/integration/autoland/rev/532ce5be3ae2
Move startsRightNow into testcommon.js and rename it to animationStartsRightNow. r=birtles
https://hg.mozilla.org/integration/autoland/rev/cbd57b06f2f1
Wait for one more frame if the animation starts at the current timeline time in the first test case. r=birtles
https://hg.mozilla.org/integration/autoland/rev/614d32cc3555
Re-enable test_running_on_compositor.html on Android. r=birtles

	Tiberius Oros[:tiberius_oros]
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6e277cff709f
https://hg.mozilla.org/mozilla-central/rev/532ce5be3ae2
https://hg.mozilla.org/mozilla-central/rev/cbd57b06f2f1
https://hg.mozilla.org/mozilla-central/rev/614d32cc3555