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

Fri May 9 09:30:01 PDT 2014

Eric Carlson <eric.carlson at apple.com> has granted 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 231129: 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=231129&action=review

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