[Webkit-unassigned] [Bug 28327] Media layout tests should have a way to provide test files in different formats

Thu Sep 17 09:08:55 PDT 2009

https://bugs.webkit.org/show_bug.cgi?id=28327

Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #39674|review?                     |review-
               Flag|                            |

--- Comment #42 from Eric Carlson <eric.carlson at apple.com>  2009-09-17 09:08:54 PDT ---
(From update of attachment 39674)

> --- 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
<source> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.