Bug 83788 – Update webkitSourceAppend() signature to match Media Source v0.4 spec

• Component: Media
• Status: RESOLVED LATER

[Aaron Colwell]

The Media Source v0.4 spec(http://html5-mediasource-api.googlecode.com/svn/tags/0.4/draft-spec/mediasource-draft-spec.html) adds an ID parameter to the webkitSourceAppend() signature. 

The method signature needs to be updated in the IDL, ID validation logic needs to be added, and the id parameter needs to be propagated to the MediaPlayerPrivate interface so the Chromium code has access to it.

[Aaron Colwell]

Created attachment 138663 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 138663 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Aaron Colwell]

Created attachment 138668 [details]
Making style bot happy with my ChangeLog entry

[Eric Carlson]

Comment on attachment 138668 [details]
Making style bot happy with my ChangeLog entry

View in context: https://bugs.webkit.org/attachment.cgi?id=138668&action=review

The WebKit portion looks OK modulo the minor style nit noted. Not marking r+ or cq + because I am not [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail]

> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:114
> +function addSourceId(videoTag) {
> +    videoTag.webkitSourceAddId(sourceID, 'video/webm; codecs="vp8, vorbis"');
> +}

Nit: the opening brace should be on its own line (darned EMACS formatter :-) ).

[Aaron Colwell]

Created attachment 138832 [details]
Addressed style nit

[Aaron Colwell]

Ping. abarth, dglazkov, fishd or tkent can I get a review & cq+ please.

[Darin Fisher (:fishd, Google)]

Comment on attachment 138832 [details]
Addressed style nit

View in context: https://bugs.webkit.org/attachment.cgi?id=138832&action=review

> Source/WebCore/html/HTMLMediaElement.idl:107
> +    [V8EnabledAtRuntime=mediaSource] void webkitSourceAppend(in DOMString id, in Uint8Array data) raises (DOMException);

I assume we have not yet shipped with mediaSource enabled at runtime in a stable Chrome build, right?  We don't have to worry about breaking existing content with this change, right?

[Aaron Colwell]

(In reply to comment #8)
> (From update of attachment 138832 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138832&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.idl:107
> > +    [V8EnabledAtRuntime=mediaSource] void webkitSourceAppend(in DOMString id, in Uint8Array data) raises (DOMException);
> 
> I assume we have not yet shipped with mediaSource enabled at runtime in a stable Chrome build, right?  We don't have to worry about breaking existing content with this change, right?

This feature is not enabled by default. Since Chrome 17 people have had to manually turn it on through about:flags. If people happen to be experimenting with this API this change will break them. They would need to call webkitSourceAddId() and then use the ID in all their webkitSourceAppend() calls.

[Aaron Colwell]

fishd: Is it ok to introduce such a breakage since the feature is behind about:flags? If so could you please cq+ the patch. If not could you point me to some docs/info indicating how I should go about deprecating the old signature in favor of the new one. Thanks.

[Aaron Colwell]

Closing in favor of landing all Media Source v0.4 method signatures at once.