Closed Bug 825336 Opened 7 years ago Closed 7 years ago

Add constraint to suppress data channel

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: ekr, Assigned: abr)

Details

(Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+][qa-])

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #696415 - Flags: review?(adam)
Comment on attachment 696415 [details] [diff] [review]
Add constraint to suppress data channel

Review of attachment 696415 [details] [diff] [review]:
-----------------------------------------------------------------

Minor omission in the test driver (new test case doesn't use new SHOULD_OMIT_DATA flag), plus a recommendation to catch potential future regressions. The first one is simple; the other is my personal opinion with which reasonable people may disagree; so I'm going to r+ this.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +832,5 @@
>      }
> +
> +    if (flags & SHOULD_OMIT_DATA) {
> +      ASSERT_EQ(sdp.find("m=application"), std::string::npos);
> +    }

Given that this patch introduces new code that can turn data off, I would think we want to check for possible regressions here (i.e., suppressing the m=application section when unintended); at the very least, something like:

>if (flags & SHOULD_OMIT_DATA) {
>  ASSERT_EQ(sdp.find("m=application"), std::string::npos);
>} else {
>  ASSERT_NE(sdp.find("m=application"), std::string::npos);
>}

You'll need to update the tests AudioOnlyG711Call, ChromeOfferAnswer, and OfferAllDynamicTypes to either include the SHOULD_OMIT_DATA flag, or add a new DONT_CHECK_DATA flag.

@@ +999,5 @@
> +  sipcc::MediaConstraints constraints;
> +  constraints.setBooleanConstraint("OfferToReceiveAudio", true, false);
> +  constraints.setBooleanConstraint("OfferToReceiveVideo", true, false);
> +  constraints.setBooleanConstraint("MozDontOfferDataChannel", true, false);
> +  CreateOffer(constraints, OFFER_AV, SHOULD_SENDRECV_AV);

CreateOffer(constraints, OFFER_AV, SHOULD_SENDRECV_AV | SHOULD_OMIT_DATA);
Attachment #696415 - Flags: review?(adam) → review+
Attachment #696415 - Attachment is obsolete: true
Assignee: ekr → adam
Whiteboard: [WebRTC] [blocking-webrtc-interop]
Status: NEW → ASSIGNED
Attachment #696428 - Flags: review?(rjesup)
Whiteboard: [WebRTC] [blocking-webrtc-interop] → [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+]
Attachment #696428 - Flags: review?(rjesup) → review+
Attachment #696428 - Flags: checkin?(rjesup)
Attachment #696428 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/aaee8c35e1d0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] → [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.