1150064 - Implement Animation.finish()

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

Now that the finishing behavior (bug 1074630) is implemented, we can implement the Animation.finish() function.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: WIP

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Implement the Animation.finish() method

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: Tests

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8595418 [details] [diff] [review]
Implement the Animation.finish() method

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

Looks good to me. We'll probably change this behavior so that finish also unpauses (see https://lists.w3.org/Archives/Public/public-fx/2015AprJun/0013.html) but for now this is good.

This needs DOM peer review.

::: dom/animation/Animation.cpp
@@ +241,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return;
> +  }
> +
> +  // https://w3c.github.io/web-animations/#finish-an-animation

Put this comment at the top of the function?

::: dom/animation/Animation.h
@@ +95,5 @@
>    virtual Promise* GetReady(ErrorResult& aRv);
>    virtual Promise* GetFinished(ErrorResult& aRv);
>    virtual void Play(LimitBehavior aLimitBehavior);
>    virtual void Pause();
> +  virtual void Finish(ErrorResult& aRv);

Please match the ordering in the IDL (i.e. before Play()/Pause()).

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 8595418 [details] [diff] [review]
Implement the Animation.finish() method

For review of the addition of the |void finish ()| method to the webidl.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8595420 [details] [diff] [review]
Tests

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

This looks good but:

1. I think test_animation-finished.html should test the 'finished' promise. I think we should add test_animation-finish.html to test the 'finish()' method.

2. We need the following additional tests:
* Test for the second case that produces an InvalidStateError. Namely, when the effect end is infinity (and playback rate > 0). You should be able to get there by setting animation-iteration-count: infinite.
* Test that seeking *past* the effect end then calling finish() seeks back to the effect end
* Likewise when playbackRate < 0, seeking to, say, -5000 then calling finish() should seek back to 0.
* Test that with a running animation (i.e. wait on animation.ready) with no forwards fill, that immediately after calling finish() the computed style of the target properties are reset

Apart from that it looks great but I probably want to check the additional tests if they're going to be part of this same patch.

::: dom/animation/test/css-animations/test_animation-finished.html
@@ +346,5 @@
> +  }).then(t.step_func(function() {
> +    animation.finish();
> +    assert_equals(animation.playState, 'paused',
> +                  'The play state of a paused animation should remain ' +
> +                  '"paused" even after finish() is called');

I don't quite understand this test. Why do we wait after calling pause() but before calling finish()? Should we be waiting after calling finish() instead?

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: tests

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8595715 [details] [diff] [review]
tests

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

Looks good.

::: dom/animation/test/css-animations/test_animation-finish.html
@@ +123,5 @@
> +                  'should be set back to zero');
> +    t.done();
> +  }));
> +}, 'Test finishing of reversed animation with a current time past the effect ' +
> +   'end');

Nit: This should probably be "... with a current time less than zero".

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7247ca604c6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d7088b556a

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7247ca604c6b
https://hg.mozilla.org/mozilla-central/rev/a4d7088b556a