RESOLVED FIXED 158621
WebRTC: Add test that verifies that RTCPeerConnection.addTrack can reuse an existing RTCRtpSender 
https://bugs.webkit.org/show_bug.cgi?id=158621
Summary WebRTC: Add test that verifies that RTCPeerConnection.addTrack can reuse an e...
Adam Bergkvist
Reported 2016-06-10 10:10:29 PDT
This is behavior is detailed in the RTCPeerConnection.addTrack algorithm step 5 [1]. [1] https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#dom-rtcpeerconnection-addtrack
Attachments
Proposed patch (6.67 KB, patch)
2016-06-10 10:19 PDT, Adam Bergkvist 		no flags
DetailsFormatted DiffDiff
Updated patch (6.64 KB, patch)
2016-06-13 03:08 PDT, Adam Bergkvist 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Adam Bergkvist
Comment 1 2016-06-10 10:19:29 PDT
Created attachment 281013 [details] Proposed patch
Eric Carlson
Comment 2 2016-06-10 10:37:58 PDT
Comment on attachment 281013 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=281013&action=review > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> I just noticed that you are using the HTML 2.0 doctype for these tests. Why not just "<!DOCTYPE html>"? > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:52 > + debug(""); Minor nit: you can get rid of all debug("")s by adding a '<br>' to the beginning of the next debug string. > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:58 > + debug("*** An extra sender should have been added"); Another minor nit: why no blank line before this? > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:69 > + debug("*** The number of senders should not have changed"); Ditto.
Adam Bergkvist
Comment 3 2016-06-13 03:02:01 PDT
(In reply to comment #2) > Comment on attachment 281013 [details] > Proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281013&action=review > > > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > I just noticed that you are using the HTML 2.0 doctype for these tests. Why > not just "<!DOCTYPE html>"? I had not noticed that, but you're right. I based this on some other test which had this old doctype. Fixed now. > > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:52 > > + debug(""); > > Minor nit: you can get rid of all debug("")s by adding a '<br>' to the > beginning of the next debug string. Let's use that. Fixed. > > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:58 > > + debug("*** An extra sender should have been added"); > > Another minor nit: why no blank line before this? These lines belong to the same chunk as the ones above. I can see that the initial "***" on debug messages makes them look like "headers". I borrowed that style from some other test to highlight debug messages. > > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:69 > > + debug("*** The number of senders should not have changed"); > > Ditto.
Adam Bergkvist
Comment 4 2016-06-13 03:05:35 PDT
(In reply to comment #3) > > > LayoutTests/fast/mediastream/RTCPeerConnection-addTrack-reuse-sender.html:58 > > > + debug("*** An extra sender should have been added"); > > > > Another minor nit: why no blank line before this? > > These lines belong to the same chunk as the ones above. I can see that the > initial "***" on debug messages makes them look like "headers". I borrowed > that style from some other test to highlight debug messages. > But then again, looking at the actual test these sections are separated by newlines. Let's space them out in the results as well. :)
Adam Bergkvist
Comment 5 2016-06-13 03:08:46 PDT
Created attachment 281163 [details] Updated patch Fixed review comments
Adam Bergkvist
Comment 6 2016-06-13 12:14:10 PDT
Comment on attachment 281163 [details] Updated patch Thanks for reviewing Eric!
WebKit Commit Bot
Comment 7 2016-06-13 12:37:25 PDT
Comment on attachment 281163 [details] Updated patch Clearing flags on attachment: 281163 Committed r201999: <http://trac.webkit.org/changeset/201999>
WebKit Commit Bot
Comment 8 2016-06-13 12:37:28 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.