Expose AnimationPlayer.playState

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: birtles, Assigned: dzbarsky)

Tracking

({dev-doc-complete})

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 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

5 years ago
Blocks: 998245
Reporter

Comment 1

5 years ago
WIP patch. I should probably add tests before landing this
Assignee

Updated

5 years ago
Blocks: 1070744
Reporter

Comment 2

5 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

5 years ago
Blocks: 927349
Assignee

Comment 3

5 years ago
Posted patch Patch (obsolete) — Splinter Review
Assignee: nobody → dzbarsky
Attachment #8454273 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8494139 - Flags: review?(birtles)
Reporter

Comment 4

5 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

5 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

5 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

5 years ago
No longer blocks: 998245
Depends on: 1070745
Reporter

Comment 7

5 years ago
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

5 years ago
Attachment #8494139 - Attachment is obsolete: true
Reporter

Updated

5 years ago
Assignee: dzbarsky → birtles
Reporter

Comment 8

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

Updated

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

Updated

5 years ago
Assignee: birtles → dzbarsky
Reporter

Updated

5 years ago
Blocks: 1078119
Assignee

Comment 9

5 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

5 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

5 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.