Summary: | Add support for webkitSourceAddId() & webkitSourceRemoveId() | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Colwell <acolwell> | ||||||||||||||
Component: | Media | Assignee: | Aaron Colwell <acolwell> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, ojan, tkent, tkent+wkapi, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 83607 | ||||||||||||||||
Attachments: |
|
Description
Aaron Colwell
2012-04-10 14:14:03 PDT
Created attachment 136552 [details]
Patch
Created attachment 136553 [details]
Fix test_expectations.txt
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 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? 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.
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. (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. (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. 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. Created attachment 136714 [details]
Patch
Ping. Can I get a review please. > Ping. Can I get a review please.
Who should review this patch?
(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. 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? 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? Created attachment 137147 [details]
Patch
Comment on attachment 137147 [details] Patch Marking r+ only, pending approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org. 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 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 Created attachment 137361 [details]
Patch
Created attachment 137449 [details]
Rebase patch now that WK84046 style fixes landed.
eric.carlson or fishd can I get an r+,cq+ please. I've addressed all outstanding concerns. 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> All reviewed patches have been landed. Closing bug. |