MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state. The following use case will not work, if it checks for active state while adding a new track. 1. Create a MediaStream without any argument, then try to add tracks to mediaStream will not work.
Created attachment 230808 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. MediaStream.addTrack Should not check for active state.
(In reply to comment #1) > Created an attachment (id=230808) [details] > MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. > > MediaStream.addTrack Should not check for active state. What, specifically, does the spec say about this?
(In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=230808) [details] [details] > > MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. > > > > MediaStream.addTrack Should not check for active state. > > What, specifically, does the spec say about this? This bug arise while modifying MediaStream.ended state to active state. Spec is not specifying to check for active state in addTrack steps [1]. 1.Let track be the MediaStreamTrack argument and stream this MediaStream object. 2.If track is already in stream's track set, then abort these steps. 3.Add track to stream's track set. Further if we add this check then the scenario mentioned in comment 1, i.e., "Create a MediaStream without any argument, then try to add tracks to the newly created mediaStream" will not work, because in this case since a newly created mediaStream has no tracks its state is inactive. [1] http://dev.w3.org/2011/webrtc/editor/getusermedia.html#widl-MediaStream-addTrack-void-MediaStreamTrack-track
Comment on attachment 230808 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. View in context: https://bugs.webkit.org/attachment.cgi?id=230808&action=review > Source/WebCore/ChangeLog:11 > + No new tests, no behavior change. This is incorrect, you note the need for a behavior change in the bug: "Create a MediaStream without any argument, then try to add tracks to the newly created mediaStream". It sounds like it should be easy to create a test case for this change.
Created attachment 231049 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. The new patch file is modified according to the comments received. Please review the new patch file.
Comment on attachment 231049 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. Attachment 231049 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6166218729848832 New failing tests: svg/text/non-bmp-positioning-lists.svg media/W3C/video/networkState/networkState_during_loadstart.html
Created attachment 231052 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 231057 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. Modified patch.
Comment on attachment 231057 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. View in context: https://bugs.webkit.org/attachment.cgi?id=231057&action=review r- for now because the test needs a little bit of work, but this is close. Thanks for responding to previous feedback. > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:9 > + <body> > + <p id="description"></p> > + <div id="console"></div> > + <script> I know that you based this on an existing test, but I would really prefer to have this <script> element inside of <head> and call it from body.load or some such. > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:17 > + function error() { Nit: WebKit coding style is to put a function's opening brace on a new line. > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:40 > + function shouldNotFire() { This is not used. You need to add "onaddtrack" and "onremovetrack" handlers to stream2. > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:51 > + function shouldFireActive() { > + debug("Stream2 is active."); > + finishJSTest(); > + } > + > + function shouldFireInActive() { > + debug("Stream2 is inactive."); > + finishJSTest(); Neither of these is called, should they be? If not, each should call testFailed(...) instead of debug() to make it clear that there is an error. > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:54 > + function createStreamAndAddTRacks() { Nit: "TRacks" should be "Tracks"
Created attachment 231129 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. Modified the Test file as per the comments.
Comment on attachment 231129 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. View in context: https://bugs.webkit.org/attachment.cgi?id=231129&action=review I am marking this r+ but not cq+ because of the minor changes needed. That means that your revised patch does not need to be reviewed again, so when you upload a new patch: 1 -put "Reviewed by Eric Carlson" in the ChangeLog files 2 - name your patch "Patch for landing" 3 - DO NOT mark this patch as obsolete 4 - mark the new patch cq+ but not r? (because it doesn't need a new review) and any reviewer can mark it cq+. Thanks for sticking with this! > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:6 > + <p id="description"></p> > + <div id="console"></div> Oops, these elements should be in <body> intend of in <head>! > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:8 > + description("Test adding tracks to inactive MediaStream."); This can go in startMedia(). > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:57 > + //getUserMedia({audio:true, video:true}, gotStream1); Nit: this should be removed. > LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:68 > + <body onload="javascript:startMedia()"> Nit: "javascript:" is unnecessary.
Created attachment 231211 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. Thanks for review. The patch is modified and attached as per the review comments.
Comment on attachment 231211 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. Rejecting attachment 231211 [details] from review queue. kiran.guduru@samsung.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Comment on attachment 231211 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. Rejecting attachment 231211 [details] from commit-queue. kiran.guduru@samsung.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Created attachment 231212 [details] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. Thanks for reviewing. Plese commit the modified patch as per review comments.
Committed r168679: <http://trac.webkit.org/changeset/168679>