third_party/WebKit/LayoutTests/media/media-play-promise.html - Issue 1576283003: Have HTMLMediaElement::play() return a Promise.

Side by Side Diff: third_party/WebKit/LayoutTests/media/media-play-promise.html

Issue 1576283003: Have HTMLMediaElement::play() return a Promise. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: hiroshige comments Created 4 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

« no previous file with comments | « third_party/WebKit/LayoutTests/media/W3C/video/paused/paused_true_during_pause-expected.txt ('k') | third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt » ('j') | third_party/WebKit/Source/core/dom/DOMException.cpp » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <html>

	2 <head>

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

	4 <script src=video-test.js></script>
	philipj_slow 2016/02/22 06:34:24 Can you add a TODO(mlamouri) in the style of https Can you add a TODO(mlamouri) in the style of https://codereview.chromium.org/1715303002/, assuming you approve? It currently takes mindreader skills to know that I have any kind of ambition to move media tests over to testharness.js and upstream to w-p-t. mlamouri (slow - plz ping) 2016/02/23 16:48:59 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Can you add a TODO(mlamouri) in the style of https://codereview.chromium.org/1715303002/, assuming you approve? It currently takes mindreader skills to know that I have any kind of ambition to move media tests over to testharness.js and upstream to w-p-t. Done.
	5

	6 <script>

	7 // This is testing the behavior of play() with regards to the returned

	8 // promise. This test file is creating a small framework in order to be able

	9 // to test different cases easily and independently of each other.

	10 //

	11 // All tests have to be part of the TESTS array. When the page is loaded,

	12 // first function in the array is ran. A test is considered done when the
	philipj_slow 2016/02/22 06:34:24 s/ran/run/ here and below s/ran/run/ here and below mlamouri (slow - plz ping) 2016/02/23 16:48:59 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > s/ran/run/ here and below Done.
	13 // promise returned by mediaElement.play() is resolved or rejected. Each

	14 // test then needs to call play() once which wraps this logic. When a test

	15 // is finished, the next test in the array is ran until the entire array

	16 // was processed.

	17 //

	18 // Each test should start by prting its name in order to facilitate reading
	philipj_slow 2016/02/22 06:34:24 s/prting/printing/ s/prting/printing/ mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > s/prting/printing/ Done.
	19 // the output.

	20

	21 function runNextTestOrFinish()

	22 {

	23 currentTest++;

	24 if (currentTest >= TESTS.length) {

	25 endTest();

	26 return;

	27 }

	28

	29 consoleWrite("");

	30 TESTS[currentTest]();

	31 }

	32

	33 function play()

	34 {

	35 return mediaElement.play().then(function() {
	philipj_slow 2016/02/22 06:34:24 The return value is never used, skip return to mak The return value is never used, skip return to make that clear. mlamouri (slow - plz ping) 2016/02/23 16:48:59 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > The return value is never used, skip return to make that clear. Done.
	36 consoleWrite("Promise Resolved");
	philipj_slow 2016/02/22 06:34:24 Verify arguments.length == 1 and arguments[0]===un Verify arguments.length == 1 and arguments[0]===undefined, if that is in fact what happens. The spec does say "resolve it with undefined" and it looks like script-created promises that resolve with no argument still call the callback with 1 argument (undefined). Also the capitalization of Resolved and Failed looks a bit random in the output. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Verify arguments.length == 1 and arguments[0]===undefined, if that is in fact what happens. The spec does say "resolve it with undefined" and it looks like script-created promises that resolve with no argument still call the callback with 1 argument (undefined). Done. Show quoted text > Also the capitalization of Resolved and Failed looks a bit random in the output. Done.
	37 }, function(e) {

	38 consoleWrite("Promise Failed with " + e.name);
	philipj_slow 2016/02/22 06:34:24 Verify arguments.length == 1, e instanceof DOMExce Verify arguments.length == 1, e instanceof DOMException and also print e.message. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Verify arguments.length == 1, e instanceof DOMException and also print e.message. Done.
	39 }).then(runNextTestOrFinish);

	40 }

	41

	42 function playWithUserGesture()

	43 {

	44 if (!window.eventSender) {
	philipj_slow 2016/02/22 06:34:24 If we're not running under LayoutTests we will hav If we're not running under LayoutTests we will have already choked on internals. Maybe bail out in start() if that's valuable? mlamouri (slow - plz ping) 2016/02/23 16:48:59 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > If we're not running under LayoutTests we will have already choked on internals. Maybe bail out in start() if that's valuable? Done.
	45 failTest("No window.eventSender");

	46 return;

	47 }

	48

	49 var target = document.querySelector("p");

	50 target.onclick = function() {

	51 play();

	52 target.onclick = null;

	53 };

	54

	55 var boundingRect = target.getBoundingClientRect();

	56 var x = boundingRect.left + (boundingRect.width / 2);

	57 var y = boundingRect.top + (boundingRect.height / 2);

	58

	59 eventSender.mouseMoveTo(x, y);

	60 eventSender.mouseDown();

	61 eventSender.mouseUp();

	62 }

	63

	64 var currentTest = -1;

	65

	66 var TESTS = [

	67 // Test that play() on an element that is currently loading returns a

	68 // promise which resolves successfuly.

	69 function playLoading()

	70 {

	71 consoleWrite("playLoading()");

	72 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	73

	74 run("mediaElement = document.createElement('audio')");

	75 var mediaFile = findMediaFile("audio", "content/test");

	76 run("mediaElement.src = '" + mediaFile + "'");

	77

	78 waitForEvent('playing');

	79 play();
	philipj_slow 2016/02/22 06:34:24 Verify the conditions for "currently loading" that Verify the conditions for "currently loading" that this is intended to test. Unless there is a bug, networkState is NETWORK_NO_SOURCE here, and it's not really true that it's loading, the src attribute hasn't even been inspected yet, rather we're inside the resource selection algorithm awaiting a stable state (step 4). Roughly speaking, I think there should be one test per place that the spec either resolves or rejects promises: 1. Two for NotAllowedError and NotSupportedError rejections at the top of play() 2. One for "The media element is already playing" resolving towards the end of play() 3. Four for "notify about playing", one for each call site 4. One for the media element load algorithm rejection 5. One for the dedicated media source failure steps 6. One for the internal pause steps rejection This playLoading() test doesn't fall into any of these buckets, so unless it's anticipating a particular bug I think it would be fine to remove it. It could probably be tweaked into one of the "notify about playing" cases, namely the one for "If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready state is HAVE_FUTURE_DATA" mlamouri (slow - plz ping) 2016/02/23 16:49:00 Already there. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Verify the conditions for "currently loading" that this is intended to test. Unless there is a bug, networkState is NETWORK_NO_SOURCE here, and it's not really true that it's loading, the src attribute hasn't even been inspected yet, rather we're inside the resource selection algorithm awaiting a stable state (step 4). > > Roughly speaking, I think there should be one test per place that the spec either resolves or rejects promises: > 1. Two for NotAllowedError and NotSupportedError rejections at the top of play() Already there. Show quoted text > 2. One for "The media element is already playing" resolving towards the end of play() Added. Show quoted text > 3. Four for "notify about playing", one for each call site Done. Show quoted text > 4. One for the media element load algorithm rejection Already there. Show quoted text > 5. One for the dedicated media source failure steps Already there. Show quoted text > 6. One for the internal pause steps rejection Already there. Show quoted text > This playLoading() test doesn't fall into any of these buckets, so unless it's anticipating a particular bug I think it would be fine to remove it. It could probably be tweaked into one of the "notify about playing" cases, namely the one for "If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready state is HAVE_FUTURE_DATA" Done. Kind of. I think this test was already testing this case. Not sure how to make it more obvious.
	80 },

	81

	82 // Test that play() on an element that is already loaded returns a
	philipj_slow 2016/02/22 06:34:24 This can be rephrased as testing the "notify about This can be rephrased as testing the "notify about playing" case inside play(). Waiting for canplay and calling the test playWhenCanPlay would make the test as strict possible. For this test, because the promise is resolved while still inside play(), verify that this is the case with a script-created promise right after the call to play() that is expected to be resolved after. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done, apart from the promise thing. The play() pro Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > This can be rephrased as testing the "notify about playing" case inside play(). Waiting for canplay and calling the test playWhenCanPlay would make the test as strict possible. > > For this test, because the promise is resolved while still inside play(), verify that this is the case with a script-created promise right after the call to play() that is expected to be resolved after. Done, apart from the promise thing. The play() promise is resolved in a task so it doesn't sound right. philipj_slow 2016/02/24 09:38:27 Sorry about that, you're right. Show quoted text On 2016/02/23 16:49:00, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > This can be rephrased as testing the "notify about playing" case inside > play(). Waiting for canplay and calling the test playWhenCanPlay would make the > test as strict possible. > > > > For this test, because the promise is resolved while still inside play(), > verify that this is the case with a script-created promise right after the call > to play() that is expected to be resolved after. > > Done, apart from the promise thing. The play() promise is resolved in a task so > it doesn't sound right. Sorry about that, you're right.
	83 // promise which which resolves successfuly.

	84 function playLoaded()

	85 {

	86 consoleWrite("playLoaded()");

	87 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	88

	89 run("mediaElement = document.createElement('audio')");

	90 var mediaFile = findMediaFile("audio", "content/test");

	91 run("mediaElement.src = '" + mediaFile + "'");

	92

	93 waitForEvent('playing');

	94

	95 waitForEvent('canplaythrough', function() {

	96 testExpected(HTMLMediaElement.HAVE_ENOUGH_DATA, mediaElement.rea dyState);

	97 testExpected(true, mediaElement.paused)

	98

	99 play();

	100 });

	101

	102 mediaElement.load();
	philipj_slow 2016/02/22 06:34:25 Calling load() in addition to setting src doesn't Calling load() in addition to setting src doesn't do anything useful. AFAICT, the load() call can be dropped here and in all cases below, except loadRejectPendingPromises(). mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:25, philipj_UTC7 wrote: > Calling load() in addition to setting src doesn't do anything useful. AFAICT, the load() call can be dropped here and in all cases below, except loadRejectPendingPromises(). Done.
	103 },

	104

	105 // Test that play() on an element when media playback requires a gesture

	106 // returns a rejected promise if there is no user gesture.
	philipj_slow 2016/02/22 06:34:24 Looks like the description should be switched with Looks like the description should be switched with the below test, this is the opposite of what is being tested. (This test falls outside the buckets I listed, but that makes sense because it's testing that the user gesture setting doesn't simply always reject or something silly like that.) mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Looks like the description should be switched with the below test, this is the opposite of what is being tested. > > (This test falls outside the buckets I listed, but that makes sense because it's testing that the user gesture setting doesn't simply always reject or something silly like that.) Done.
	107 function playRequiresUserGestureAndHasIt()

	108 {

	109 consoleWrite("playRequiresUserGestureAndHasIt()");

	110 internals.settings.setMediaPlaybackRequiresUserGesture(true);

	111

	112 run("mediaElement = document.createElement('audio')");

	113 var mediaFile = findMediaFile("audio", "content/test");

	114 run("mediaElement.src = '" + mediaFile + "'");

	115

	116 waitForEvent('playing');

	117 playWithUserGesture();

	118 },

	119

	120 // Test that play() on an element when media playback requires a gesture
	philipj_slow 2016/02/22 06:34:24 Switch description with above. Switch description with above. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Switch description with above. Done.
	121 // returns a resolved promise if there is a user gesture.

	122 function playRequiresUserGestureAndDoesNotHaveIt()

	123 {

	124 consoleWrite("playRequiresUserGestureAndDoesNotHaveIt()");

	125 internals.settings.setMediaPlaybackRequiresUserGesture(true);

	126

	127 run("mediaElement = document.createElement('audio')");

	128 var mediaFile = findMediaFile("audio", "content/test");

	129 run("mediaElement.src = '" + mediaFile + "'");

	130

	131 waitForEvent('playing');

	132 play();

	133 },

	134

	135 // Test that play() on an element with an unsupported content will

	136 // return a rejected promise.

	137 function playNotSupportedContent()

	138 {

	139 consoleWrite("playNotSupportedContent()");

	140 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	141

	142 run("mediaElement = document.createElement('audio')");

	143 var mediaFile = findMediaFile("audio", "content/garbage");
	philipj_slow 2016/02/22 06:34:24 content/garbage doesn't exist. "data:," is a fine content/garbage doesn't exist. "data:," is a fine URL for getting into MEDIA_ERR_SRC_NOT_SUPPORTED. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > content/garbage doesn't exist. "data:," is a fine URL for getting into MEDIA_ERR_SRC_NOT_SUPPORTED. Done.
	144 run("mediaElement.src = '" + mediaFile + "'");

	145

	146 waitForEvent('playing');

	147 waitForEvent('error', function() {

	148 testExpected("mediaElement.error", "[object MediaError]");

	149 testExpected("mediaElement.error.code", MediaError.MEDIA_ERR_SRC _NOT_SUPPORTED);

	150 });

	151 play();

	152 },

	153

	154 // Test that play() returns a resolved promise if called after the

	155 // element suffered from a decode error.

	156 function playDecodeError()

	157 {

	158 consoleWrite("playDecodeError()");

	159 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	160

	161 run("mediaElement = document.createElement('audio')");

	162 var mediaFile = findMediaFile("audio", "content/test");

	163 run("mediaElement.src = '" + mediaFile + "'");

	164

	165 waitForEvent('playing');

	166 waitForEvent('error', function() {

	167 testExpected("mediaElement.error", "[object MediaError]");

	168 testExpected("mediaElement.error.code", MediaError.MEDIA_ERR_DEC ODE);

	169 });

	170

	171 // The setMediaElementNetworkState() method requires metadata to be

	172 // available.

	173 waitForEvent('loadedmetadata', function() {

	174 internals.setMediaElementNetworkState(mediaElement, 6 /* NetworkSt ateDecodeError */);

	175 play();
	philipj_slow 2016/02/22 06:34:24 This doesn't map to any real-world situation AFAIC This doesn't map to any real-world situation AFAICT. If there's a decode error as you reach loadedmetadata, how does the play() actually succeed? Per spe the promise should rather be left pending until a seek or something causes it to succeed. That this test can succeed suggests that we've put the HTMLMediaElement in an inconsistent state. It doesn't look like there are any tests for MEDIA_ERR_DECODE with plain media elements that you could adapt, so I guess this is OK. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Ack. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > This doesn't map to any real-world situation AFAICT. If there's a decode error as you reach loadedmetadata, how does the play() actually succeed? Per spe the promise should rather be left pending until a seek or something causes it to succeed. That this test can succeed suggests that we've put the HTMLMediaElement in an inconsistent state. > > It doesn't look like there are any tests for MEDIA_ERR_DECODE with plain media elements that you could adapt, so I guess this is OK. Ack.
	176 });

	177 },

	178

	179 // Test that play() returns a resolved promise if called after the
	philipj_slow 2016/02/22 06:34:24 IMHO it would be more interesting to see what happ IMHO it would be more interesting to see what happens if the play promise is already pending when the network error happens. The answer is still nothing, of course. Maybe move these two tests to the end with comments as they're not testing anything in the spec? mlamouri (slow - plz ping) 2016/02/23 16:49:00 I've added two comments. I guess the reason why I Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > IMHO it would be more interesting to see what happens if the play promise is already pending when the network error happens. The answer is still nothing, of course. Maybe move these two tests to the end with comments as they're not testing anything in the spec? I've added two comments. I guess the reason why I can't keep the promise pending is that testing that nothing happens is flaky and it's not worth the trouble given what is being tested here. philipj_slow 2016/02/24 09:38:27 Agree, this is OK for now. We should probably revi Show quoted text On 2016/02/23 16:49:00, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > IMHO it would be more interesting to see what happens if the play promise is > already pending when the network error happens. The answer is still nothing, of > course. Maybe move these two tests to the end with comments as they're not > testing anything in the spec? > > I've added two comments. I guess the reason why I can't keep the promise pending > is that testing that nothing happens is flaky and it's not worth the trouble > given what is being tested here. Agree, this is OK for now. We should probably revisit the spec and do something better with these errors anyway.
	180 // element suffered from a network error.

	181 function playNetworkError()

	182 {

	183 consoleWrite("playNetworkError()");

	184 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	185

	186 run("mediaElement = document.createElement('audio')");

	187 var mediaFile = findMediaFile("audio", "content/test");

	188 run("mediaElement.src = '" + mediaFile + "'");

	189

	190 waitForEvent('playing');

	191 waitForEvent('error', function() {

	192 testExpected("mediaElement.error", "[object MediaError]");

	193 testExpected("mediaElement.error.code", MediaError.MEDIA_ERR_NET WORK);

	194 });

	195

	196 // The setMediaElementNetworkState() method requires metadata to be

	197 // available.

	198 waitForEvent('loadedmetadata', function() {

	199 internals.setMediaElementNetworkState(mediaElement, 5 /* NetworkSt ateNetworkError */);

	200 play();

	201 });

	202 },

	203

	204 // Test that play() returns a rejected promise if the element is

	205 // suferring from a not supported error.

	206 function playWithErrorAlreadySet()

	207 {

	208 consoleWrite("playWithErrorAlreadySet()");

	209 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	210

	211 run("mediaElement = document.createElement('audio')");

	212 var mediaFile = findMediaFile("audio", "content/garbage");
	philipj_slow 2016/02/22 06:34:24 data:, here and everywhere content/garbage is used data:, here and everywhere content/garbage is used. Or say content/404 if you're explicitly testing that somewhere. Or load an existing file with garbage if that's interesting, but I'd say go the quickest route to MEDIA_ERR_SRC_NOT_SUPPORTED. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > data:, here and everywhere content/garbage is used. Or say content/404 if you're explicitly testing that somewhere. Or load an existing file with garbage if that's interesting, but I'd say go the quickest route to MEDIA_ERR_SRC_NOT_SUPPORTED. Done.
	213 run("mediaElement.src = '" + mediaFile + "'");

	214

	215 run("mediaElement.load()");

	216

	217 waitForEvent('playing');

	218 waitForEvent('error', function() {

	219 testExpected("mediaElement.error", "[object MediaError]");

	220 testExpected("mediaElement.error.code", MediaError.MEDIA_ERR_SRC _NOT_SUPPORTED);

	221 play();

	222 });

	223 },

	224

	225 // Test that play() returns a resolved promise if the element had its

	226 // source changed after suffering from an error.

	227 function playSrcChangedAfterError()

	228 {

	229 consoleWrite("playSrcChangedAfterError()");

	230 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	231

	232 run("mediaElement = document.createElement('audio')");

	233 var mediaFile = findMediaFile("audio", "content/garbage");

	234 run("mediaElement.src = '" + mediaFile + "'");

	235

	236 run("mediaElement.load()");

	237

	238 waitForEvent('error', function() {

	239 testExpected("mediaElement.error", "[object MediaError]");

	240 testExpected("mediaElement.error.code", MediaError.MEDIA_ERR_SRC _NOT_SUPPORTED);

	241

	242 mediaFile = findMediaFile("audio", "content/test");

	243 run("mediaElement.src = '" + mediaFile + "'");

	244

	245 waitForEvent('playing');

	246 waitForEvent('loadedmetadata', function() {

	247 play();

	248 });

	249 });

	250 },

	251

	252 // Test that play() returns a rejected promise if the element had an
	philipj_slow 2016/02/22 06:34:24 You already test that the promise rejects when err You already test that the promise rejects when error is MEDIA_ERR_SRC_NOT_SUPPORTED and that it's really checking the error attribute with playSrcChangedAfterError. This is only testing setting src really synchronously invokes the media element load algorithm and clears the error, which is more like it's testing the src attribute setter than the play() promise. I suggest keeping only one of this and above test to guard against that possible bug of using something other than the error attribute. This test, being the one with stricter timing, might be the best to keep. mlamouri (slow - plz ping) 2016/02/23 16:49:00 There are 4 src_not_supported related tests: 1. pl Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > You already test that the promise rejects when error is MEDIA_ERR_SRC_NOT_SUPPORTED and that it's really checking the error attribute with playSrcChangedAfterError. This is only testing setting src really synchronously invokes the media element load algorithm and clears the error, which is more like it's testing the src attribute setter than the play() promise. I suggest keeping only one of this and above test to guard against that possible bug of using something other than the error attribute. This test, being the one with stricter timing, might be the best to keep. There are 4 src_not_supported related tests: 1. play() on a src_not_supported file, before the error fires. 2. play() on a src_not_supported file, after the error fires. 3. play() on a file that was src_not_supported. 4. play() on a file that was src_not_supported and just changed. I agree that it might be similar to some other but it still slightly different. If we remove #2 or #3 (which #4 is close to), it will be unclear what the error is if #4 fails. With #4 failing and #2 and #3 passing, it will be clear to developers that there is a race somewhere. I guess it's a different approach: my goal isn't to test the spec but to make sure that any regression is caught and this issue is something I had while writing the code. philipj_slow 2016/02/24 09:38:27 Aha, well I think you've found a bit of a problem Show quoted text On 2016/02/23 16:49:00, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > You already test that the promise rejects when error is > MEDIA_ERR_SRC_NOT_SUPPORTED and that it's really checking the error attribute > with playSrcChangedAfterError. This is only testing setting src really > synchronously invokes the media element load algorithm and clears the error, > which is more like it's testing the src attribute setter than the play() > promise. I suggest keeping only one of this and above test to guard against that > possible bug of using something other than the error attribute. This test, being > the one with stricter timing, might be the best to keep. > > There are 4 src_not_supported related tests: > 1. play() on a src_not_supported file, before the error fires. > 2. play() on a src_not_supported file, after the error fires. > 3. play() on a file that was src_not_supported. > 4. play() on a file that was src_not_supported and just changed. > > I agree that it might be similar to some other but it still slightly different. > If we remove #2 or #3 (which #4 is close to), it will be unclear what the error > is if #4 fails. With #4 failing and #2 and #3 passing, it will be clear to > developers that there is a race somewhere. > > I guess it's a different approach: my goal isn't to test the spec but to make > sure that any regression is caught and this issue is something I had while > writing the code. Aha, well I think you've found a bit of a problem here. See next comment.
	253 // error and just changed its source.

	254 function playRaceWithSrcChangeError()

	255 {

	256 consoleWrite("playRaceWithSrcChangeError()");

	257 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	258

	259 run("mediaElement = document.createElement('audio')");

	260 var mediaFile = findMediaFile("audio", "content/garbage");

	261 run("mediaElement.src = '" + mediaFile + "'");

	262

	263 run("mediaElement.load()");

	264

	265 waitForEvent('error', function() {

	266 testExpected("mediaElement.error", "[object MediaError]");

	267 testExpected("mediaElement.error.code", MediaError.MEDIA_ERR_SRC _NOT_SUPPORTED);

	268

	269 mediaFile = findMediaFile("audio", "content/test");

	270 run("mediaElement.src = '" + mediaFile + "'");

	271

	272 // TODO(mlamouri): if we print the 'playing' event, it seems

	273 // that it actually happens later. It's unclear why.
	philipj_slow 2016/02/22 06:34:24 This sounds like it could be the task ordering pro This sounds like it could be the task ordering problem that I wrote an async_test for below. Can you investigate? It seems very unlikely that listening for the event should change when it's fired. mlamouri (slow - plz ping) 2016/02/23 16:48:59 Hmm, not sure if this comment is clear. Also it's Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > This sounds like it could be the task ordering problem that I wrote an async_test for below. Can you investigate? It seems very unlikely that listening for the event should change when it's fired. Hmm, not sure if this comment is clear. Also it's not actually a bug: 'playing' gets fired but I wasn't expecting this. I assume this is because play() sets pause to false and when src is set and the data loaded, playback starts. Am I right? I removed the TODO. philipj_slow 2016/02/24 09:38:27 Right, setting the src should clear the error, and Show quoted text On 2016/02/23 16:48:59, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > This sounds like it could be the task ordering problem that I wrote an > async_test for below. Can you investigate? It seems very unlikely that listening > for the event should change when it's fired. > > Hmm, not sure if this comment is clear. Also it's not actually a bug: 'playing' > gets fired but I wasn't expecting this. I assume this is because play() sets > pause to false and when src is set and the data loaded, playback starts. Am I > right? > > I removed the TODO. Right, setting the src should clear the error, and then there's nothing stopping the play from succeeding. So why is the play() promise rejected? First, can you assert that the state is as expected right after setting src / before calling play. readyState should be HAVE_NOTHING and the error should be null. It's HTMLMediaElement::noneSupported() that's rejecting this promise, but how did we get there? Unless I'm now extra confused, I think for this test, the promise should be resolved. mlamouri (slow - plz ping) 2016/02/25 11:07:35 As discussed on IRC, the issue was that the tasks Show quoted text On 2016/02/24 at 09:38:27, philipj_UTC7 wrote: > On 2016/02/23 16:48:59, Mounir Lamouri wrote: > > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > > This sounds like it could be the task ordering problem that I wrote an > > async_test for below. Can you investigate? It seems very unlikely that listening > > for the event should change when it's fired. > > > > Hmm, not sure if this comment is clear. Also it's not actually a bug: 'playing' > > gets fired but I wasn't expecting this. I assume this is because play() sets > > pause to false and when src is set and the data loaded, playback starts. Am I > > right? > > > > I removed the TODO. > > Right, setting the src should clear the error, and then there's nothing stopping the play from succeeding. So why is the play() promise rejected? First, can you assert that the state is as expected right after setting src / before calling play. readyState should be HAVE_NOTHING and the error should be null. > > It's HTMLMediaElement::noneSupported() that's rejecting this promise, but how did we get there? Unless I'm now extra confused, I think for this test, the promise should be resolved. As discussed on IRC, the issue was that the tasks resolving the promises needed to be cancelled which did not happen. There is no simple way to add this so I used CancellableTaskFactory with an error code as a member.
	274 play();

	275 });

	276 },

	277

	278 // Test that play() returns a resolved promise when calling play() then

	279 // pause() on an element that already has enough data to play. In other

	280 // words, pause() doesn't cancel play() because it was resolved
	philipj_slow 2016/02/22 06:34:24 Right, this is an important case that will potenti Right, this is an important case that will potentially change when the playing event is delayed until playback really begins. If you change playLoaded() as suggested there will already be a test to verify the timing of the play() promise resolving for this case, so if you find this test redundant I won't mind if you remove it. (The timing is the relevant bit, pause() is just used to test it indirectly here, it could just as well have been load().) mlamouri (slow - plz ping) 2016/02/23 16:49:00 I tend to prefer keeping tests that look redundant Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Right, this is an important case that will potentially change when the playing event is delayed until playback really begins. If you change playLoaded() as suggested there will already be a test to verify the timing of the play() promise resolving for this case, so if you find this test redundant I won't mind if you remove it. (The timing is the relevant bit, pause() is just used to test it indirectly here, it could just as well have been load().) I tend to prefer keeping tests that look redundant. My experience shows that they often end up useful in unexpected ways. philipj_slow 2016/02/24 09:38:27 Acknowledged. Show quoted text On 2016/02/23 16:49:00, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > Right, this is an important case that will potentially change when the playing > event is delayed until playback really begins. If you change playLoaded() as > suggested there will already be a test to verify the timing of the play() > promise resolving for this case, so if you find this test redundant I won't mind > if you remove it. (The timing is the relevant bit, pause() is just used to test > it indirectly here, it could just as well have been load().) > > I tend to prefer keeping tests that look redundant. My experience shows that > they often end up useful in unexpected ways. Acknowledged.
	281 // immediately.

	282 function playFollowedByPauseWhenLoaded()
	philipj_slow 2016/02/22 06:34:24 I suggest avoiding "loaded" as there's no readySta I suggest avoiding "loaded" as there's no readyState that means "fully loaded". readyState>=HAVE_FUTURE_DATA is the relevant condition, so playAndPauseWhenCanPlay could be a more fitting name. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > I suggest avoiding "loaded" as there's no readyState that means "fully loaded". readyState>=HAVE_FUTURE_DATA is the relevant condition, so playAndPauseWhenCanPlay could be a more fitting name. Done.
	283 {

	284 consoleWrite("playFollowedByPauseWhenLoaded()");

	285 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	286

	287 run("mediaElement = document.createElement('audio')");

	288 var mediaFile = findMediaFile("audio", "content/test");

	289 run("mediaElement.src = '" + mediaFile + "'");

	290

	291 run("mediaElement.load()");

	292

	293 waitForEvent('canplaythrough', function() {

	294 waitForEvent('playing');

	295 testExpected("mediaElement.readyState", HTMLMediaElement.HAVE_EN OUGH_DATA);

	296 play();

	297 testExpected("mediaElement.paused", false);

	298 mediaElement.pause();

	299 testExpected("mediaElement.paused", true);

	300 });

	301 },

	302

	303 // Test that play() returns a rejected promise when calling play() then

	304 // pause() on an element that doesn't have enough data to play. In other

	305 // words, pause() cancels play() before it can be resolved.

	306 function playFollowedByPauseWhenLoading()
	philipj_slow 2016/02/22 06:34:24 playAndPauseBeforeCanPlay if you change the naming playAndPauseBeforeCanPlay if you change the naming of the above. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > playAndPauseBeforeCanPlay if you change the naming of the above. Done.
	307 {

	308 consoleWrite("playFollowedByPauseWhenLoaded()");

	309 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	310

	311 run("mediaElement = document.createElement('audio')");

	312 var mediaFile = findMediaFile("audio", "content/test");

	313 run("mediaElement.src = '" + mediaFile + "'");
	philipj_slow 2016/02/22 06:34:25 This test shouldn't need to set a src at all. Tryi This test shouldn't need to set a src at all. Trying to catch the element in a readyState >HAVE_NOTHING but <HAVE_FUTURE_DATA is likely to be racy, there have been many flaky media tests due to this kind of thing. You could make a copy of the below test and call it pauseRejectsPendingPromises(). mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:25, philipj_UTC7 wrote: > This test shouldn't need to set a src at all. Trying to catch the element in a readyState >HAVE_NOTHING but <HAVE_FUTURE_DATA is likely to be racy, there have been many flaky media tests due to this kind of thing. You could make a copy of the below test and call it pauseRejectsPendingPromises(). Done.
	314

	315 waitForEvent('playing');

	316 testExpected("mediaElement.readyState", HTMLMediaElement.HAVE_NOTHIN G);

	317 play();

	318 testExpected("mediaElement.paused", false);

	319 mediaElement.pause();

	320 testExpected("mediaElement.paused", true);

	321 },

	322

	323 // Test that load() rejects all the pending play() promises.

	324 function loadRejectPendingPromises()
	philipj_slow 2016/02/22 06:34:24 Rejects with an s Rejects with an s mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Rejects with an s Done.
	325 {

	326 consoleWrite("loadRejectPendingPromises()");

	327 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	328

	329 run("mediaElement = document.createElement('audio')");

	330

	331 play(); // the promise will be left pending.

	332

	333 waitForEvent('playing');

	334 run("mediaElement.load()");

	335 },

	336

	337 // Test that changing the src rejects the pending play() promises.
	philipj_slow 2016/02/22 06:34:24 Note that this is a paranoid test or remove it. Th Note that this is a paranoid test or remove it. That setting the src attribute invokes the media element load algorithm should be covered by many other tests. It doesn't hurt, though, other than taking more time to run of course. mlamouri (slow - plz ping) 2016/02/23 16:48:59 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Note that this is a paranoid test or remove it. That setting the src attribute invokes the media element load algorithm should be covered by many other tests. It doesn't hurt, though, other than taking more time to run of course. Done. philipj_slow 2016/02/24 09:38:27 Not done, but that's OK :) Show quoted text On 2016/02/23 16:48:59, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > Note that this is a paranoid test or remove it. That setting the src attribute > invokes the media element load algorithm should be covered by many other tests. > It doesn't hurt, though, other than taking more time to run of course. > > Done. Not done, but that's OK :)
	338 function newSrcRejectPendingPromises()

	339 {

	340 consoleWrite("newSrcRejectPendingPromises()");

	341 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	342

	343 run("mediaElement = document.createElement('audio')");

	344

	345 play(); // the promise will be left pending.

	346

	347 var mediaFile = findMediaFile("audio", "content/test");

	348 run("mediaElement.src = '" + mediaFile + "'");

	349 },

	350

	351 // Test ordering of events and promises.

	352 // This is testing a bug in Blink, see https://crbug.com/587871

	353 function testEventAndPromiseOrdering()

	354 {

	355 consoleWrite("testEventAndPromiseOrdering");

	356 internals.settings.setMediaPlaybackRequiresUserGesture(false);

	357

	358 var promiseResolver = null;

	359 var p = new Promise(function(resolve, reject) {

	360 promiseResolver = resolve;

	361 });

	362 p.then(function() {

	363 consoleWrite("Should be after the play() promise is resolved");
	philipj_slow 2016/02/22 06:34:24 I think this test is not quite right. Per spec the I think this test is not quite right. Per spec the event is dispatched in the same task that resolves the promise, but the event is dispatched first. A script-created promise resolved in that event handler is still resolved before the play() promise, and therefore its callbacks should run first, unless there are some rules about promise resolution order that I'm not aware of. What you could perhaps do is to create and resolve a second promise here. We should at this point be after the play() promise is resolved but before its callback i run, and so a newly resolved promise should run after it, but still in the same task. Another bug we'll have is that the task that is queued resolve/reject promises won't be on the correct task source, and so the ordering relative to media element events can be wrong in this way: async_test(function() { var v = document.createElement('video'); v.src = 'data:,'; v.onerror = this.step_func(function() { assert_equals(v.error.code, MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED); var order = []; var push = this.step_func(function(event) { order.push(event); if (order.length == 3) { assert_array_equals(order, ["volumechange", "rejection", "volumechange"]); this.done(); } }); v.onvolumechange = push.bind(this, "volumechange"); v.volume = 0.1; v.play().catch(push.bind(this, "rejection")); v.volume = 0.2; }); }); The actual order will be ["volumechange", "volumechange", "rejection"] because the timer to handle events is started by the first change to volume, and both events are handled at the same time. mlamouri (slow - plz ping) 2016/02/23 16:49:00 Done. Show quoted text On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > I think this test is not quite right. Per spec the event is dispatched in the same task that resolves the promise, but the event is dispatched first. A script-created promise resolved in that event handler is still resolved before the play() promise, and therefore its callbacks should run first, unless there are some rules about promise resolution order that I'm not aware of. > > What you could perhaps do is to create and resolve a second promise here. We should at this point be after the play() promise is resolved but before its callback i run, and so a newly resolved promise should run after it, but still in the same task. > > Another bug we'll have is that the task that is queued resolve/reject promises won't be on the correct task source, and so the ordering relative to media element events can be wrong in this way: > > async_test(function() { > var v = document.createElement('video'); > v.src = 'data:,'; > v.onerror = this.step_func(function() { > assert_equals(v.error.code, MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED); > var order = []; > var push = this.step_func(function(event) { > order.push(event); > if (order.length == 3) { > assert_array_equals(order, ["volumechange", "rejection", "volumechange"]); > this.done(); > } > }); > v.onvolumechange = push.bind(this, "volumechange"); > v.volume = 0.1; > v.play().catch(push.bind(this, "rejection")); > v.volume = 0.2; > }); > }); > > The actual order will be ["volumechange", "volumechange", "rejection"] because the timer to handle events is started by the first change to volume, and both events are handled at the same time. Done.
	364 });

	365

	366 run("mediaElement = document.createElement('audio')");

	367 var mediaFile = findMediaFile("audio", "content/test");

	368 run("mediaElement.src = '" + mediaFile + "'");

	369

	370 play();

	371 waitForEvent('playing', function() {

	372 promiseResolver();

	373 });

	374 }

	375 ];

	376

	377 function start()

	378 {

	379 runNextTestOrFinish();

	380 }

	381

	382 </script>

	383 </head>

	384

	385 <body onload="start()">

	386

	387 <p>Test the play() behaviour with regards to the returned promise for media elem ents.</p>

	388

	389 </body>

	390 </html>

OLD	NEW