Bug 158621

Summary: WebRTC: Add test that verifies that RTCPeerConnection.addTrack can reuse an existing RTCRtpSender
Product: WebKit Reporter: Adam Bergkvist <adam.bergkvist>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 143211    
Attachments:
Description Flags
Proposed patch
none
Updated patch none

Description Adam Bergkvist 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
Comment 1 Adam Bergkvist 2016-06-10 10:19:29 PDT
Created attachment 281013 [details]
Proposed patch
Comment 2 Eric Carlson 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.
Comment 3 Adam Bergkvist 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.
Comment 4 Adam Bergkvist 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. :)
Comment 5 Adam Bergkvist 2016-06-13 03:08:46 PDT
Created attachment 281163 [details]
Updated patch

Fixed review comments
Comment 6 Adam Bergkvist 2016-06-13 12:14:10 PDT
Comment on attachment 281163 [details]
Updated patch

Thanks for reviewing Eric!
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2016-06-13 12:37:28 PDT
All reviewed patches have been landed.  Closing bug.