Expose AnimationPlayer.playState

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: birtles, Assigned: dzbarsky)

Tracking

({dev-doc-complete})

Trunk
mozilla36
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

4 years ago
Blocks: 998245
(Reporter)

Comment 1

4 years ago
Created attachment 8454273 [details] [diff] [review]
Expose AnimationPlayer.playState

WIP patch. I should probably add tests before landing this
(Assignee)

Updated

4 years ago
Blocks: 1070744
(Reporter)

Comment 2

4 years ago
Unassigning this since David might get to this first with his work on playback control.
Assignee: birtles → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

4 years ago
Blocks: 927349
(Assignee)

Comment 3

4 years ago
Created attachment 8494139 [details] [diff] [review]
Patch
Assignee: nobody → dzbarsky
Attachment #8454273 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8494139 - Flags: review?(birtles)
(Reporter)

Comment 4

4 years ago
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()?
Attachment #8494139 - Flags: review?(birtles) → review+
(Assignee)

Comment 5

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

Comment 6

4 years ago
(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?
(Reporter)

Updated

4 years ago
No longer blocks: 998245
Depends on: 1070745
(Reporter)

Comment 7

4 years ago
Created attachment 8500248 [details] [diff] [review]
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?
Attachment #8500248 - Flags: review?(dzbarsky)
(Reporter)

Updated

4 years ago
Attachment #8494139 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Assignee: dzbarsky → birtles
(Reporter)

Comment 8

4 years ago
Created attachment 8500249 [details] [diff] [review]
Implement playState on AnimationPlayer

Simplify test case a little
Attachment #8500249 - Flags: review?(dzbarsky)
(Reporter)

Updated

4 years ago
Attachment #8500248 - Attachment is obsolete: true
Attachment #8500248 - Flags: review?(dzbarsky)
(Reporter)

Updated

4 years ago
Assignee: birtles → dzbarsky
(Reporter)

Updated

4 years ago
Blocks: 1078119
(Assignee)

Comment 9

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

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

Looks good to me.
Attachment #8500249 - Flags: review?(dzbarsky) → review+
(Reporter)

Comment 10

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

Hi Boris, can you check the WebIDL parts of this? Thanks.
Attachment #8500249 - Flags: review?(bzbarsky)
Comment on attachment 8500249 [details] [diff] [review]
Implement playState on AnimationPlayer

We never return "pending"?

r=me on the IDL bits
Attachment #8500249 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 12

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

Not until bug 927349.

Thanks for the quick review!
https://hg.mozilla.org/mozilla-central/rev/3e5016be92f4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.