Bug 158985 - WebRTC: Add support for the negotiationneeded event in MediaEndpointPeerConnection
Summary: WebRTC: Add support for the negotiationneeded event in MediaEndpointPeerConne...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 143211
  Show dependency treegraph
 
Reported: 2016-06-21 06:34 PDT by Adam Bergkvist
Modified: 2016-06-22 11:15 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (16.12 KB, patch)
2016-06-21 06:48 PDT, Adam Bergkvist
eric.carlson: review+
Details | Formatted Diff | Diff
Updated patch for landing (16.12 KB, patch)
2016-06-21 22:32 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Bergkvist 2016-06-21 06:34:22 PDT
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
Comment 1 Adam Bergkvist 2016-06-21 06:48:31 PDT
Created attachment 281737 [details]
Proposed patch
Comment 2 Eric Carlson 2016-06-21 08:16:21 PDT
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"
Comment 3 Adam Bergkvist 2016-06-21 22:19:42 PDT
(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.
Comment 4 Adam Bergkvist 2016-06-21 22:32:06 PDT
Created attachment 281816 [details]
Updated patch for landing
Comment 5 WebKit Commit Bot 2016-06-22 11:15:51 PDT
Comment on attachment 281816 [details]
Updated patch for landing

Clearing flags on attachment: 281816

Committed r202339: <http://trac.webkit.org/changeset/202339>
Comment 6 WebKit Commit Bot 2016-06-22 11:15:55 PDT
All reviewed patches have been landed.  Closing bug.