1104435 - Add AnimationPlayer.ready

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Bug 927349 will change the way animations start so that they don't begin until their first frame has been rendered. However, some tests assume that animations begin immediately and these tests will begin to fail if we don't update them.

For tests of the Web Animations API, Web Animations defines a 'ready' promise specifically for this behavior. When a player is played a new ready promise object is created and resolved when the player actually starts playing.

In this bug I'd like to add the interface member and create the promise objects each time play() is called. Since we don't do the deferred start behavior yet these promises will be more-or-less resolved immediately. That should be enough that we can update tests to wait on the ready promise so that they continue to pass once we introduced the deferred-start behavior in bug 927349.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Spec link:

  http://w3c.github.io/web-animations/#the-current-ready-promise

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Make AnimationTimeline::GetParentObject return an nsIGlobalObject

In order to create a Promise object for AnimationPlayer.ready, we need an
nsIGlobalObject. Currently we can access this through the following chain:

  AnimationPlayer -> AnimationTimeline -> Document -> nsIGlobalObject

Come bug 1096776 (Support AnimationPlayers without a timeline or with an
inactive timeline) we will no longer be able to rely on having an
AnimationTimeline so we will probably have to store the corresponding window
object on the AnimationPlayer. For now, however, we can look up the timeline as
above.

This patch makes this a little more straightforward by changing the return type
and value of AnimationTimeline::GetParentObject to return the nsIGlobalObject of
the document with which it is associated.

REVIEW: I have no idea if this makes sense or not!

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Make AnimationPlayer derive from nsISupports

Web Animations defines the AnimationPlayer.ready Promise as type
Promise<AnimationPlayer>. This promise resolves to the AnimationPlayer object on
which it is defined. However, in order to be able to pass AnimationPlayer as
a resolution value it needs to implement nsISupports.

REVIEW: Does this make any sense? If it doesn't, we can fix the spec.

This patch makes AnimationPlayer derive from nsISupports.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Add AnimationPlayer.ready promise

This patch simply adds the ready promise to AnimationPlayer. Creating new
promises and resolving them is added in a subsequent patch.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8528192 [details] [diff] [review]
part 1 - Make AnimationTimeline::GetParentObject return an nsIGlobalObject

>-  nsISupports* GetParentObject() const { return mDocument; }
>+  nsIGlobalObject* GetParentObject() const {
{ goes to the next line

>+    return mDocument->GetParentObject();
>+  }
This is wrong per https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class
but so is the old setup too. mDocument stays the same sure, but mDocument->GetParentObject() returns a different object
after document.open() calls (called after the document has loaded), since document.open() creates a new Window object.


Since this patch doesn't cause any new bugs, fine, but please file a new bug to sort out the parent object handling.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8528193 [details] [diff] [review]
part 2 - Make AnimationPlayer derive from nsISupports

> 
>   AnimationTimeline* GetParentObject() const { return mTimeline; }
Again not related to this bug, but returning mTimeline here looks wrong.

I'm assuming here we implement the Constructor for the AnimationPlayer, like the spec says.
(looks like it is not implemented yet)
If you create a player in Window 1 but pass timeline from Window2.document.timeline as param to the ctor,
we'd parent the player to Window2.

This could be sorted out in the same bug which would fix AnimationTimeline parenting.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8528193 [details] [diff] [review]
part 2 - Make AnimationPlayer derive from nsISupports

But, remember, per spec AnimationPlayer may not always be associated with a timeline.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8528194 [details] [diff] [review]
part 3 - Add AnimationPlayer.ready promise

>+AnimationPlayer::AnimationPlayer(AnimationTimeline* aTimeline)
>+  : mTimeline(aTimeline)
>+  , mIsRunningOnCompositor(false)
>+  , mIsPreviousStateFinished(false)
>+{
>+  nsIGlobalObject* global = aTimeline->GetParentObject();
>+
>+  // The ready promise should be initially resolved
>+  if (global) {
>+    ErrorResult rv;
>+    mReady = Promise::Create(global, rv);
>+    if (mReady) {
>+      // REVIEW: Using |this| in the ctor like this isn't great.
>+      // We can probably fix this when we implement the AnimationPlayer ctor
>+      // (bug 1096774) by doing it there and calling it separately.
>+      // I'm not sure if it's worth doing anything until then.
>+      mReady->MaybeResolve(this);
This is no no. If MaybeResolve happens to addref/release the object you pass in, you'd get
first refcnt -> 1 and then refcnt -> 0 and the object would be deleted.
(in practice it wouldn't be currently since this is a cycle collectable object, and refcnt 0 means
 in the current CC setup that object will be deleted asynchronously later if refcnt is still 0 then.)


Why don't you create the Promise object lazily when Ready() is first time called?

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Since this patch doesn't cause any new bugs, fine, but please file a new bug
> to sort out the parent object handling.

Thanks, I filed bug 1105098 for this.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Add AnimationPlayer.ready promise

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8528194 [details] [diff] [review]
> part 3 - Add AnimationPlayer.ready promise
...
> This is no no. If MaybeResolve happens to addref/release the object you pass
> in, you'd get
> first refcnt -> 1 and then refcnt -> 0 and the object would be deleted.
> (in practice it wouldn't be currently since this is a cycle collectable
> object, and refcnt 0 means
>  in the current CC setup that object will be deleted asynchronously later if
> refcnt is still 0 then.)
> 
> 
> Why don't you create the Promise object lazily when Ready() is first time
> called?

Oh, good point. I've done that now. Let me know if you prefer I just make
GetReady non-const rather than doing const_cast-ing. I think keeping GetReady
const is preferable but I don't mind either way.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Create and resolve new ready promises on play()

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Factor out common animation test methods into testcommon.js

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Use step_func inside Promise callbacks

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 7 - Update AnimationPlayer tests to wait on ready promise

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 8 - Add tests for AnimationPlayer.ready

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 9 - Make getting AnimationPlayer.ready flush for CSS Animation players

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8528764 [details] [diff] [review]
part 3 - Add AnimationPlayer.ready promise

>+Promise*
>+AnimationPlayer::GetReady(ErrorResult& aRv) const

I would just drop 'const', since the method isn't doing const stuff.

>+{
>+  // Lazily create the ready promise if it doesn't exist.
>+  if (!mReady) {
>+    nsIGlobalObject* global = mTimeline->GetParentObject();
>+    if (global) {
>+      AnimationPlayer* mutableThis = const_cast<AnimationPlayer*>(this);
>+      mutableThis->mReady = Promise::Create(global, aRv);
>+      // The ready promise should be initially resolved.
>+      if (mReady) {
>+        mutableThis->mReady->MaybeResolve(mutableThis);
>+      }
>+    }
>+  }
>+  if (!mReady && !aRv.Failed()) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+  }
Drop ' && !aRv.Failed()'

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Make AnimationTimeline::GetParentObject return an nsIGlobalObject

Updated with review feedback addressed

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Add AnimationPlayer.ready promise

Updated with review feedback addressed

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Create and resolve new ready promises on play()

Fix bitrot

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Factor out common animation test methods into testcommon.js

Add patch description

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 9 - Make getting AnimationPlayer.ready flush for CSS Animation players

Fix bitrot

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8528857 [details] [diff] [review]
part 8 - Add tests for AnimationPlayer.ready

Review of attachment 8528857 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/test/css-animations/test_animation-player-ready.html
@@ +61,5 @@
> +                  'Ready promise has same object identity after redundant call'
> +                  + ' to play()');
> +    t.done();
> +  });
> +}, 'Redundant calls to play() to not generate new ready promise objects');

