third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html - Issue 1815033003: Add srcObject attribute of type MediaStream to HTMLMediaElement.

Side by Side Diff: third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html

Issue 1815033003: Add srcObject attribute of type MediaStream to HTMLMediaElement. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: fix condition Created 4 years, 8 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 | « media/blink/webmediaplayer_impl.cc ('k') | third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html » ('j') | third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE HTML>

	2 <html>

	3 <body>

	4 <video id="testVideo" autoplay="autoplay"></video>
	philipj_slow 2016/04/06 11:58:42 Nit: The autoplay attribute doesn't need any speci Nit: The autoplay attribute doesn't need any specific value, the empty string is fine, which you'll get by just <video autoplay>. Guido Urdaneta 2016/04/06 16:34:54 Done. Show quoted text On 2016/04/06 11:58:42, philipj wrote: > Nit: The autoplay attribute doesn't need any specific value, the empty string is > fine, which you'll get by just <video autoplay>. Done.
	5 <div id="log"></div>

	6 <p>Test assigment of a mediastream via the srcObject attribute.</p>
	philipj_slow 2016/04/06 11:58:42 Can you put this inside <title> at the top? This w Can you put this inside <title> at the top? This way, when there's only a single test, that test gets the document.title as its title by default. Also capitalize MediaStream. Guido Urdaneta 2016/04/06 16:34:54 Done. Show quoted text On 2016/04/06 11:58:42, philipj wrote: > Can you put this inside <title> at the top? This way, when there's only a single > test, that test gets the document.title as its title by default. Also capitalize > MediaStream. Done.
	7 <script src="../resources/testharness.js"></script>

	8 <script src="../resources/testharnessreport.js"></script>

	9 <script src="./w3c-media-utils.js"></script>
	philipj_slow 2016/04/06 11:58:41 Doesn't look like you're using anything from w3c-m Doesn't look like you're using anything from w3c-media-utils.js. Guido Urdaneta 2016/04/06 16:34:54 Done. Show quoted text On 2016/04/06 11:58:41, philipj wrote: > Doesn't look like you're using anything from w3c-media-utils.js. Done.
	10 <script>

	11 var test = async_test("srcObject test");

	12 var video = document.getElementById("testVideo");

	13

	14 function startTest()

	15 {

	16 test.step(() => assert_idl_attribute(video, "srcObject"));
	philipj_slow 2016/04/06 11:58:42 You shouldn't need to wrap any individual steps li You shouldn't need to wrap any individual steps like this. Just make sure that each of these functions is appropriately wrapped in test.step_func(). This way if exceptions are thrown where you're not anticipating them, like say if navigator.webkitGetUserMedia is removed, it'll still fail the test properly. You can avoid the startTest() method entirely by just doing async_test(test => { /* the steps that start at this line / }, / no title if you have a <title> above /). Guido Urdaneta* 2016/04/06 16:34:54 Done. Show quoted text On 2016/04/06 11:58:42, philipj wrote: > You shouldn't need to wrap any individual steps like this. Just make sure that > each of these functions is appropriately wrapped in test.step_func(). This way > if exceptions are thrown where you're not anticipating them, like say if > navigator.webkitGetUserMedia is removed, it'll still fail the test properly. > > You can avoid the startTest() method entirely by just doing async_test(test => { > /* the steps that start at this line / }, / no title if you have a <title> > above */). Done.
	17 video.addEventListener("playing", playingSrcObject);
	philipj_slow 2016/04/06 11:58:41 Here you'll need test.step_func(playingSrcObject). Here you'll need test.step_func(playingSrcObject). Similarly elsewhere. Guido Urdaneta 2016/04/06 16:34:54 Done. Show quoted text On 2016/04/06 11:58:41, philipj wrote: > Here you'll need test.step_func(playingSrcObject). Similarly elsewhere. Done.
	18 test.step(_ => assert_equals(video.srcObject, null));

	19 test.step(_ => assert_equals(video.currentSrc, ""));

	20 navigator.webkitGetUserMedia(

	21 {video:true},

	22 stream => video.srcObject = stream,
	philipj_slow 2016/04/06 11:58:42 The sequence of events could be a bit clearer if y The sequence of events could be a bit clearer if you added the playing event listener in this callback, because I guess it's setting the srcObject which will eventually cause the play to happen? An explicit call to play() and removing autoplay would also be clearer, IMHO. Guido Urdaneta 2016/04/06 16:34:54 Done, but kept the autoplay. With an explicit call Show quoted text On 2016/04/06 11:58:42, philipj wrote: > The sequence of events could be a bit clearer if you added the playing event > listener in this callback, because I guess it's setting the srcObject which will > eventually cause the play to happen? An explicit call to play() and removing > autoplay would also be clearer, IMHO. Done, but kept the autoplay. With an explicit call to play() the test worked, but I got the following console message: "Uncaught (in promise) AbortError: The play() request was interrupted by a new load request." philipj_slow 2016/04/07 15:38:11 Do you get a rejected promise even if you call pla Show quoted text On 2016/04/06 16:34:54, Guido Urdaneta wrote: > On 2016/04/06 11:58:42, philipj wrote: > > The sequence of events could be a bit clearer if you added the playing event > > listener in this callback, because I guess it's setting the srcObject which > will > > eventually cause the play to happen? An explicit call to play() and removing > > autoplay would also be clearer, IMHO. > > Done, but kept the autoplay. With an explicit call to play() the test worked, > but I got the following console message: > "Uncaught (in promise) AbortError: The play() request was interrupted by a new > load request." Do you get a rejected promise even if you call play() after setting srcObject? I think that should be fixed by a CL mlamouri@ is working on: https://codereview.chromium.org/1865933002/ Just using autoplay is fine, though.
	23 _ => test.step(() => assert_unreached("Did not get mediastream") ));
	philipj_slow 2016/04/06 11:58:41 This can be test.unreached_func("Did not get Media This can be test.unreached_func("Did not get MediaStream"); Guido Urdaneta 2016/04/06 16:34:54 Done. Show quoted text On 2016/04/06 11:58:41, philipj wrote: > This can be test.unreached_func("Did not get MediaStream"); Done.
	24 }

	25

	26 function playingSrcObject()

	27 {

	28 video.removeEventListener("playing", playingSrcObject)
	philipj_slow 2016/04/06 11:58:42 Is this needed? If it is, then using the onplaying Is this needed? If it is, then using the onplaying attribute and setting it to null here could be cleaner than saving the test.step_func(playingSrcObject) value that you'd otherwise have to do. Guido Urdaneta 2016/04/06 16:34:54 Done. Not needed. Show quoted text On 2016/04/06 11:58:42, philipj wrote: > Is this needed? If it is, then using the onplaying attribute and setting it to > null here could be cleaner than saving the test.step_func(playingSrcObject) > value that you'd otherwise have to do. Done. Not needed.
	29 video.addEventListener("emptied", playingNothing)
	philipj_slow 2016/04/06 11:58:41 This is an oddly named callback. If you don't enjo This is an oddly named callback. If you don't enjoy coming up with names for callbacks, you can of course just nest everything. Go down to two spaces indent and it may still be readable enough. Guido Urdaneta 2016/04/06 16:34:54 Done. What would have been a good name? :) Show quoted text On 2016/04/06 11:58:41, philipj wrote: > This is an oddly named callback. If you don't enjoy coming up with names for > callbacks, you can of course just nest everything. Go down to two spaces indent > and it may still be readable enough. Done. What would have been a good name? :) philipj_slow 2016/04/07 15:38:11 Maybe "emptied"? Just "playing" for the current fu Show quoted text On 2016/04/06 16:34:54, Guido Urdaneta wrote: > On 2016/04/06 11:58:41, philipj wrote: > > This is an oddly named callback. If you don't enjoy coming up with names for > > callbacks, you can of course just nest everything. Go down to two spaces > indent > > and it may still be readable enough. > > Done. What would have been a good name? :) Maybe "emptied"? Just "playing" for the current function would also work.
	30 test.step(() => assert_not_equals(video.srcObject, null));

	31 test.step(() => assert_class_string(video.srcObject, "MediaStream")) ;

	32 test.step(() => assert_equals(video.currentSrc, ""));

	33 video.srcObject=null;
	philipj_slow 2016/04/06 11:58:42 Needs more whitespace. Needs more whitespace. Guido Urdaneta 2016/04/06 16:34:54 Done. Show quoted text On 2016/04/06 11:58:42, philipj wrote: > Needs more whitespace. Done.
	34 }

	35

	36 function playingNothing()

	37 {

	38 test.step(() => assert_equals(video.srcObject, null));

	39 test.step(() => assert_equals(video.currentSrc, ""));

	40 test.done();
	philipj_slow 2016/04/06 11:58:42 When this is wrapped instead, you can use test.ste When this is wrapped instead, you can use test.step_func_done() and omit the step.done(). Guido Urdaneta 2016/04/06 16:34:54 Done. Show quoted text On 2016/04/06 11:58:42, philipj wrote: > When this is wrapped instead, you can use test.step_func_done() and omit the > step.done(). Done.
	41 }

	42

	43 test.step(startTest);

	44 </script>

	45 </body>

	46 </html>

OLD	NEW