1037321 - Expose AnimationPlayer.playState

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles)]

This was proposed here:

  http://lists.w3.org/Archives/Public/public-fx/2014JulSep/0007.html

And resolved on (with modifications) here:

  http://lists.w3.org/Archives/Public/public-fx/2014JulSep/0013.html

Originally I started implementing this in order to test bug 1037316 but it proved not to be necessary.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Expose AnimationPlayer.playState

WIP patch. I should probably add tests before landing this

Comment 2

[Brian Birtles (:birtles)]

Unassigning this since David might get to this first with his work on playback control.

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8494139 [details] [diff] [review]
Patch

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

Looks good. Is it possible to write the tests as testrunner.js tests? Or are we still missing too many pieces to do that?

Ultimately I'd like to make all this testing cross-browser but that can happen as a follow-up bug if it's not possible yet. (Some of these tests will have to be re-written as part of bug 927349 anyway since that bug will mean new animations end up in the pending state.)

r=birtles with comments addressed.

::: dom/animation/AnimationPlayer.cpp
@@ +56,5 @@
>  
> +AnimationPlayState
> +AnimationPlayer::PlayState() const
> +{
> +  Nullable<double> currentTime = GetCurrentTime();

Maybe just use GetCurrentTimeDuration() here since it should be null in the same circumstances as GetCurrentTime() and saves one level of indirection.

@@ +67,5 @@
> +  }
> +
> +  if (currentTime.Value() >= SourceContentEnd().ToMilliseconds()) {
> +    return AnimationPlayState::Finished;
> +  }

I think we should skip the finished state for now and do all the finishing-related handling in a separate bug. Perhaps just put a FIXME here for now?

@@ +147,5 @@
>  
> +TimeDuration
> +AnimationPlayer::SourceContentEnd() const
> +{
> +  // This computes the end time because we haven't implemented end delay yet.

Actually, the end time includes the end delay so this comment should just be something like, "This doesn't include the end delay since we haven't implemented that yet"

  http://w3c.github.io/web-animations/#end-time

@@ +150,5 @@
> +{
> +  // This computes the end time because we haven't implemented end delay yet.
> +  const AnimationTiming& timing = mSource->Timing();
> +  return timing.mDelay + mSource->GetComputedTiming().mActiveDuration;
> +}

This should account for the case when mSource is null (which, even if it doesn't currently happen, will be possible in the near future).

In that case this should return TimeDuration(0)

  http://w3c.github.io/web-animations/#source-content-end

::: dom/animation/AnimationPlayer.h
@@ +62,5 @@
>    bool IsPaused() const {
>      return mPaused;
>    }
>  
> +  AnimationPlayState PlayState() const;

Can you move this so it appears in the same place in the header file as the corresponding item in the .webidl file? i.e. after GetCurrentTime()?

Comment 5

[David Zbarsky (:dzbarsky)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Comment on attachment 8494139 [details] [diff] [review]
> Patch
> 
> Review of attachment 8494139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Is it possible to write the tests as testrunner.js tests? Or are
> we still missing too many pieces to do that?
> 

I thought we would be able to switch to testrunner after implementing settable currentTime, but I didn't realize that setting the currentTime when the player is paused still advances it, while calling advance_clock doesn't.  We may be able to switch to using rAF instead, but that will slow the test down and make it non-deterministic.  So I'm not sure what we want to do here yet.

> 
> ::: dom/animation/AnimationPlayer.cpp
> @@ +56,5 @@
> >  
> > +AnimationPlayState
> > +AnimationPlayer::PlayState() const
> > +{
> > +  Nullable<double> currentTime = GetCurrentTime();
> 
> Maybe just use GetCurrentTimeDuration() here since it should be null in the
> same circumstances as GetCurrentTime() and saves one level of indirection.
> 

That's not necessary if we get rid of the finished state - but I don't think we should do that.

> @@ +67,5 @@
> > +  }
> > +
> > +  if (currentTime.Value() >= SourceContentEnd().ToMilliseconds()) {
> > +    return AnimationPlayState::Finished;
> > +  }
> 
> I think we should skip the finished state for now and do all the
> finishing-related handling in a separate bug. Perhaps just put a FIXME here
> for now?
> 

I think we can report animations as being in the finished state even if we haven't implemented the updating finished state stuff.  It seems weird to report animations as "running" even if they have finished all iterations.

Comment 6

[Brian Birtles (:birtles)]

(In reply to David Zbarsky (:dzbarsky) from comment #5)
> I thought we would be able to switch to testrunner after implementing
> settable currentTime, but I didn't realize that setting the currentTime when
> the player is paused still advances it, while calling advance_clock doesn't.
> We may be able to switch to using rAF instead, but that will slow the test
> down and make it non-deterministic.  So I'm not sure what we want to do here
> yet.

I think using rAF is fine. Waiting one frame here and there shouldn't slow the test down too significantly. What's the indeterminism you're concerned about?

Comment 7

[Brian Birtles (:birtles)]

Attachment: Implement playState on AnimationPlayer

Hi David, I've updated this patch with the changes we discussed. Can you check
it over to see if it makes sense?

Comment 8

[Brian Birtles (:birtles)]

Attachment: Implement playState on AnimationPlayer

Simplify test case a little

Comment 9

[David Zbarsky (:dzbarsky)]

Comment on attachment 8500249 [details] [diff] [review]
Implement playState on AnimationPlayer

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

Looks good to me.

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8500249 [details] [diff] [review]
Implement playState on AnimationPlayer

Hi Boris, can you check the WebIDL parts of this? Thanks.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8500249 [details] [diff] [review]
Implement playState on AnimationPlayer

We never return "pending"?

r=me on the IDL bits

Comment 12

[Brian Birtles (:birtles)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> We never return "pending"?

Not until bug 927349.

Thanks for the quick review!

Comment 13

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e5016be92f4

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e5016be92f4

Comment 15

[Jean-Yves Perrier [:teoli]]

Doc has been written:
https://developer.mozilla.org/en-US/docs/Web/API/AnimationPlayer.playState
and
https://developer.mozilla.org/en-US/Firefox/Releases/36#Interfaces.2FAPIs.2FDOM