s/to not/do not/

::: dom/animation/test/css-transitions/test_animation-player-ready.html
@@ +7,5 @@
> +<script>
> +'use strict';
> +
> +async_test(function(t) {
> +  var div = addDiv();

Pass in t here and remove the div.remove() call later?

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8536263 [details] [diff] [review]
part 9 - Make getting AnimationPlayer.ready flush for CSS Animation players

Review of attachment 8536263 [details] [diff] [review]:
-----------------------------------------------------------------

Why don't we need to do this for CSS Transitions as well?

Comment 25

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Cameron McCormack (:heycam) from comment #24)
> Comment on attachment 8536263 [details] [diff] [review]
> part 9 - Make getting AnimationPlayer.ready flush for CSS Animation players
> 
> Review of attachment 8536263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why don't we need to do this for CSS Transitions as well?

Because there would be no observable difference even if we did.

The identity and state of the ready promise will only change if we call/set play()/pause()/cancel()/reverse()/startTime/currentTime/timeline/source and there's no style we can change that will result in any of those things happening for an existing transition. For CSS Animations, on the other hand, updating animation-play-state can cause play()/pause() to be called.

Comment 26

[Cameron McCormack (:heycam)]

OK.  Can you comment in here to that effect (just in case someone goes looking for the corresponding transitions style flush)?

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/949f07b6431b
https://hg.mozilla.org/integration/mozilla-inbound/rev/423b74c2d755
https://hg.mozilla.org/integration/mozilla-inbound/rev/2873d38a77f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1738cc0a795c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e822aece7d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b58effcd86
https://hg.mozilla.org/integration/mozilla-inbound/rev/22425168f2a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c2b54c64de
https://hg.mozilla.org/integration/mozilla-inbound/rev/745bffad22a6

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb9dd540956f

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Cameron McCormack (:heycam) from comment #26)
> OK.  Can you comment in here to that effect (just in case someone goes
> looking for the corresponding transitions style flush)?

Oops, just saw this now. There are a number of cases where this is true, not just the ready promise. I'll add a cover-all comment to this effect in CSSTransitionsPlayer.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/949f07b6431b
https://hg.mozilla.org/mozilla-central/rev/423b74c2d755
https://hg.mozilla.org/mozilla-central/rev/2873d38a77f7
https://hg.mozilla.org/mozilla-central/rev/1738cc0a795c
https://hg.mozilla.org/mozilla-central/rev/0e822aece7d2
https://hg.mozilla.org/mozilla-central/rev/d7b58effcd86
https://hg.mozilla.org/mozilla-central/rev/22425168f2a9
https://hg.mozilla.org/mozilla-central/rev/e8c2b54c64de
https://hg.mozilla.org/mozilla-central/rev/745bffad22a6