825336 - Add constraint to suppress data channel

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Add constraint to suppress data channel

Comment 2

[Adam Roach [:abr]]

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);

Comment 3

[Adam Roach [:abr]]

Attachment: Add constraint to suppress data channel

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaee8c35e1d0

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aaee8c35e1d0