Closed Bug 1442150 Opened 2 years ago Closed 2 years ago

Enable test_running_on_compositor.html due to bug 1193394

Categories

(Core :: DOM: Animation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(5 files)

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.
Comment on attachment 8955037 [details] [diff] [review]
Disable it on Android

arai, please use this patch when landing the microtask change. Thanks!
Attachment #8955037 - Flags: review?(arai.unmht)
Attachment #8955037 - Flags: review?(arai.unmht) → review+
Blocks: 1193394
https://hg.mozilla.org/mozilla-central/rev/5a0f18ca10ec
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Oops, I didn't set leave-open.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
The test has been disabled.  Reuse this bug for enabling it.
No longer blocks: 1193394
Depends on: 1193394
Summary: Fix a failure in test_running_on_compositor.html due to bug 1193394 → Enable test_running_on_compositor.html due to bug 1193394
I have to check why the test failed on Android.
Flags: needinfo?(hikezoe)
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
Assignee: nobody → hikezoe
Status: REOPENED → ASSIGNED
Flags: needinfo?(hikezoe)
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.
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!
Attachment #8956005 - Flags: review?(bbirtles) → 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
Attachment #8956006 - Flags: review?(bbirtles) → 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?
Comment on attachment 8956008 [details]
Bug 1442150 - Re-enable test_running_on_compositor.html on Android.

https://reviewboard.mozilla.org/r/224954/#review231140
Attachment #8956008 - Flags: review?(bbirtles) → review+
(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.
Ah, ok. If we're pretty sure that's the issue, could we add a comment to the patch to explain that?
Sure will do.
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?
Attachment #8956007 - Flags: review?(bbirtles) → review+
(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!
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.