Bug 158985 – WebRTC: Add support for the negotiationneeded event in MediaEndpointPeerConnection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Bergkvist]

The negotiationneeded event [1] helps the developer to know when an SDP negotiation is needed to reflect local changes to the other peer.

[1] https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#session-negotiation-model

[Adam Bergkvist]

Created attachment 281737 [details]
Proposed patch

[Eric Carlson]

Comment on attachment 281737 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281737&action=review

> LayoutTests/ChangeLog:15
> +        * fast/mediastream/RTCPeerConnection-more-media-to-negotiate-expected.txt: Added.
> +        * fast/mediastream/RTCPeerConnection-more-media-to-negotiate.html: Added.
> +        Verify that a negotiationneeded event is fired when not all local media can be included in
> +        an answer. (The answerer cannot reply with more m-lines than the offer had to begin with.)
> +        * fast/mediastream/RTCPeerConnection-onnegotiationneeded-expected.txt:
> +        * fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:

Don't we also need a test for a stream track entering an ended state? A bug for adding that test is fine if you don't want to do it in this patch.

Are there any other operations that should generate a 'negotiationneeded' event?

> LayoutTests/fast/mediastream/RTCPeerConnection-more-media-to-negotiate.html:10
> +            description("Test that a negotiationneeded event is fired when all local media can't be included in an answer");

Nit: "when all local media can't be included" -> "when not all local media can be included"

[Adam Bergkvist]

(In reply to comment #2)
> Comment on attachment 281737 [details]
> Proposed patch

Thanks for reviewing

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281737&action=review
> 
> > LayoutTests/ChangeLog:15
> > +        * fast/mediastream/RTCPeerConnection-more-media-to-negotiate-expected.txt: Added.
> > +        * fast/mediastream/RTCPeerConnection-more-media-to-negotiate.html: Added.
> > +        Verify that a negotiationneeded event is fired when not all local media can be included in
> > +        an answer. (The answerer cannot reply with more m-lines than the offer had to begin with.)
> > +        * fast/mediastream/RTCPeerConnection-onnegotiationneeded-expected.txt:
> > +        * fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:
> 
> Don't we also need a test for a stream track entering an ended state? A bug
> for adding that test is fine if you don't want to do it in this patch.

We should also test that case. I remember that we've talked about an internal test API to emulate events originating from the browser; something like "stopSourceOfTrack(track)". That would be useful here. About a bug, see next comment.

> Are there any other operations that should generate a 'negotiationneeded'
> event?

There's an issue [1] filed on the WebRTC 1.0 github repo about putting together a table will all cases that must set the 'negotiation needed flag'. I've also referenced this issue in a new bug about adding more tests [2].

[1] https://github.com/w3c/webrtc-pc/issues/661
[2] http://webkit.org/b/159016


> > LayoutTests/fast/mediastream/RTCPeerConnection-more-media-to-negotiate.html:10
> > +            description("Test that a negotiationneeded event is fired when all local media can't be included in an answer");
> 
> Nit: "when all local media can't be included" -> "when not all local media
> can be included"

Will fix.

[Adam Bergkvist]

Created attachment 281816 [details]
Updated patch for landing

[WebKit Commit Bot]

Comment on attachment 281816 [details]
Updated patch for landing

Clearing flags on attachment: 281816

Committed r202339: <http://trac.webkit.org/changeset/202339>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.