Wait for two requestAnimationFrames in test_animation_performance_warning.html and test_running_on_compositor.html

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(4 attachments)

In test_animation_performance_warning.html, we use Animation.ready to wait for the animation being sent to the compositor, but once bug 1193394 is fixed, it won't work as expected.

Instead of Animation.ready we should wait for MozAfterPaint for that purpose, but unfortunately the event does not reliably work (bug 1341294).

We should wait for two requestAnimationFrames as a workaround here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b66a39cb788820c86ff3781165606bb0b8deece1
test_running_on_compositor.html has the same setup, I will post additional patches here.
Summary: Wait for two requestAnimationFrames in test_animation_performance_warning.html → Wait for two requestAnimationFrames in test_animation_performance_warning.html and test_running_on_compositor.html
Comment on attachment 8925777 [details]
Bug 1415042 - Use arrow function in test_animation_performance_warning.html.

https://reviewboard.mozilla.org/r/196948/#review202098
Attachment #8925777 - Flags: review?(bbirtles) → review+
Comment on attachment 8925778 [details]
Bug 1415042 - Wait for two requestAnimationFrames to ensure that animations have been tried to send to the compositor in test_animation_performance_warning.html.

https://reviewboard.mozilla.org/r/196950/#review202102

Thanks for the detailed explanation!

::: dom/animation/test/chrome/test_animation_performance_warning.html:46
(Diff revision 2)
> +// a fake function waiting for a paint.
> +function waitForPaints() {
> +  return waitForAnimationFrames(2);
> +  // FIXME: Instead waiting for two requestAnimationFrames, we should wait for
> +  // MozAfterPaint once after MozAfterPaint is fired properly (bug 1341294).
> +  //return new Promise(function(resolve, reject) {
> +  //  waitForAllPaintsFlushed(resolve);
> +  //});
> +}

Can you file a bug to fix this and make that bug depend on bug 1193394?

(Also, see my comment on the last patch about dropping or simplifying the commented-out implementation.)
Attachment #8925778 - Flags: review?(bbirtles) → review+
Comment on attachment 8925784 [details]
Bug 1415042 - Use arrow function in test_running_on_compositor.html.

https://reviewboard.mozilla.org/r/196956/#review202104
Attachment #8925784 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #10)
> Comment on attachment 8925778 [details]
> Bug 1415042 - Wait for two requestAnimationFrames to ensure that animations
> have been tried to send to the compositor in
> test_animation_performance_warning.html.
> 
> https://reviewboard.mozilla.org/r/196950/#review202102
> 
> Thanks for the detailed explanation!
> 
> ::: dom/animation/test/chrome/test_animation_performance_warning.html:46
> (Diff revision 2)
> > +// a fake function waiting for a paint.
> > +function waitForPaints() {
> > +  return waitForAnimationFrames(2);
> > +  // FIXME: Instead waiting for two requestAnimationFrames, we should wait for
> > +  // MozAfterPaint once after MozAfterPaint is fired properly (bug 1341294).
> > +  //return new Promise(function(resolve, reject) {
> > +  //  waitForAllPaintsFlushed(resolve);
> > +  //});
> > +}
> 
> Can you file a bug to fix this and make that bug depend on bug 1193394?

Sorry, my mistake. This depends on bug 1341294 which is already mentioned. Ignore this comment.
Comment on attachment 8925785 [details]
Bug 1415042 - Wait for two requestAnimationFrames to ensure that animations have been tried to send to the compositor in test_running_on_compositor.html.

https://reviewboard.mozilla.org/r/196958/#review202096

::: dom/animation/test/chrome/test_running_on_compositor.html:6
(Diff revision 1)
>  <!doctype html>
>  <head>
>  <meta charset=utf-8>
>  <title>Bug 1045994 - Add a chrome-only property to inspect if an animation is
>         running on the compositor or not</title>
> +<!-- script src="chrome://mochikit/content/tests/SimpleTest/paint_listener.js"></script -->

Not sure we really need this do we?

(And if we do, perhaps we should add a comment mentioning bug 1341294.)

::: dom/animation/test/testcommon.js:378
(Diff revision 1)
> +  //return new Promise(function(resolve, reject) {
> +  //  waitForAllPaintsFlushed(resolve);
> +  //});

I'm not sure we really need the commented-out implementation, but if we do we could probably make it a little simpler:

  return new Promise(resolve => {
    waitForAllPaintsFlushed(resolve);
  }

::: dom/animation/test/testcommon.js:382
(Diff revision 1)
> +  // MozAfterPaint once after MozAfterPaint is fired properly (bug 1341294).
> +  //return new Promise(function(resolve, reject) {
> +  //  waitForAllPaintsFlushed(resolve);
> +  //});
> +}
> +

Nit: unnecessary blank line at end here?
Attachment #8925785 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #13)
> Comment on attachment 8925785 [details]
> Bug 1415042 - Wait for two requestAnimationFrames to ensure that animations
> have been tried to send to the compositor in test_running_on_compositor.html.
> 
> https://reviewboard.mozilla.org/r/196958/#review202096
> 
> ::: dom/animation/test/chrome/test_running_on_compositor.html:6
> (Diff revision 1)
> >  <!doctype html>
> >  <head>
> >  <meta charset=utf-8>
> >  <title>Bug 1045994 - Add a chrome-only property to inspect if an animation is
> >         running on the compositor or not</title>
> > +<!-- script src="chrome://mochikit/content/tests/SimpleTest/paint_listener.js"></script -->
> 
> Not sure we really need this do we?

Yeah, I will drop this here in this bug.

> ::: dom/animation/test/testcommon.js:378
> (Diff revision 1)
> > +  //return new Promise(function(resolve, reject) {
> > +  //  waitForAllPaintsFlushed(resolve);
> > +  //});
> 
> I'm not sure we really need the commented-out implementation, but if we do
> we could probably make it a little simpler:
> 
>   return new Promise(resolve => {
>     waitForAllPaintsFlushed(resolve);
>   }

I wonder why I did not use arrow function.

> ::: dom/animation/test/chrome/test_animation_performance_warning.html:46
> (Diff revision 2)
> > +// a fake function waiting for a paint.
> > +function waitForPaints() {
> > +  return waitForAnimationFrames(2);
> > +  // FIXME: Instead waiting for two requestAnimationFrames, we should wait for
> > +  // MozAfterPaint once after MozAfterPaint is fired properly (bug 1341294).
> > +  //return new Promise(function(resolve, reject) {
> > +  //  waitForAllPaintsFlushed(resolve);
> > +  //});
> > +}
> 
> Can you file a bug to fix this and make that bug depend on bug 1193394?

Filed bug 1415065.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b023fdb6af09
Use arrow function in test_animation_performance_warning.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/1dc0be215168
Wait for two requestAnimationFrames to ensure that animations have been tried to send to the compositor in test_animation_performance_warning.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d73110cb1e93
Use arrow function in test_running_on_compositor.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/917324392dd4
Wait for two requestAnimationFrames to ensure that animations have been tried to send to the compositor in test_running_on_compositor.html. r=birtles
You need to log in before you can comment on or make changes to this bug.