[webkit-reviews] review granted: [Bug 28327] Media layout tests should have a way to provide test files in different formats : [Attachment 38997] First round of changes - 10 tests

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted 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 38997: First round of changes - 10 tests
https://bugs.webkit.org/attachment.cgi?id=38997&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> +++ 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).