Summary: | WebRTC: Add support for the negotiationneeded event in MediaEndpointPeerConnection | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Bergkvist <adam.bergkvist> | ||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 143211 | ||||||||
Attachments: |
|
Description
Adam Bergkvist
2016-06-21 06:34:22 PDT
Created attachment 281737 [details]
Proposed patch
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" (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. Created attachment 281816 [details]
Updated patch for landing
Comment on attachment 281816 [details] Updated patch for landing Clearing flags on attachment: 281816 Committed r202339: <http://trac.webkit.org/changeset/202339> All reviewed patches have been landed. Closing bug. |