[webkit-reviews] review denied: [Bug 125926] [GStreamer] WebKit gets stalled when trying to play a stream : [Attachment 225346] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Andres Gomez Garcia
<agomez at igalia.com>'s request for review:
Bug 125926: [GStreamer] WebKit gets stalled when trying to play a stream
https://bugs.webkit.org/show_bug.cgi?id=125926

Attachment 225346: Patch
https://bugs.webkit.org/attachment.cgi?id=225346&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=225346&action=review


> LayoutTests/http/tests/media/media-play-stream-chunked-icy.html:5
> +	   <script src=../../media-resources/video-test.js></script>
> +	   <script src=../../media-resources/media-file.js></script>

As this is an http test, please just use absolute paths to resources:

<script src=/media-resources/video-test.js></script>

> LayoutTests/http/tests/media/media-play-stream-chunked-icy.html:8
> +	       var media = findMediaFile('audio',
'../../../../media/content/silence');

Do we still need a relative path here?

> LayoutTests/http/tests/media/resources/media-stream-chunked-icy.php:10
> +    $fileName = $_GET["name"];
> +    $type = $_GET["type"];
> +
> +    $_GET = array();
> +    $_GET["name"] = $fileName;
> +    $_GET["type"] = $type;
> +    $_GET["content-length"] = "no";
> +    $_GET["icy-data"] = "yes";

I don't understand why this script is needed. All it does is add two _GET
arguments, so why don't we just pass all the arguments in
media-play-stream-chunked-icy.html?

> LayoutTests/http/tests/media/resources/serve-video.php:11
> +	   $tmpFile = tempnam($dir, $prefix);
> +	   if (!file_exists($tmpFile))

Please see how other tests store temporary files. We use portabilityLayer.php
for cross-platform compatibility, and make it clear from file names which
specific test the temporary file belongs to. E.g.
http/tests/appcache/resources/counter.php. This uses sys_get_temp_dir() to get
temporary directory.

The last thing we'd want is to have temporary files shared across tests, they
must be all unique.

> LayoutTests/http/tests/media/resources/serve-video.php:35
> +	   "httpStatus" => "404 Not Fond",

Typo: "404 Not Fond".

> LayoutTests/http/tests/media/resources/serve-video.php:46
> +    // 404 on errors

Why not a 5xx code?

> LayoutTests/http/tests/media/resources/serve-video.php:79
> +		   $id3Url =
"http://sourceforge.net/projects/getid3/files/latest/download?source=files";

The script to update the database should not be the same script that's run as
part of tests, it's just too confusing. I can think if a few options, but Eric
is the better one to advise on the course of action:

- Check in the whole getid3 (assuming its license is compatible), and have a
script to update the database, which one will need to run when changing the
assets.
- Have a separate script for this purpose, that will download getid3 like here.

- Check in the whole getid3 (assuming its license is compatible), and just
parse the files every time, not checking in the database.

> LayoutTests/http/tests/media/resources/serve-video.php:184
> +    header("Status: " . $settings["httpStatus"]);
> +    header("HTTP/1.1 " . $settings["httpStatus"]);

This looks suspicious. Is sending the HTTP/1.1 line something one actually
needs to do?

> LayoutTests/http/tests/media/resources/serve-video.php:187
> +    if ($settings["httpStatus"] == "404 Not Fond") {

Typo: "404 Not Fond"