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

Tue Aug 30 07:35:37 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 105535: Patch
https://bugs.webkit.org/attachment.cgi?id=105535&action=review

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

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

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:1
1
> +<html>
> + <head>
> +  <script src="../../../media-resources/video-test.js"></script>
> +  <script src="webm-media-source.js"></script>
> +  <script>
> +function onError(event)
> +{
> +    var videoTag = event.target;
> +
> +    var errorString = "UNKNOWN";

Now your indentation is using a mixture of *one* and four space tabs:

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

Please be consistent - use the same size tabs throughout for JS and markup.
Indenting the markup and leaving the JS flush left is unusual.

Please pick a tab size and stick with it. As I noted before, the rest of the
media tests use four space tabs so I would prefer that:

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

In any case, be consistent. This may seem a niggling complaint, but indentation
is meant to make code easier to read and comprehend and to make structure
obvious. This is especially important in a big project like WebKit, and I don't
think your current style succeeds on either account.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:6
0
> +    appendEnoughClustersToTriggerMetadataLoaded(videoTag, function() {
> +	       appendCluster(videoTag, nextClusterIndex - 1);
> +	   });

The closing brace should line up with the beginning of the line with the
opening brace, as it does in the "eventHandler" function in
appendEnoughClustersToTriggerMetadataLoaded above.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:7
5
> +    appendEnoughClustersToTriggerMetadataLoaded(videoTag, function() {
> +	       // Append 1 past the next cluster.
> +	       appendCluster(videoTag, nextClusterIndex + 1);
> +
> +	       // Append the next cluster.
> +	       appendCluster(videoTag, nextClusterIndex);
> +	   });

The closing brace should line up with the beginning of the line with the
opening brace

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:1
01
> +    appendEnoughClustersToTriggerMetadataLoaded(videoTag, function() {
> +	      
videoTag.webkitSourceEndOfStream(HTMLMediaElement.EOS_DECODE_ERR);
> +	   });

Ditto.

>
LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:1
23
> +    "networkErrorAfterHaveMetadata"
> +		    ];

Ditto, here and throughout the rest of the patch.

> Source/WebCore/ChangeLog:20
> +	   (WebCore::HTMLMediaElement::HTMLMediaElement):
> +	   (WebCore::HTMLMediaElement::prepareForLoad):
> +	   (WebCore::HTMLMediaElement::loadResource):

It is helpful to have function by function explanations of what changed in your
changelog.

> Source/WebCore/html/HTMLMediaElement.cpp:1297
> +    bool noSeekRequired = (!seekableRanges->length() || (time == now &&
displayMode() != Poster));

Is this change necessary?

> Source/WebCore/html/HTMLMediaElement.h:149
> +    enum EndOfStreamStatus { EOS_NO_ERROR, EOS_NETWORK_ERR, EOS_DECODE_ERR
};
> +    void webkitSourceEndOfStream(unsigned short status, ExceptionCode&);

Is there any reason to not have "status" be an EndOfStreamStatus instead of an
unsigned short?

> Source/WebCore/html/HTMLMediaElement.idl:110
> +#if defined(ENABLE_MEDIA_SOURCE) && ENABLE_MEDIA_SOURCE
> +    // URL passed to src attribute to enable the media source logic.
> +    readonly attribute [URL] DOMString webkitMediaSourceURL;
> +
> +    // Appends media to to the source.
> +    void webkitSourceAppend(in Uint8Array data) raises (DOMException);
> +
> +    // Signals the end of stream.
> +    const unsigned short EOS_NO_ERROR = 0; // End of stream reached w/o
error.
> +    const unsigned short EOS_NETWORK_ERR = 1; // A network error triggered
end of stream.
> +    const unsigned short EOS_DECODE_ERR = 2; // A decode error triggered end
of stream.
> +    void webkitSourceEndOfStream(in unsigned short status) raises
(DOMException);
> +
> +    // Indicates the current state of the media source.
> +    const unsigned short SOURCE_CLOSED = 0;
> +    const unsigned short SOURCE_OPEN = 1;
> +    const unsigned short SOURCE_ENDED = 2;
> +    readonly attribute unsigned short webkitSourceState;
> +#endif

Can these use "[Conditional= ENABLE_MEDIA_SOURCE]" instead of "#if
definedENABLE_MEDIA_SOURCE"? See https://bugs.webkit.org/show_bug.cgi?id=64231
and https://bugs.webkit.org/show_bug.cgi?id=64961.