[webkit-reviews] review denied: [Bug 28327] Media layout tests should have a way to provide test files in different formats : [Attachment 39674] Third round of changes - 25 tests

Thu Sep 17 09:08:54 PDT 2009

Eric Carlson <eric.carlson at apple.com> has denied Hin-Chung Lam
<hclam at google.com>'s request for review:
Bug 28327: Media layout tests should have a way to provide test files in
different formats
https://bugs.webkit.org/show_bug.cgi?id=28327

Attachment 39674: Third round of changes - 25 tests
https://bugs.webkit.org/attachment.cgi?id=39674&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>

> --- a/LayoutTests/ChangeLog

> +
> +	   Updating 20 media layout tests to use media files based on supported
codecs.
> +
> +	   * media/media-file.js: Swap the order of audio file extension
preference.

Please expand the comment to say why the preferred order was changed.

> +++ b/LayoutTests/media/video-played-reset.html

>  
> -		   run("video.src = \"content/test.mp4\"");
> +		   run("video.src = \"" + findMediaFile("video",
"content/test") + "\"");

I think the pattern you used in the previous patch, where the media file path
is put into a local variable in a case like this:

		var mediaFile = findMediaFile("video", "content/test");
		run("video.src = \"" + mediaFile + "\"");

would make it easier to read this test.

> --- a/LayoutTests/media/video-seek-past-end-paused.html

> -    run("video.src = 'content/test.mp4'");
> +    run("video.src = '" + findMediaFile("video", "content/test") + "'");

Ditto.

> --- a/LayoutTests/media/video-seek-past-end-playing.html

> -    run("video.src = 'content/test.mp4'");
> +    run("video.src = '" + findMediaFile("video", "content/test") + "'");

Ditto.

> --- a/LayoutTests/media/video-source.html
> +++ b/LayoutTests/media/video-source.html

I really hate tests like this that use a document fragment and put script in
the body for no obvious reason, so I try to clean them up when I have to change
them anyway. It obviously isn't necessary for what you are fixing with this
patch, but taking a few extra minutes to tidy up ugly tests like this would be
good for everyone. Something like the following is what I would prefer
(warning, I haven't actually tested this):

<html>
    <head>
	<script src=media-file.js></script>
	<script src=video-test.js></script>
	<script>
	    function loadStart()
	    {
		testExpected("relativeURL(video.currentSrc) ",
"content/test.mp4");
		endTest();
	    }

	    function setup()
	    {
		video = mediaElement =
document.getElementsByTagName('video')[0];

		var source = document.createElement('source'); 
		source.src = findMediaFile("video", "content/test");
		video.appendChild(source);
	    }
	</script>
    </head>
    <body onload="setup()">
	<video controls onloadstart="loadStart()"> </video>
    </body>
</html>

> +++ a/LayoutTests/media/video-src-invalid-remove.html

Looking at this test I see that it is already quite timing dependent, so it
would be nice to restructure it up as long as you are changing it. I suggest
you include an empty <video> element, and set the 'src' and add the <source>
element in a body onload handler so the listeners are setup before the file can
load. Maybe something like this (again, not tested):

<html>
    <head>
	<script src=media-file.js></script>
	<script src=video-test.js></script>
	<script>
	    var loadCount = 0;
	    var mediaFile = findMediaFile("video", "content/test");;

	    function loadStart() { [code goes here] }
	    function loadedmetadata() { [code goes here] }
	    function errorEvent() { [code goes here] }

	    function setup()
	    {
		video = mediaElement =
document.getElementsByTagName('video')[0];

		consoleWrite("");
		waitForEvent('loadedmetadata', loadedmetadata);
		waitForEvent('loadstart', loadStart );
		waitForEvent('error', errorEvent);

		video.src = "bogus.mov"

		var source = document.createElement('source'); 
		source.src = mediaFile;
		video.appendChild(source);
	    }
	</script>
    </head>
    <body onload="setup()">
	<video controls > </video>
	<p>Test that removing invalid 'src' attribute triggers load of
&lt;source&gt; elements</p>
    </body>
</html>

> --- a/LayoutTests/media/video-src-remove.html

> -    <video src=content/silence.mpg controls
onloadedmetadata="loadedmetadata()" >
> -	   <source src=content/test.mp4>
> -    </video>
> +    <div id=panel></div>
> +    <script>
> +	   var panel = document.getElementById("panel");
> +	   var mediaFile = findMediaFile("video", "content/test");
> +	   panel.innerHTML = "<video src=" + mediaFile + " controls
onloadedmetadata='loadedmetadata()'><source src=content/counting.mp4></video>";

> +    </script>

I would prefer to see the video element set up with DOM calls instead
innerHTML.

> --- a/LayoutTests/media/video-src-source.html

> -<video src=content/test.mp4 controls>
> -    <source src=content/error.mpeg>
> -</video>
> +<div id=panel></div>
> +<script>
> +    var panel = document.getElementById("panel");
> +    var mediaFile = findMediaFile("video", "content/test");
> +    panel.innerHTML = "<video src=" + mediaFile + " controls><source
src=content/error.mpeg></video>";
> +</script>

Same here.

> --- a/LayoutTests/media/video-zoom.html

> -<body onload="init()">
> +<body onload="setSrcByTagName('video', findMediaFile('video',
'content/test')); init();">

It would be cleaner to call setSrcByTagName() in the init() function.