Bug 83616 – Add support for webkitSourceAddId() & webkitSourceRemoveId()

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Aaron Colwell]

WebKit changes needed to support the new webkitSourceAddId() & webkitSourceRemoveId() Media Source methods. 

LayoutTests for the new methods will be initially disabled because the Chromium changes depend on the AddIdStatus enum.

[Aaron Colwell]

Created attachment 136552 [details]
Patch

[Aaron Colwell]

Created attachment 136553 [details]
Fix test_expectations.txt

[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.

[Adam Barth]

Comment on attachment 136553 [details]
Fix test_expectations.txt

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

> Source/WebCore/ChangeLog:9
> +        Add webkitSourceAddId() & webkitSourceRemoveId() to HTMLMediaElement
> +        and propagate calls to the MediaPlayerPrivate interface.
> +        https://bugs.webkit.org/show_bug.cgi?id=83616
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html

Why?  What is this?  Is there a spec?

[WebKit Review Bot]

Attachment 136553 [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
LayoutTests/platform/chromium/test_expectations.txt:3822:  Entries on line 3529 and line 3822 match overlapping sets of configurations webaudio/convolution-mono-mono.html  [test/expectations] [5]
Total errors found: 1 in 15 files


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

[Adam Barth]

Comment on attachment 136553 [details]
Fix test_expectations.txt

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

> Source/WebCore/html/HTMLMediaElement.idl:100
> +    [V8EnabledAtRuntime=webkitMediaSource] void webkitSourceAddId(in DOMString id, in DOMString type) raises (DOMException);
> +    [V8EnabledAtRuntime=webkitMediaSource] void webkitSourceRemoveId(in DOMString id) raises (DOMException);

Somewhat unrelated to this change, but the V8EnabledAtRuntime "webkitMediaSource" shouldn't have a "webkit" prefix.  The prefix is just for the web-facing API.

[Aaron Colwell]

(In reply to comment #4)
> (From update of attachment 136553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136553&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Add webkitSourceAddId() & webkitSourceRemoveId() to HTMLMediaElement
> > +        and propagate calls to the MediaPlayerPrivate interface.
> > +        https://bugs.webkit.org/show_bug.cgi?id=83616
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Test: http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html
> 
> Why?  What is this?  Is there a spec?
Yes. http://html5-mediasource-api.googlecode.com/svn/tags/0.4/draft-spec/mediasource-draft-spec.html . Sorry I forgot to mention it here.

[Aaron Colwell]

(In reply to comment #6)
> (From update of attachment 136553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136553&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.idl:100
> > +    [V8EnabledAtRuntime=webkitMediaSource] void webkitSourceAddId(in DOMString id, in DOMString type) raises (DOMException);
> > +    [V8EnabledAtRuntime=webkitMediaSource] void webkitSourceRemoveId(in DOMString id) raises (DOMException);
> 
> Somewhat unrelated to this change, but the V8EnabledAtRuntime "webkitMediaSource" shouldn't have a "webkit" prefix.  The prefix is just for the web-facing API.

Ok. I'll fix this in another patch.

[Aaron Colwell]

Comment on attachment 136553 [details]
Fix test_expectations.txt

Removing commit-queue request until the patch for https://bugs.webkit.org/show_bug.cgi?id=83633 lands.

[Aaron Colwell]

Created attachment 136714 [details]
Patch

[Aaron Colwell]

Ping. Can I get a review please.

[Adam Barth]

> Ping. Can I get a review please.

Who should review this patch?

[Aaron Colwell]

(In reply to comment #12)
> > Ping. Can I get a review please.
> 
> Who should review this patch?
So it appears that the sheriff-bot says abarth, dglazkov, fishd, darin, jamesr, or tkent need to review it at a minimum. It isn't clear to me which of these options is most appropriate. If you don't mind abarth, could you do it?

I also assumed that eric.carlson would review the code since he works on HTMLMediaElement a fair amount.

Sorry I wasn't a little more explicit.

[Eric Carlson]

Comment on attachment 136714 [details]
Patch

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

This looks fine to me modulo the mostly stylistic comments. Not marking r+ because I would prefer to see at least some of the comments addressed, and because I can't review Chromium public API parts.

> Source/WebCore/html/HTMLMediaElement.cpp:2212
> +    if (id.isNull() || id.isEmpty()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return;
> +    }
> +
> +    if (m_sourceIDs.contains(id)) {
> +        ec = INVALID_STATE_ERR;
> +        return;
> +    }

Why not use isValidSourceId()?

> Source/WebCore/html/HTMLMediaElement.cpp:2231
> +    case MediaPlayer::AisNoError:
> +        m_sourceIDs.add(id);
> +        return;
> +    case MediaPlayer::AisNotSupported:
> +        ec = NOT_SUPPORTED_ERR;
> +        return;
> +    case MediaPlayer::AisReachedIdLimit:

These enum names are unusual, why do they have an "A" prefix? 

(some time later) Ah, I just got that "Ais" is probably "add ID status". I didn't figure this out until I saw the enum name, so I don't think the prefix is helpful. Will it not work to just use "NoError", "NotSupported", and "ReachedIdLimit"?

> Source/WebCore/html/HTMLMediaElement.cpp:2236
> +    ec = INVALID_STATE_ERR;

Is there a legitimate way to reach this part of the function? If not, please add an ASSERT_NOT_REACHED.

> Source/WebCore/html/HTMLMediaElement.cpp:2250
> +    m_player->sourceRemoveId(id);

Should the player be able to signal failure? The only case I can think of where the source looks valid to the media element but not to the player is a bug in one or the other, but it might be worth making MediaPlayer::sourceRemoveId return a bool and ASSERT here if it returns false.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:7
> +            var DEFAULT_SOURCE_MIMETYPE = "video/webm; codecs=\"vp8, vorbis\"";

I don't recall seeing all upper case, underscore delimited, variable names in any other layout tests.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:10
> +            function runOnSourceOpen(videoTag, onOpenFunction) {
> +                var eventHandlerFunction = function (event) {

WebKit style is to have the opening brace for a function on a new line.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:22
> +            function expectExceptionOnAddId(videoTag, id, type, error) {
> +                try {

Ditto here and throughout the rest of this test.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:29
> +                } catch (e) {
> +                    if (!(e.code == error)) {
> +                        failTest("Unexpected exception " + e);
> +                        throw e;
> +                    }

Nit: I would prefer to have logging for success as well as for failure. I think it is helpful for the test results to document what happens, so logging the exception thrown would be good. Here and throughout the rest of this test.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:65
> +                runOnSourceOpen(videoTag, function (vt) {
> +                    // Test empty ID case
> +                    expectExceptionOnAddId(videoTag, "", DEFAULT_SOURCE_MIMETYPE, DOMException.INVALID_ACCESS_ERR);
> +
> +                    videoTag.webkitSourceAddId("123", DEFAULT_SOURCE_MIMETYPE);

Nit: Why does this function have a video element parameter (vt) but use the one from the parent scope (videoTag)? Here and throughout the test.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:67
> +                    // Test adding the same ID again.

Nit: I would prefer to see comments like this in the test output so it will be easier to understand what is being tested when a test fails unexpectedly. Here and throughout the test.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:79
> +                    for (var i = 0; i < 20; ++i) {

Is 20 from the spec? If so, a comment would be helpful for someone looking at a failure later.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:111
> +                    // Test removing and ID that was never added.

Type: Test removing *and* ID...

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:169
> +                var functionName = testCases[testCaseIndex];
> +                var testCaseFunction = eval(functionName);

Nit: Is there any reason to have an array of function names instead of an array of functions and descriptive strings for logging?

[Aaron Colwell]

Comment on attachment 136714 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:2212
>> +    }
> 
> Why not use isValidSourceId()?

isValidSourceId() is subtly different. If the ID is not in m_sourceIDs then it triggers an error. Here we want to trigger an error if the ID is already in m_sourceIDs. isValidSourceId() only has one caller right now, but when I update webkitSourceAppend() and add webkitSourceBuffered() & webkitSourceAbort() it will be used more.

>> Source/WebCore/html/HTMLMediaElement.cpp:2231
>> +    case MediaPlayer::AisReachedIdLimit:
> 
> These enum names are unusual, why do they have an "A" prefix? 
> 
> (some time later) Ah, I just got that "Ais" is probably "add ID status". I didn't figure this out until I saw the enum name, so I don't think the prefix is helpful. Will it not work to just use "NoError", "NotSupported", and "ReachedIdLimit"?

Andrew Scherkus just had the same reaction to my related Chromium changes. :) I've removed the Ais and changed NoError to Ok.

>> Source/WebCore/html/HTMLMediaElement.cpp:2236
>> +    ec = INVALID_STATE_ERR;
> 
> Is there a legitimate way to reach this part of the function? If not, please add an ASSERT_NOT_REACHED.

No. Done.

>> Source/WebCore/html/HTMLMediaElement.cpp:2250
>> +    m_player->sourceRemoveId(id);
> 
> Should the player be able to signal failure? The only case I can think of where the source looks valid to the media element but not to the player is a bug in one or the other, but it might be worth making MediaPlayer::sourceRemoveId return a bool and ASSERT here if it returns false.

I was expecting WebKit to be the authority here, but you're probably right. I think there are cases where the player is shutting down and WebKit doesn't know about it yet that might require this. Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:7
>> +            var DEFAULT_SOURCE_MIMETYPE = "video/webm; codecs=\"vp8, vorbis\"";
> 
> I don't recall seeing all upper case, underscore delimited, variable names in any other layout tests.

Done

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:10
>> +                var eventHandlerFunction = function (event) {
> 
> WebKit style is to have the opening brace for a function on a new line.

Done

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:22
>> +                try {
> 
> Ditto here and throughout the rest of this test.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:29
>> +                    }
> 
> Nit: I would prefer to have logging for success as well as for failure. I think it is helpful for the test results to document what happens, so logging the exception thrown would be good. Here and throughout the rest of this test.

Done

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:65
>> +                    videoTag.webkitSourceAddId("123", DEFAULT_SOURCE_MIMETYPE);
> 
> Nit: Why does this function have a video element parameter (vt) but use the one from the parent scope (videoTag)? Here and throughout the test.

No reason. Just the result of copy-and-pasting. Fixed to use vt.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:67
>> +                    // Test adding the same ID again.
> 
> Nit: I would prefer to see comments like this in the test output so it will be easier to understand what is being tested when a test fails unexpectedly. Here and throughout the test.

Great idea. Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:79
>> +                    for (var i = 0; i < 20; ++i) {
> 
> Is 20 from the spec? If so, a comment would be helpful for someone looking at a failure later.

It's an arbitrary number to ensure termination. I added a comment indicating this fact and that we are assuming that implementation won't allow more than 20 simultaneous sourceIDs.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:111
>> +                    // Test removing and ID that was never added.
> 
> Type: Test removing *and* ID...

Done

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:169
>> +                var testCaseFunction = eval(functionName);
> 
> Nit: Is there any reason to have an array of function names instead of an array of functions and descriptive strings for logging?

No. I was able to get rid of the eval and just put functions in the list. I was hoping the function names were descriptive enough to avoid extra description strings. Is that ok?

[Aaron Colwell]

Created attachment 137147 [details]
Patch

[Eric Carlson]

Comment on attachment 137147 [details]
Patch

Marking r+ only, pending approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail].

[Darin Fisher (:fishd, Google)]

Comment on attachment 137147 [details]
Patch

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

> Source/WebKit/chromium/public/WebMediaPlayer.h:93
> +    enum AddIdStatus {

nit: please see the WebKit API style guide for rules on enum naming:
http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

The rest of this interface is not consistent either :-(

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:348
> +    return static_cast<WebCore::MediaPlayer::AddIdStatus>(m_webMediaPlayer->sourceAddId(id, type));

you should add compile time asserts to AssertMatchingEnums.cpp

[Aaron Colwell]

Comment on attachment 137147 [details]
Patch

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

>> Source/WebKit/chromium/public/WebMediaPlayer.h:93
>> +    enum AddIdStatus {
> 
> nit: please see the WebKit API style guide for rules on enum naming:
> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums
> 
> The rest of this interface is not consistent either :-(

Fixed. Created http://webk.it/84046 to take care of the rest of the style violations.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:348
>> +    return static_cast<WebCore::MediaPlayer::AddIdStatus>(m_webMediaPlayer->sourceAddId(id, type));
> 
> you should add compile time asserts to AssertMatchingEnums.cpp

Done

[Aaron Colwell]

Created attachment 137361 [details]
Patch

[Aaron Colwell]

Created attachment 137449 [details]
Rebase patch now that WK84046 style fixes landed.

[Aaron Colwell]

eric.carlson or fishd can I get an r+,cq+ please. I've addressed all outstanding concerns.

[WebKit Review Bot]

Comment on attachment 137449 [details]
Rebase patch now that WK84046 style fixes landed.

Clearing flags on attachment: 137449

Committed r114390: <http://trac.webkit.org/changeset/114390>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.