[Webkit-unassigned] [Bug 64731] Add MediaSource API to HTMLMediaElement

Tue Aug 30 07:35:38 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=64731

Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105535|review?, commit-queue?      |review+
               Flag|                            |

--- Comment #33 from Eric Carlson <eric.carlson at apple.com>  2011-08-30 07:35:37 PST ---
(From update of attachment 105535)
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:11
> +<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:60
> +    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:75
> +    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:101
> +    appendEnoughClustersToTriggerMetadataLoaded(videoTag, function() {
> +            videoTag.webkitSourceEndOfStream(HTMLMediaElement.EOS_DECODE_ERR);
> +        });

Ditto.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:123
> +    "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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.