[webkit-reviews] review denied: [Bug 80978] [BlackBerry] MMRPlayer will hang webkit thread when retrieving media metadata : [Attachment 136391] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied  review:
Bug 80978: [BlackBerry] MMRPlayer will hang webkit thread when retrieving media
metadata
https://bugs.webkit.org/show_bug.cgi?id=80978

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=136391&action=review


Marking r- for the incorrect 'type' param in the cgi url.

> LayoutTests/http/tests/media/video-throttle-load-metadata-worker.js:2
> +postMessage("Message from worker thread");
>  \ No newline at end of file

Nit: please add a blank line at the end of the file.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:5
> +    <!--
> +    This test case is for https://bugs.webkit.org/show_bug.cgi?id=80978
> +    --!>

Nit: Personally, I think comments like this are very helpful to have in the
markup of the test so someone looking at the test results, or running the test
itself, can see it without having to look through the source.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:13
> +	   function loadedmetadata(e) {
> +	       logResult(true, "loaded metadata of media file");
> +	       endTest();
> +	   }

WebKit style is to have a function's opening brace on a new line.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:15
> +	   function error(e) {

Ditto.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:20
> +	   function start() {

Ditto.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:32
> +	       video.src =
"http://127.0.0.1:8000/media/video-throttled-load.cgi?name=" + movie +
"&throttle=400&type=video/mp4";

"type=video/mp4" isn't necessarily correct. findMediaFile() will return a file
that the current port can load, which may not be MPEG-4. You should use
something like "mimeTypeForExtension(movie.split('.').pop())" instead.