LayoutTests/media/video-loop.html - Issue 898883003: Fixes play seek when user sets loop after ended.

Side by Side Diff: LayoutTests/media/video-loop.html

Issue 898883003: Fixes play seek when user sets loop after ended. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Minor cleanup Created 5 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <html>	2 <html>

3 <head>	3 <head>

4 <script src=media-file.js></script>	4 <script src=media-file.js></script>

5 <script src=video-test.js></script>	5 <script src=video-test.js></script>

6	6

7 <script>	7 <script>

8 var seekCount = 0;	8 var seekCount = 0;

9 var playCount = 0;	9 var playCount = 0;

10	10

(...skipping 36 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
47	47

48 testExpected("mediaElement.currentTime", 0, '>=');	48 testExpected("mediaElement.currentTime", 0, '>=');

49	49

50 // don't use "testExpected()" so we won't log the actual duration as the floating point result may differ with different engines	50 // don't use "testExpected()" so we won't log the actual duration as the floating point result may differ with different engines

51 reportExpected(mediaElement.currentTime < mediaElement.d uration, "mediaElement.currentTime", "<", "mediaElement.duration", mediaElement. currentTime);	51 reportExpected(mediaElement.currentTime < mediaElement.d uration, "mediaElement.currentTime", "<", "mediaElement.duration", mediaElement. currentTime);

52 run("video.loop = false");	52 run("video.loop = false");

53 run("video.currentTime = video.duration - 0.4");	53 run("video.currentTime = video.duration - 0.4");

54 consoleWrite("");	54 consoleWrite("");

55 break;	55 break;

56 case 3:	56 case 3:

57 consoleWrite("<br><em>++ third seek completed, beginning playback for the last time.</em>");	57 consoleWrite("<br><em>++ third seek completed, unset loo p and let play to the end.</em>");
	wolenetz 2015/02/12 22:12:26 nit: s/loop/'loop' nit: s/loop/'loop' chcunningham 2015/02/13 03:37:29 Done. Show quoted text On 2015/02/12 22:12:26, wolenetz wrote: > nit: s/loop/'loop' Done.
58 testExpected("video.paused", true);	58 testExpected("video.paused", true);

59 testExpected("video.ended", false);	59 testExpected("video.ended", false);

60 run("video.play()");	60 run("video.play()");

61 consoleWrite("");	61 consoleWrite("");

62 break;	62 break;

	63 case 4:

	64 consoleWrite("<br><em>++ fourth seek completed.</em>");

	65 // don't use "testExpected()" so we won't log the actual duration as the floating point result may differ with different engines

	66 reportExpected(mediaElement.currentTime < mediaElement.d uration, "mediaElement.currentTime", "<", "mediaElement.duration", mediaElement. currentTime);

	67 testExpected("video.loop", true);

	68 testExpected("video.paused", false);

	69 testExpected("video.ended", false);

	70 // Pause now that test is over to prevent re-firing ende d and annoying debugging experience.
	wolenetz 2015/02/12 22:12:27 nit: change comment to a consoleWrite to make expe nit: change comment to a consoleWrite to make expectation even clearer (perhaps like ..."pausing playback to prevent another 'ended' while ending test") chcunningham 2015/02/13 03:37:29 Done. Show quoted text On 2015/02/12 22:12:27, wolenetz wrote: > nit: change comment to a consoleWrite to make expectation even clearer (perhaps > like ..."pausing playback to prevent another 'ended' while ending test") Done.
	71 run("video.pause()");

	72 consoleWrite("");

	73 endTest();

63 default:	74 default:

64 failTest("Video should have only seeked three times.");	75 failTest("Video should have only seeked four times.");

65 break;	76 break;

66	77

67 }	78 }

68 }	79 }

69	80

70 function ended()	81 function ended()

