[webkit-reviews] review denied: [Bug 132558] [MediaStream] MediaStream.addTrack Should not check for active state. : [Attachment 231057] MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

Thu May 8 07:19:27 PDT 2014

Eric Carlson <eric.carlson at apple.com> has denied Kiran
<kiran.guduru at samsung.com>'s request for review:
Bug 132558: [MediaStream] MediaStream.addTrack Should not check for active
state.
https://bugs.webkit.org/show_bug.cgi?id=132558

Attachment 231057: MediaStream.addTrack method is checking for active state of
a MediaStream, but it should not check for active state while adding a Track.
https://bugs.webkit.org/attachment.cgi?id=231057&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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"