[webkit-reviews] review granted: [Bug 64731] Add MediaSource API to HTMLMediaElement : [Attachment 103985] Patch

Wed Aug 17 12:00:02 PDT 2011

Eric Carlson <eric.carlson at apple.com> has granted Aaron Colwell
<acolwell at chromium.org>'s request for review:
Bug 64731: Add MediaSource API to HTMLMediaElement
https://bugs.webkit.org/show_bug.cgi?id=64731

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

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

Marking r+ although I would prefer to have the stylistic nits addressed before
it is landed. 

I think that it is important to run test for basic functionality like those in
LayoutTests/media/ when data is loaded with this API. It would be great if
there was a way to reuse the existing tests so we are sure there aren't any
subtle errors caused by the use of this API, but I can't immediately think of a
way to make that work.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:8

> + <head>
> +  <script src="../../../media-resources/video-test.js"></script>
> +  <script src="webm-media-source.js"></script>
> +  <script>
> +    function onError(event) {
> +	   var video_tag = event.target;

Indentation should be consistent, not a mixture of 2 and 4 tab spaces.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:1
1
> +	   var error_string = "UNKNOWN";
> +	   switch(video_tag.error.code) {

Nit: WebKit tests almost always use camelCase for JavaScript variables. I was
going to say that *all* WebKit tests do this but then I found a few WebGL
tests... None-the-less, I would prefer to see these tests follow the convention
used in all other media tests.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:2
9
> +    function appendClustersUntilMetadataLoaded(video_tag, done_callback) {

Very minor nit: this function name is confusing, I assumed that it would append
clusters until the event, not just append two and wait.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:4
5
> +    function didNotSendHeadersFirst(event) {
> +	   appendCluster(event.target, 0);
> +    }

WebKit convention is to put a function's first brace on its own line:

function didNotSendHeadersFirst(event)
{
    appendCluster(event.target, 0);
}

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:5
4
> +	   appendClustersUntilMetadataLoaded(video_tag,
function(next_cluster_id) {
> +	       appendCluster(video_tag, next_cluster_id - 1);
> +	   });

Nit: declaring next_cluster_id as a global would have made this and the other
places it is used slightly easier to understand.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:1
30
> +	   var event_handler_function = function (event) {
> +		consoleWrite("running " + function_name);
> +		event.target.removeEventListener('webkitsourceopen',
event_handler_function);
> +		on_open_function(event);
> +	    };

Minor Nit: Having a blank line between tests results would make them slightly
easier to read.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:28
> +	   if (video_tag.currentTime > 2 && do_seek) {
> +	       consoleWrite("EVENT(timeupdate) : seeking to " + seek_time);
> +	       video_tag.pause();
> +	       video_tag.currentTime = seek_time;
> +	       do_seek = false;

Nit: logging a float with full precision caused platform specific failures in
other media tests, so tests were changed to log and compare "value.toFixed(2)".

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:45
> +	   if (current_time != seek_time ) {
> +	       failTest("Seeked to " + current_time + " instead of " +
seek_time);
> +	       return;
> +	   }

Ditto.

> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:80
> +function appendCluster(video_tag, clusterIndex) {

Nit: Please use consistent naming style: camelCase or underscores.

> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:94
> +function appendUntilEndOfStream(video_tag, startIndex) {

Ditto.

> Source/WebKit/chromium/features.gypi:64
>	 'ENABLE_METER_TAG=1',
> +	 'ENABLE_MEDIA_SOURCE=1',
>	 'ENABLE_MEDIA_STATISTICS=1',

Do you mean to turn this feature on for all Chromium builds even though it
isn't complete?