1166164 - Update the finishing behavior of Animation objects

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles)]

We recently resolved[1] to fix a number of inconsistencies in the way finishing behavior works for Animation objects.

In attempting to implement these changes I've discovered some other existing issues with the spec and our implementation.

[1] https://lists.w3.org/Archives/Public/public-fx/2015AprJun/0038.html

Comment 1

[Brian Birtles (:birtles)]

Attachment: MozReview Request: bz://1166164/birtles

/r/8999 - Bug 1166164 part 1 - Make setting the current time complete a pending pause, not abort it
/r/9001 - Bug 1166164 part 2 - Make UpdateFinishedState take a non-defaulted enum
/r/9003 - Bug 1166164 part 3 - Resolve start time on finish()
/r/9005 - Bug 1166164 part 4 - Make finished promise not resolve when paused
/r/9007 - Bug 1166164 part 5 - Make play() throw when it should seek to the end of an infinite effect
/r/9009 - Bug 1166164 part 6 - Make pausing from idle set the current time
/r/9011 - Bug 1166164 part 7 - Call pause directly when creating an initially-paused animation
/r/9013 - Bug 1166164 part 8 - Drop a few references to players

Pull down these commits:

hg pull -r fadb9581d971d6693e59187f5474a4d6b36651a3 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Could you explain why play() and pause() need [Throws]?
That is not in the spec, nor in that email to the mailing list.

Comment 3

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9001/#review7673

