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

Wed Aug 10 15:41:53 PDT 2011

Eric Carlson <eric.carlson at apple.com> has denied 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 102925: Patch
https://bugs.webkit.org/attachment.cgi?id=102925&action=review

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

All of these tests are extremely WebM specific, whereas all of the existing
media tests are format agnostic. This seems reasonable for now because I assume
the feature only works with WebM files at the moment, but the test file names
should reflect this. Or it may make more sense to make a "mediasource"
directory in LayoutTests/media, and to have a folder in that for each format.
This test structure will also make it easier to skip tests on ports where they
aren't supported because you can just include "media/mediasource/" in the
skipped file to skip all tests.

> LayoutTests/http/tests/media/media-source.js:23
> +  var r = new XMLHttpRequest();
> +  r.open("GET", url, false);
> +  r.responseType = 'arraybuffer';
> +  r.send();

Nit: We try to use descriptive names for variables in WebKit.

> LayoutTests/http/tests/media/media-source.js:25
> +  if (r.status != 200) {

Nit: Although there isn't a formal standard for tab width in JS files, I
believe that all of the media tests use 4-tab spaces.

> LayoutTests/http/tests/media/media-source.js:39
> +function getNumClusters() {
> +  return cluster_info.length;
> +}

Nit: We try to use full names instead of abbreviations, eg. getClusterCount,
getNumberOfClusters, etc.

> LayoutTests/http/tests/media/media-source.js:41
> +function getCluster(i) {

Nit: descriptive variable names are preferred. Here and throughout the rest of
the file.

> LayoutTests/http/tests/media/media-source.js:119
> +    case 0:
> +	 str = "SOURCE_CLOSED";
> +	 break;
> +    case 1:
> +	 str = "SOURCE_OPEN";
> +	 break;
> +    case 2:

Is there any reason to not use the named constants, eg. "case
HTMLMediaElement.SOURCE_CLOSED:", etc?

> LayoutTests/http/tests/media/video-media-source-errors.html:1
> +<html>

Nit: Is there any reason to not include an html5 DOCTYPE? Here and elsewhere.

> LayoutTests/http/tests/media/video-media-source-errors.html:35
> +	   appendCluster(v, next_cluster_index);
> +	   window.setTimeout(f, 0, next_cluster_index + 1);

Is there any way to do this with an event listener instead of setTimeout (here
and elsewhere)? Timeouts have been the source of lots of flakiness in media
tests.

> LayoutTests/http/tests/media/video-media-source-seek.html:40
> +	  if (Math.abs(currentTime - seekTime) > 0.001) {

Why do you need the magic number? At the very least you should add a comment
about why this is needed and what this tolerance is OK. How much variation do
you actually see?

> LayoutTests/http/tests/media/video-media-source-state-changes.html:17
> +	 window.setTimeout(appendUntilEnd, 0, v, i + 1);

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:23
> +	   return

Missing a semi-colon here.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:32
> +	 window.setTimeout(appendClustersUntilTrue, 0, v, i + 1, conditionStr,
done_cb);

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:62
> +	 window.setTimeout(appendUntilEnd, 0, v, 0);

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:88
> +	 window.setTimeout(appendClustersUntilTrue, 0,
> +			   v, startIndex, "firstSeekComplete",
> +			   function () { firstSeekClusterAppendComplete(v);} );

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:120
> +	 window.setTimeout(appendClustersUntilTrue, 0,
> +			   v, startIndex, "secondSeekComplete",
> +			   function () { secondSeekClusterAppendComplete(v);}
);
> +    }

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:159
> +	 window.setTimeout(appendUntilEnd, 0, v, 0);

More setTimeout.

> Source/WebCore/html/HTMLMediaElement.cpp:571
>      if (m_networkState != NETWORK_EMPTY) {
> +#if ENABLE(MEDIA_SOURCE)
> +	   setSourceState(SOURCE_CLOSED);
> +#endif
>	   m_networkState = NETWORK_EMPTY;
>	   m_readyState = HAVE_NOTHING;

Why do you set the state to SOURCE_CLOSED here...

> Source/WebCore/html/HTMLMediaElement.cpp:599
>      m_networkState = NETWORK_NO_SOURCE;
>  
> +#if ENABLE(MEDIA_SOURCE)
> +    setSourceState(SOURCE_CLOSED);
> +#endif
> +
>      // 2 - Asynchronously await a stable state.

and here?

> Source/WebCore/html/HTMLMediaElement.idl:104
> +    // media source state

Comment should be a complete sentence.

> Source/WebKit/chromium/ChangeLog:19
> +2011-08-04  Aaron Colwell  <acolwell at chromium.org>
> +
> +	   Add MediaSource API to HTMLMediaElement
> +	   https://bugs.webkit.org/show_bug.cgi?id=64731
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * features.gypi:
> +	   * public/WebMediaPlayer.h:
> +	   (WebKit::WebMediaPlayer::sourceAppend):
> +	   (WebKit::WebMediaPlayer::sourceEndOfStream):
> +	   * public/WebMediaPlayerClient.h:
> +	   * src/WebMediaPlayerClientImpl.cpp:
> +	   (WebKit::WebMediaPlayerClientImpl::sourceOpened):
> +	   (WebKit::WebMediaPlayerClientImpl::sourceURL):
> +	   (WebKit::WebMediaPlayerClientImpl::sourceAppend):
> +	   (WebKit::WebMediaPlayerClientImpl::sourceEndOfStream):
> +	   * src/WebMediaPlayerClientImpl.h:
> +

I am going to assume that you have had someone familiar with Chromium's
embedder API review these changes, because I am not qualified.