Closed Bug 1037321 Opened 10 years ago Closed 10 years ago

Expose AnimationPlayer.playState

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: birtles, Assigned: dzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Expose AnimationPlayer.playState (obsolete) — Splinter Review
WIP patch. I should probably add tests before landing this
Blocks: 1070744
Unassigning this since David might get to this first with his work on playback control.
Assignee: birtles → nobody
Status: ASSIGNED → NEW
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → dzbarsky
Attachment #8454273 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8494139 - Flags: review?(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()?
Attachment #8494139 - Flags: review?(birtles) → review+
(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.
(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?
No longer blocks: 998245
Depends on: 1070745
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)
Attachment #8494139 - Attachment is obsolete: true
Assignee: dzbarsky → birtles
Simplify test case a little
Attachment #8500249 - Flags: review?(dzbarsky)
Attachment #8500248 - Attachment is obsolete: true
Attachment #8500248 - Flags: review?(dzbarsky)
Assignee: birtles → dzbarsky
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+
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+
(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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.