::: dom/animation/Animation.h:303
(Diff revision 1)
> +  enum class SeekBehavior {
> +    NoSeek,
> +    DidSeek

To better match the spec text I'd suggest calling the enum "SeekFlag" and the values "Seek" and "NoSeek".

::: dom/animation/Animation.cpp:92
(Diff revision 1)
> -  UpdateTiming();
> +  UpdateTiming(SeekBehavior::NoSeek);

It would be useful to have a comment here explaining why this is not a seek, since initially one might think it effectively is.

Comment 4

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9005/#review7703

::: dom/animation/Animation.cpp
(Diff revision 1)
> -Animation::IsFinished() const

Yay!

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Could you explain why play() and pause() need [Throws]?
> That is not in the spec, nor in that email to the mailing list.

Looks like part of the spec is out of date. The exception is mentioned in the prose that describes the algorithm for play() and pause() though. See the mention of InvalidStateError in these sections:

http://w3c.github.io/web-animations/#play-an-animation
http://w3c.github.io/web-animations/#pause-an-animation

Comment 6

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/8999/#review7705

Ship It!

Comment 7

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9001/#review7707

Ship It!

Comment 8

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9003/#review7709

Ship It!

Comment 9

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9005/#review7711

Ship It!

Comment 10

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9007/#review7713

Ship It!

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8607373 [details]
MozReview Request: bz://1166164/birtles

r+ for the webidl (I assume that is what the request was about.)

Comment 12

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9009/#review7715

Ship It!

::: layout/style/nsAnimationManager.cpp:104
(Diff revision 1)
> +    NS_WARNING("Unexpected exception pausing animation");

Can you expand on this message by tacking on " - silently failing" or something?

Comment 13

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9011/#review7717

Ship It!

Comment 14

[Jonathan Watt [:jwatt]]

https://reviewboard.mozilla.org/r/9013/#review7719

Ship It!

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8607373 [details]
MozReview Request: bz://1166164/birtles

https://reviewboard.mozilla.org/r/8997/#review7721

Ship It!

Comment 16

[Brian Birtles (:birtles)]

(In reply to Jonathan Watt [:jwatt] from comment #5)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > Could you explain why play() and pause() need [Throws]?
> > That is not in the spec, nor in that email to the mailing list.
> 
> Looks like part of the spec is out of date. The exception is mentioned in
> the prose that describes the algorithm for play() and pause() though. See
> the mention of InvalidStateError in these sections:
> 
> http://w3c.github.io/web-animations/#play-an-animation
> http://w3c.github.io/web-animations/#pause-an-animation

Which part of the spec is out of date? It's mentioned in those two sections Jonathan pointed too. Where else does it need to be mentioned?

As far as I can tell, WebIDL doesn't have [Throws] extended attributes anymore, that's specific to our bindings.

Comment 17

[Brian Birtles (:birtles)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8607373 [details]
> MozReview Request: bz://1166164/birtles
> 
> r+ for the webidl (I assume that is what the request was about.)

Yes, sorry, I haven't worked out how to indicate that in reviewboard yet.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Brian Birtles (:birtles) from comment #17)
> 
> Yes, sorry, I haven't worked out how to indicate that in reviewboard yet.
me neither. I end up downloading the diffs and looking at them in an editor because that is how I like
to see the patches. (I don't like the diff-view) :)

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4b781ca3ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/7788741c4c1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/862ed7c76925
https://hg.mozilla.org/integration/mozilla-inbound/rev/68ac3fa4a647
https://hg.mozilla.org/integration/mozilla-inbound/rev/468f0935e2d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/70c5890d9577
https://hg.mozilla.org/integration/mozilla-inbound/rev/98d7ad6eacbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2257257525

Comment 20

[Brian Birtles (:birtles)]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> https://reviewboard.mozilla.org/r/9001/#review7673
> 
> ::: dom/animation/Animation.h:303
> (Diff revision 1)
> > +  enum class SeekBehavior {
> > +    NoSeek,
> > +    DidSeek
> 
> To better match the spec text I'd suggest calling the enum "SeekFlag" and
> the values "Seek" and "NoSeek".

I renamed the enum to SeekFlag as suggested but I left the values as NoSeek and DidSeek since the spec refers to the "did seek" flag and it's only set when a seek happened in the past. I'm happy to push a follow-up patch to change this if you still think it's odd.

> ::: dom/animation/Animation.cpp:92
> (Diff revision 1)
> > -  UpdateTiming();
> > +  UpdateTiming(SeekBehavior::NoSeek);
> 
> It would be useful to have a comment here explaining why this is not a seek,
> since initially one might think it effectively is.

Sorry, I overlooked this comment. (Reviewboard makes it really hard to find the bit of code a comment is associated with but I believe this is referring to SetStartTime.) I'm not sure exactly what's surprising here; I guess setting the start time feels like seeking?

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae4b781ca3ca
https://hg.mozilla.org/mozilla-central/rev/7788741c4c1b
https://hg.mozilla.org/mozilla-central/rev/862ed7c76925
https://hg.mozilla.org/mozilla-central/rev/68ac3fa4a647
https://hg.mozilla.org/mozilla-central/rev/468f0935e2d3
https://hg.mozilla.org/mozilla-central/rev/70c5890d9577
https://hg.mozilla.org/mozilla-central/rev/98d7ad6eacbc
https://hg.mozilla.org/mozilla-central/rev/cf2257257525

Comment 22

[Jonathan Watt [:jwatt]]

(In reply to Brian Birtles (:birtles) from comment #20)
> I renamed the enum to SeekFlag as suggested but I left the values as NoSeek
> and DidSeek since the spec refers to the "did seek" flag and it's only set
> when a seek happened in the past. I'm happy to push a follow-up patch to
> change this if you still think it's odd.

That's fine. On a bit more reflection it's the "NoSeek" I have a bit of an issue with since it sounds imperative rather than informative about something that has already happened. Not a big deal though, and I don't have a suggestion other than DidNotSeek or something. Up to you whether you agree enough to write a follow-up.

> Sorry, I overlooked this comment. (Reviewboard makes it really hard to find
> the bit of code a comment is associated with but I believe this is referring
> to SetStartTime.) I'm not sure exactly what's surprising here; I guess
> setting the start time feels like seeking?

Yeah, Reviewboard is a bit annoying in that regard.

Comment 24

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1166164 part 1 - Make setting the current time complete a pending pause, not abort it

Comment 25

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1166164 part 2 - Make UpdateFinishedState take a non-defaulted enum

Comment 26

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1166164 part 3 - Resolve start time on finish()

Comment 27

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1166164 part 4 - Make finished promise not resolve when paused

Comment 28

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1166164 part 5 - Make play() throw when it should seek to the end of an infinite effect

Comment 29

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1166164 part 6 - Make pausing from idle set the current time

Comment 30

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1166164 part 7 - Call pause directly when creating an initially-paused animation

Comment 31

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1166164 part 8 - Drop a few references to players