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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 10 09:31:49 PDT 2012

Eric Carlson <eric.carlson at apple.com> has denied  review:
Bug 80978: [BlackBerry] MMRPlayer will hang webkit thread when retrieving media

Attachment 136391: Patch

------- 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) {


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


> LayoutTests/http/tests/media/video-throttle-load-metadata.html:32
> +	       video.src =
"" + movie +

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

More information about the webkit-reviews mailing list