71 {	82 {

72 consoleWrite("<br><em>++ played to end and stopped.</em>");	83 consoleWrite("<br><em>++ played to end and stopped.</em>");

73 testExpected("video.ended", true);	84 testExpected("video.ended", true);

74	85

75 // don't use "testExpected()" so we won't log the actual duratio n as the floating point result may differ with different engines	86 // don't use "testExpected()" so we won't log the actual duratio n as the floating point result may differ with different engines

76 reportExpected(mediaElement.currentTime == mediaElement.duration , "mediaElement.currentTime", "==", "mediaElement.duration", mediaElement.curren tTime);	87 reportExpected(mediaElement.currentTime == mediaElement.duration , "mediaElement.currentTime", "==", "mediaElement.duration", mediaElement.curren tTime);

77	88

	89 // Testing edge case where we set loop after playback ends. Veri fies fix for crbug.com/364442
	wolenetz 2015/02/12 22:12:26 nit: usually we don't spend code to reference a fi nit: usually we don't spend code to reference a fixed bug in the [test] code (except in chromium for cases like test media files specific to particular regressions). Curious folks can always see the commit history to figure out which bug these lines were added to verify. (also s/loop/'loop'/ if you keep some reference to the attribute in comment here) chcunningham 2015/02/13 03:37:29 Know what you mean, but this comment is pretty tin Show quoted text On 2015/02/12 22:12:26, wolenetz wrote: > nit: usually we don't spend code to reference a fixed bug in the [test] code > (except in chromium for cases like test media files specific to particular > regressions). Curious folks can always see the commit history to figure out > which bug these lines were added to verify. (also s/loop/'loop'/ if you keep > some reference to the attribute in comment here) Know what you mean, but this comment is pretty tiny and the significance of this test is not obvious. I'd like to keep it so no one comes along and deletes what might initially appear to be an over-engineered test case. fs 2015/02/13 11:49:22 This whole interchange seems to suggest that you r Show quoted text On 2015/02/13 03:37:29, chcunningham wrote: > On 2015/02/12 22:12:26, wolenetz wrote: > > nit: usually we don't spend code to reference a fixed bug in the [test] code > > (except in chromium for cases like test media files specific to particular > > regressions). Curious folks can always see the commit history to figure out > > which bug these lines were added to verify. (also s/loop/'loop'/ if you keep > > some reference to the attribute in comment here) > > Know what you mean, but this comment is pretty tiny and the significance of this > test is not obvious. I'd like to keep it so no one comes along and deletes what > might initially appear to be an over-engineered test case. This whole interchange seems to suggest that you really just should make a separate TC for the bug you're fixing. That ought to address both of your concerns (and it's quite possible that this TC shouldn't have been allowed to balloon like this in the first place.) chcunningham 2015/02/13 23:01:43 Done. Show quoted text On 2015/02/13 11:49:22, fs wrote: > On 2015/02/13 03:37:29, chcunningham wrote: > > On 2015/02/12 22:12:26, wolenetz wrote: > > > nit: usually we don't spend code to reference a fixed bug in the [test] code > > > (except in chromium for cases like test media files specific to particular > > > regressions). Curious folks can always see the commit history to figure out > > > which bug these lines were added to verify. (also s/loop/'loop'/ if you keep > > > some reference to the attribute in comment here) > > > > Know what you mean, but this comment is pretty tiny and the significance of > this > > test is not obvious. I'd like to keep it so no one comes along and deletes > what > > might initially appear to be an over-engineered test case. > > This whole interchange seems to suggest that you really just should make a > separate TC for the bug you're fixing. That ought to address both of your > concerns (and it's quite possible that this TC shouldn't have been allowed to > balloon like this in the first place.) Done.
	90 consoleWrite("<br><em>++ with ended == true, set loop and play a gain.</em>");
	wolenetz 2015/02/12 22:12:26 nit: s/loop/'loop' nit: s/loop/'loop' chcunningham 2015/02/13 03:37:29 Done. Show quoted text On 2015/02/12 22:12:26, wolenetz wrote: > nit: s/loop/'loop' Done.
	91 testExpected("video.loop", false);

	92 run("video.loop = true");

	93 testExpected("video.loop", true);

	94 // Playback cannot be "ended" when loop attribute is set; It is simply paused.
	wolenetz 2015/02/12 22:12:26 nit: move this part of this comment to the console nit: move this part of this comment to the consoleWrite to make expectation even more clear. chcunningham 2015/02/13 03:37:29 Done. Show quoted text On 2015/02/12 22:12:26, wolenetz wrote: > nit: move this part of this comment to the consoleWrite to make expectation even > more clear. Done.
	95 // See http://dev.w3.org/html5/spec-preview/media-elements.html# ended-playback
	wolenetz 2015/02/12 22:12:26 nit: No strong feeling here, but I suspect we don' nit: No strong feeling here, but I suspect we don't really need to clarify with link to spec definition of HTMLMediaElement.ended here. chcunningham 2015/02/13 03:37:28 Done. Show quoted text On 2015/02/12 22:12:26, wolenetz wrote: > nit: No strong feeling here, but I suspect we don't really need to clarify with > link to spec definition of HTMLMediaElement.ended here. Done.
	96 testExpected("video.ended", false);
	wolenetz 2015/02/12 22:12:26 nit: there is no explicit check that the video is nit: there is no explicit check that the video is indeed paused, though comment, above, asserts this. add check please. chcunningham 2015/02/13 03:37:29 Done. Show quoted text On 2015/02/12 22:12:26, wolenetz wrote: > nit: there is no explicit check that the video is indeed paused, though comment, > above, asserts this. add check please. Done.
	97 run("video.play()");

78 consoleWrite("");	98 consoleWrite("");

79 endTest();

80 }	99 }

