Bug 83788 - Update webkitSourceAppend() signature to match Media Source v0.4 spec
Summary: Update webkitSourceAppend() signature to match Media Source v0.4 spec
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aaron Colwell
URL:
Keywords:
Depends on:
Blocks: 83607
  Show dependency treegraph
 
Reported: 2012-04-12 09:44 PDT by Aaron Colwell
Modified: 2012-05-01 10:31 PDT (History)
9 users (show)

See Also:


Attachments
Patch (22.12 KB, patch)
2012-04-24 15:16 PDT, Aaron Colwell
no flags Details | Formatted Diff | Diff
Making style bot happy with my ChangeLog entry (22.34 KB, patch)
2012-04-24 15:28 PDT, Aaron Colwell
no flags Details | Formatted Diff | Diff
Addressed style nit (23.37 KB, patch)
2012-04-25 10:02 PDT, Aaron Colwell
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Colwell 2012-04-12 09:44:04 PDT
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.
Comment 1 Aaron Colwell 2012-04-24 15:16:07 PDT
Created attachment 138663 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-24 15:19:43 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 WebKit Review Bot 2012-04-24 15:20:27 PDT
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.
Comment 4 Aaron Colwell 2012-04-24 15:28:03 PDT
Created attachment 138668 [details]
Making style bot happy with my ChangeLog entry
Comment 5 Eric Carlson 2012-04-25 09:05:46 PDT
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 abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org

> 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 :-) ).
Comment 6 Aaron Colwell 2012-04-25 10:02:20 PDT
Created attachment 138832 [details]
Addressed style nit
Comment 7 Aaron Colwell 2012-04-26 09:36:22 PDT
Ping. abarth, dglazkov, fishd or tkent can I get a review & cq+ please.
Comment 8 Darin Fisher (:fishd, Google) 2012-04-26 14:10:44 PDT
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?
Comment 9 Aaron Colwell 2012-04-26 14:24:29 PDT
(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.
Comment 10 Aaron Colwell 2012-04-27 08:47:36 PDT
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.
Comment 11 Aaron Colwell 2012-05-01 10:31:44 PDT
Closing in favor of landing all Media Source v0.4 method signatures at once.