Bug 158985

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.