81	100

82 function start()	101 function start()

83 {	102 {

84 findMediaElement();	103 findMediaElement();

85	104

86 consoleWrite("<em>++ Test setting/removing the attribute.</em>") ;	105 consoleWrite("<em>++ Test setting/removing the attribute.</em>") ;

87 testExpected("video.getAttribute('loop')", null);	106 testExpected("video.getAttribute('loop')", null);

88 testExpected("video.loop", false);	107 testExpected("video.loop", false);

89	108

(...skipping 15 matching lines...) Expand all Loading...
105 consoleWrite("");	124 consoleWrite("");

106 }	125 }

107 </script>	126 </script>

108	127

109 </head>	128 </head>

110 <body>	129 <body>

111 <video controls autoplay ></video>	130 <video controls autoplay ></video>

112 <p><b>Test looping by:</b>	131 <p><b>Test looping by:</b>

113 <ol>	132 <ol>

114 <li>Play to end with 'loop' set to true.</li>	133 <li>Play to end with 'loop' set to true.</li>

115 <li>When 'seeked' event fires, verify that time has jumped back and movie is playing.</li>	134 <li>When 'seeked' event fires, verify that time has jumped back and movie is playing.</li>
	wolenetz 2015/02/12 22:12:27 nit: seeked() case 2 doesn't explicitly check that nit: seeked() case 2 doesn't explicitly check that time has jumped back relative to the last known currentTime from app's perspective. (e.g. instead maybe check currentTime < duration - 0.4). Strengthen case 2's verification? chcunningham 2015/02/13 03:37:29 Done. Show quoted text On 2015/02/12 22:12:27, wolenetz wrote: > nit: seeked() case 2 doesn't explicitly check that time has jumped back relative > to the last known currentTime from app's perspective. (e.g. instead maybe check > currentTime < duration - 0.4). Strengthen case 2's verification? Done.
116 <li>Set 'loop' to false and play again.</li>	135 <li>Set 'loop' to false and play again.</li>

117 <li>Verify that 'ended' event fires.</li>	136 <li>Verify that 'ended' event fires.</li>

	137 <li>Set 'loop' to true now that video is ended. Call play again.</li >

	138 <li>Verify 'seeked' event fires, verify that time has jumped back an d movie is playing.</li>

118 </ol>	139 </ol>

119 </p>	140 </p>

120 <script>start()</script>	141 <script>start()</script>

121 </body>	142 </body>

122 </html>	143 </html>

OLD	NEW

« no previous file with comments | « no previous file | LayoutTests/media/video-loop-expected.txt » ('j') | Source/core/html/HTMLMediaElement.h » ('J')