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

[bugzilla-daemon at webkit.org]

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


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #38997|review?                     |review+
               Flag|                            |




--- Comment #4 from Eric Carlson <eric.carlson at apple.com>  2009-09-08 14:39:48 PDT ---
(From update of attachment 38997)
> +++ b/LayoutTests/media/controls-strict.html
> +++ b/LayoutTests/media/controls-styling.html
> +++ b/LayoutTests/media/video-aspect-ratio.html
> +++ b/LayoutTests/media/video-controls-rendering.html
> +++ b/LayoutTests/media/video-layer-crash.html
> +++ b/LayoutTests/media/video-transformed.html
> +++ b/LayoutTests/media/video-zoom-controls.html

  Passing the media file path to init() will only work as long as every pixel
tests use a single media file. I think it would make more sense to add a
function to each test file to set the src attribute for each video element
before calling the existing init().

> +++ b/LayoutTests/media/media-file.js

> +function findMediaFile(type, name) {
> +    var codecs;
> +    if (type == "audio")
> +        codecs = audioCodecs;
> +    else
> +        codecs = videoCodecs;
> +
> +    var element = document.createElement(type);
> +    for (var i = 0; i < codecs.length; ++i) {
> +        if (element.canPlayType(codecs[i][0]))
> +            return name + "." + codecs[i][1];
> +    }
> +
> +    return "";
> +}


  It should be more efficient to call canPlayType() on an existing element if
there is one. I would check to see if one exists before creating a new one.


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

>  <script>
>      testExpected("video.currentSrc", "");
>      var source = document.createElement("source");
> -    source.setAttribute("src", "content/test.mp4");
> +    source.setAttribute("src", findMediaFile("video", "content/test"));
>      video.appendChild(source);
>  
>      testExpected("video.currentSrc", "");
>  
>      waitForEvent("load", function () {
> -        testExpected("relativeURL(video.currentSrc) ", "content/test.mp4");
> +        testExpected("relativeURL(video.currentSrc) ", findMediaFile("video", "content/test"));
>          endTest();
>      });

  'findMediaFile("video", "content/test")' will always return the same result,
you should only call it once.

+++ b/LayoutTests/media/video-paint-test.js
@@ -1,6 +1,11 @@
-function init()
+function init(videoFile)
 {
-    var totalCount = document.getElementsByTagName('video').length;
+    var videos = document.getElementsByTagName('video');
+    var totalCount = videos.length;
+    for (var i = 0; i < totalCount; ++i) {
+        videos[i].src = videoFile;
+    }

  These changes aren't necessary with the first suggestion above.

r+ if you make these changes (or convince me they aren't necessary).

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