Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
555063002:
Web Animations: Add tests for player state transitions and fix discovered issues (Closed)
|
Created:
6 years, 3 months ago by dstockwell Modified:
6 years, 3 months ago CC:
dstockwell, darktears, blink-reviews, blink-reviews-animation_chromium.org, Eric Willigers, Mike Lawther (Google), rjwright, shans, Steve Block, Timothy Loh Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionWeb Animations: Add tests for player state transitions and fix discovered issues
BUG=396372, 410229
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181716
Patch Set 1 #
Total comments: 11
Patch Set 2 : Setting current time shouldn't force a start time. #Patch Set 3 : Finish shouldn't exit idle. #
Total comments: 7
Patch Set 4 : Rebase. Address comments. #
Messages
Total messages: 16 (4 generated)
dstockwell@chromium.org changed reviewers: + shans@chromium.org
https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... File LayoutTests/web-animations-api/player-state-changes.html (right): https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:78: }, "running"); This is a bit of a nonsense test - all of the assertions here are either established or tested directly in runningPlayer(). https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:123: assert_equals(player.playState, 'finished'); I'm not sure that this makes sense. If paused and finished are independent, should idle and finished be independent too? i.e. should there be a ghost currentTime while idle, and calling finish() or setting currentTime updates this, but you're still idle unless play() is called? Or should there be a separate activate() method? https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:140: }, "idle -> set currentTime"); why should this act differently to finish()? https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:196: }, "pending startTime -> set currentTime"); This still feels a bit strange. I think setting current time should force start time, and exit pending state. When you have a pending start time it means you're waiting for the compositor to tell you when it started in order to (effectively) make the currentTime valid. When the currentTime is set though, it's automatically valid, and the startTime is known by computation. Shouldn't this exit pending state? https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:252: }, "pending startTime & currentTime -> set currentTime"); pending startTime & currentTime should be a weaker state than pending startTime. So setting currentTime here should not result in 'running' if it results in 'pending' above.
https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... File LayoutTests/web-animations-api/player-state-changes.html (right): https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:78: }, "running"); On 2014/09/09 09:43:10, shans wrote: > This is a bit of a nonsense test - all of the assertions here are either > established or tested directly in runningPlayer(). These are testing that runningPlayer() and friends are establishing players in the expected states. The assertion in idlePlayer() isn't meant to be there. https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:196: }, "pending startTime -> set currentTime"); On 2014/09/09 09:43:10, shans wrote: > This still feels a bit strange. I think setting current time should force start > time, and exit pending state. When you have a pending start time it means you're > waiting for the compositor to tell you when it started in order to (effectively) > make the currentTime valid. > > When the currentTime is set though, it's automatically valid, and the startTime > is known by computation. Shouldn't this exit pending state? The time at which we can actually start animating isn't known -- this is how we planned to support soft start, eg: p = animate(); p.currentTime = 1000; https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:252: }, "pending startTime & currentTime -> set currentTime"); On 2014/09/09 09:43:10, shans wrote: > pending startTime & currentTime should be a weaker state than pending startTime. > So setting currentTime here should not result in 'running' if it results in > 'pending' above. Yep, this one seems wrong.
https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... File LayoutTests/web-animations-api/player-state-changes.html (right): https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:123: assert_equals(player.playState, 'finished'); On 2014/09/09 at 09:43:10, shans wrote: > I'm not sure that this makes sense. If paused and finished are independent, should idle and finished be independent too? > > i.e. should there be a ghost currentTime while idle, and calling finish() or setting currentTime updates this, but you're still idle unless play() is called? > > Or should there be a separate activate() method? OK, if finish() is just seek, then finish() should not do anything while idle -- the same as setting currentTime/startTime. https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:252: }, "pending startTime & currentTime -> set currentTime"); On 2014/09/09 at 09:59:49, dstockwell wrote: > On 2014/09/09 09:43:10, shans wrote: > > pending startTime & currentTime should be a weaker state than pending startTime. > > So setting currentTime here should not result in 'running' if it results in > > 'pending' above. > > Yep, this one seems wrong. Fixed. https://codereview.chromium.org/555063002/diff/1/LayoutTests/web-animations-a... LayoutTests/web-animations-api/player-state-changes.html:252: }, "pending startTime & currentTime -> set currentTime"); On 2014/09/09 at 09:59:49, dstockwell wrote: > On 2014/09/09 09:43:10, shans wrote: > > pending startTime & currentTime should be a weaker state than pending startTime. > > So setting currentTime here should not result in 'running' if it results in > > 'pending' above. > > Yep, this one seems wrong. Fixed.
dstockwell@chromium.org changed reviewers: + alancutter@chromium.org
Alan, can you take a look? I've resolved the issues that Shane pointed out.
https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... File LayoutTests/web-animations-api/player-state-changes.html (right): https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... LayoutTests/web-animations-api/player-state-changes.html:46: } We should add assertions to all these helper functions to be sure they're returning players in the state we expect and to help me comprehend all these tests. :) e.g. assert_equals(player.playState, 'idle'); assert_equals(player.startTime, null); assert_equals(player.currentTime, 0);
https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... File LayoutTests/web-animations-api/player-state-changes.html (right): https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... LayoutTests/web-animations-api/player-state-changes.html:46: } On 2014/09/10 03:59:42, alancutter wrote: > We should add assertions to all these helper functions to be sure they're > returning players in the state we expect and to help me comprehend all these > tests. :) > e.g. > assert_equals(player.playState, 'idle'); > assert_equals(player.startTime, null); > assert_equals(player.currentTime, 0); That's what the tests immediately below are for.
lgtm https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... File LayoutTests/web-animations-api/player-state-changes.html (right): https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... LayoutTests/web-animations-api/player-state-changes.html:46: } On 2014/09/10 04:01:10, dstockwell wrote: > On 2014/09/10 03:59:42, alancutter wrote: > > We should add assertions to all these helper functions to be sure they're > > returning players in the state we expect and to help me comprehend all these > > tests. :) > > e.g. > > assert_equals(player.playState, 'idle'); > > assert_equals(player.startTime, null); > > assert_equals(player.currentTime, 0); > > That's what the tests immediately below are for. Acknowledged. In that case looks like idlePlayer() has a redundant assert. https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... LayoutTests/web-animations-api/player-state-changes.html:140: player.startTime = 1000; It looks like pause(), finish(), cancel() and seeking have no effect on idle players. I wonder if these actions should throw exceptions instead. https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... LayoutTests/web-animations-api/player-state-changes.html:196: player.startTime = document.timeline.currentTime; Wouldn't hurt to use "document.timeline.currentTime - 1000" to test that currentTime updates correctly here similar to "running -> set startTime".
https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... File LayoutTests/web-animations-api/player-state-changes.html (right): https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... LayoutTests/web-animations-api/player-state-changes.html:46: } On 2014/09/10 at 07:27:05, alancutter wrote: > On 2014/09/10 04:01:10, dstockwell wrote: > > On 2014/09/10 03:59:42, alancutter wrote: > > > We should add assertions to all these helper functions to be sure they're > > > returning players in the state we expect and to help me comprehend all these > > > tests. :) > > > e.g. > > > assert_equals(player.playState, 'idle'); > > > assert_equals(player.startTime, null); > > > assert_equals(player.currentTime, 0); > > > > That's what the tests immediately below are for. > > Acknowledged. > In that case looks like idlePlayer() has a redundant assert. Removed. https://codereview.chromium.org/555063002/diff/40001/LayoutTests/web-animatio... LayoutTests/web-animations-api/player-state-changes.html:196: player.startTime = document.timeline.currentTime; On 2014/09/10 at 07:27:05, alancutter wrote: > Wouldn't hurt to use "document.timeline.currentTime - 1000" to test that currentTime updates correctly here similar to "running -> set startTime". Done.
shane.stephens@gmail.com changed reviewers: + shane.stephens@gmail.com
lgtm
The CQ bit was checked by dstockwell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dstockwell@chromium.org/555063002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 181716 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
