Add AnimationPlayer.ready

RESOLVED FIXED in mozilla37

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 6 obsolete attachments)

2.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.98 KB, patch
heycam
: review+
Details | Diff | Splinter Review
22.89 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.81 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.89 KB, patch
Details | Diff | Splinter Review
5.52 KB, patch
Details | Diff | Splinter Review
2.21 KB, patch
heycam
: review+
Details | Diff | Splinter Review
33.82 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.01 KB, patch
heycam
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 2

4 years ago
Created attachment 8528192 [details] [diff] [review]
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!
Attachment #8528192 - Flags: review?(bugs)
(Assignee)

Comment 3

4 years ago
Created attachment 8528193 [details] [diff] [review]
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.
Attachment #8528193 - Flags: review?(bugs)
(Assignee)

Comment 4

4 years ago
Created attachment 8528194 [details] [diff] [review]
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.
Attachment #8528194 - Flags: review?(bugs)
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.
Attachment #8528192 - Flags: review?(bugs) → review+
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.
Attachment #8528193 - Flags: review?(bugs) → review+
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 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?
Attachment #8528194 - Flags: review?(bugs) → review-
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
Created attachment 8528764 [details] [diff] [review]
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.
Attachment #8528194 - Attachment is obsolete: true
Attachment #8528764 - Flags: review?(bugs)
(Assignee)

Comment 11

4 years ago
Created attachment 8528839 [details] [diff] [review]
part 4 - Create and resolve new ready promises on play()
Attachment #8528839 - Flags: review?(cam)
(Assignee)

Comment 12

4 years ago
Created attachment 8528842 [details] [diff] [review]
part 5 - Factor out common animation test methods into testcommon.js
Attachment #8528842 - Flags: review?(cam)
(Assignee)

Comment 13

4 years ago
Created attachment 8528843 [details] [diff] [review]
part 6 - Use step_func inside Promise callbacks
Attachment #8528843 - Flags: review?(cam)
(Assignee)

Comment 14

4 years ago
Created attachment 8528853 [details] [diff] [review]
part 7 - Update AnimationPlayer tests to wait on ready promise
Attachment #8528853 - Flags: review?(cam)
(Assignee)

Comment 15

4 years ago
Created attachment 8528857 [details] [diff] [review]
part 8 - Add tests for AnimationPlayer.ready
Attachment #8528857 - Flags: review?(cam)
(Assignee)

Comment 16

4 years ago
Created attachment 8528864 [details] [diff] [review]
part 9 - Make getting AnimationPlayer.ready flush for CSS Animation players
Attachment #8528864 - Flags: review?(cam)
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()'
Attachment #8528764 - Flags: review?(bugs) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 8536258 [details] [diff] [review]
part 1 - Make AnimationTimeline::GetParentObject return an nsIGlobalObject

Updated with review feedback addressed
(Assignee)

Updated

4 years ago
Attachment #8528192 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
Created attachment 8536260 [details] [diff] [review]
part 3 - Add AnimationPlayer.ready promise

Updated with review feedback addressed
(Assignee)

Updated

4 years ago
Attachment #8528764 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Created attachment 8536261 [details] [diff] [review]
part 4 - Create and resolve new ready promises on play()

Fix bitrot
Attachment #8536261 - Flags: review?(cam)
(Assignee)

Comment 21

4 years ago
Created attachment 8536262 [details] [diff] [review]
part 5 - Factor out common animation test methods into testcommon.js

Add patch description
Attachment #8536262 - Flags: review?(cam)
(Assignee)

Updated

4 years ago
Attachment #8528842 - Attachment is obsolete: true
Attachment #8528842 - Flags: review?(cam)
(Assignee)

Updated

4 years ago
Attachment #8528839 - Attachment is obsolete: true
Attachment #8528839 - Flags: review?(cam)
(Assignee)

Comment 22

4 years ago
Created attachment 8536263 [details] [diff] [review]
part 9 - Make getting AnimationPlayer.ready flush for CSS Animation players

Fix bitrot
Attachment #8536263 - Flags: review?(cam)
(Assignee)

Updated

4 years ago
Attachment #8528864 - Attachment is obsolete: true
Attachment #8528864 - Flags: review?(cam)
Attachment #8536261 - Flags: review?(cam) → review+
Attachment #8536262 - Flags: review?(cam) → review+
Attachment #8528843 - Flags: review?(cam) → review+
Attachment #8528853 - Flags: review?(cam) → review+
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?
Attachment #8528857 - Flags: review?(cam) → review+
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?
Attachment #8536263 - Flags: review?(cam) → review+
(Assignee)

Comment 25

4 years ago
(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.
OK.  Can you comment in here to that effect (just in case someone goes looking for the corresponding transitions style flush)?
(Assignee)

Comment 28

4 years ago
(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.
You need to log in before you can comment on or make changes to this bug.