Removing "crypto_required" from MediaContentDescription.

Removing "crypto_required" from MediaContentDescription.

"Crypto required" is a property of the PeerConnection of construction
time; it has nothing to do with SDP. So I'm moving it out of
MediaContentDescription and putting it in the BaseChannel constructor
instead. This is more intuitive, and provides the added assurance that
"secure_required_" can't be flipped from "true" to "false".

BUG=None

Committed: https://crrev.com/7af91ddd6bda4f411d0025c5d5a2d32be540a34c
Cr-Commit-Position: refs/heads/master@{#15579}

[Taylor Brandstetter, 2016-11-30 01:18:30]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2016-11-30 01:18:31]

Peter, could you take a look? Should be pretty simple. I think "crypto_required"
must be a vestigial appendage from the days of XMPP.

[pthatcher1, 2016-11-30 19:16:17]

Good find of something that can be removed.  But I think we can remove even more
:)

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/rtcstatscollector_...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:104: const bool kDefaultUseRtcpChannel
= false;
"bool rtcp" in BaseChannel is really "rtcp_enabled_", which means "Is RTCP used
at all by this type of channel?"
(https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.h?sq=packa...)

So this should probably be called kDefaultRtcpEnabled.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/rtpsenderreceiver_...
File webrtc/api/rtpsenderreceiver_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/rtpsenderreceiver_...
webrtc/api/rtpsenderreceiver_unittest.cc:65: bool rtcp = false;
Similarly, you might as well called this rtcp_enabled to be more clear.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1635: bundle_transport,
create_rtcp_transport_channel, SecureRequired(),
Does SecureRequired()/srtp_required() need to be public?  It seems like it could
be private.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/webrtcsession.h#ne...
webrtc/api/webrtcsession.h:203: bool SecureRequired() const;
"SecureRequired" sounds funny, and the logic/meaning down below is really "SRTP
is required".  So why not just call this "srtp_required" as a variable and
"srtp_required()" as a method name?

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h#newcode80
webrtc/pc/channel.h:80: // media without using SRTP (either using SDES or
DTLS-SRTP).
More specifically, it won't send RTP that's not SRTP.  It will sends SCTP (but
only with DTLS).  All the more reason to call this "srtp_required".

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h#newcode107
webrtc/pc/channel.h:107: bool secure_required() const { return secure_required_;
}
Nothing ever calls this, so I think we can remove it.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h#newcode400
webrtc/pc/channel.h:400: bool secure_required_ = false;
Perhaps we should make the default true now.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h#newcode400
webrtc/pc/channel.h:400: bool secure_required_ = false;
We can rename this to srtp_required_ in just 2-3 places where it is used.  I

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channelmanager.h#ne...
webrtc/pc/channelmanager.h:184: DataChannelType data_channel_type);
Same here.  We should be able to just assume this is always true.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/mediasession.cc#new...
webrtc/pc/mediasession.cc:777: if (secure_policy == SEC_REQUIRED &&
offer->cryptos().empty()) {
I looked around at all the uses of the SecurePolicy enum.  In WebRtcSession and
WebRtcSessionDesriptionFactory and in mediasession.cc, we can replace it all
with "bool require_srtp", because only DISABLED and REQUIRED are used, and they
mean "require_srtp" and "!require_srtp"

With TransportDescriptionFactory, we can replace it with "dtls_enabled", because
it only uses DISABLED and ENABLED, and they mean "!dtls_enabled" and
"dtls_enabled".

We can do that in a separate CL, but it's nice to know that we can get rid of
SecurePolicy its weird name altogether.

Similarly, in this file, we can remove "secure_transport" and replace it with
"dlts_enabled"

[Taylor Brandstetter, 2016-12-01 02:41:34]

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/rtcstatscollector_...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:104: const bool kDefaultUseRtcpChannel
= false;
On 2016/11/30 19:16:17, pthatcher1 wrote:
> "bool rtcp" in BaseChannel is really "rtcp_enabled_", which means "Is RTCP
used
> at all by this type of channel?"
>
(https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.h?sq=packa...)
> 
> So this should probably be called kDefaultRtcpEnabled. 

Done. I'm just bad at naming things.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/rtpsenderreceiver_...
File webrtc/api/rtpsenderreceiver_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/rtpsenderreceiver_...
webrtc/api/rtpsenderreceiver_unittest.cc:65: bool rtcp = false;
On 2016/11/30 19:16:17, pthatcher1 wrote:
> Similarly, you might as well called this rtcp_enabled to be more clear.

Done.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1635: bundle_transport,
create_rtcp_transport_channel, SecureRequired(),
On 2016/11/30 19:16:17, pthatcher1 wrote:
> Does SecureRequired()/srtp_required() need to be public?  It seems like it
could
> be private.

I guess so. I just made it public because SdesPolicy already is, and it's even
more granular.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/api/webrtcsession.h#ne...
webrtc/api/webrtcsession.h:203: bool SecureRequired() const;
On 2016/11/30 19:16:17, pthatcher1 wrote:
> "SecureRequired" sounds funny, and the logic/meaning down below is really
"SRTP
> is required".  So why not just call this "srtp_required" as a variable and
> "srtp_required()" as a method name?

This is just the name we're using elsewhere, so I used it here for consistency.
I'll change it though, I agree with you.

And for your second question: I didn't add a new variable because this can be
determined from existing variables.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h#newcode80
webrtc/pc/channel.h:80: // media without using SRTP (either using SDES or
DTLS-SRTP).
On 2016/11/30 19:16:17, pthatcher1 wrote:
> More specifically, it won't send RTP that's not SRTP.  It will sends SCTP (but
> only with DTLS).  All the more reason to call this "srtp_required".

Done.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h#newcode107
webrtc/pc/channel.h:107: bool secure_required() const { return secure_required_;
}
On 2016/11/30 19:16:17, pthatcher1 wrote:
> Nothing ever calls this, so I think we can remove it.

It's used by unit tests. It's a bad way of unit testing, but I don't want to
commit to fixing it in this CL, I don't think it would be trivial.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h#newcode400
webrtc/pc/channel.h:400: bool secure_required_ = false;
On 2016/11/30 19:16:17, pthatcher1 wrote:
> We can rename this to srtp_required_ in just 2-3 places where it is used.  I

Done.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channel.h#newcode400
webrtc/pc/channel.h:400: bool secure_required_ = false;
On 2016/11/30 19:16:17, pthatcher1 wrote:
> Perhaps we should make the default true now.
> 

It's always initialized in the constructor so that does seem the safest.

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/channelmanager.h#ne...
webrtc/pc/channelmanager.h:184: DataChannelType data_channel_type);
On 2016/11/30 19:16:17, pthatcher1 wrote:
> Same here.  We should be able to just assume this is always true.

I'm not sure what you're referring to when you say "same here" or "this"

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2537343003/diff/1/webrtc/pc/mediasession.cc#new...
webrtc/pc/mediasession.cc:777: if (secure_policy == SEC_REQUIRED &&
offer->cryptos().empty()) {
On 2016/11/30 19:16:17, pthatcher1 wrote:
> I looked around at all the uses of the SecurePolicy enum.  In WebRtcSession
and
> WebRtcSessionDesriptionFactory and in mediasession.cc, we can replace it all
> with "bool require_srtp", because only DISABLED and REQUIRED are used, and
they
> mean "require_srtp" and "!require_srtp"
> 
> With TransportDescriptionFactory, we can replace it with "dtls_enabled",
because
> it only uses DISABLED and ENABLED, and they mean "!dtls_enabled" and
> "dtls_enabled".
> 
> We can do that in a separate CL, but it's nice to know that we can get rid of
> SecurePolicy its weird name altogether.
> 
> Similarly, in this file, we can remove "secure_transport" and replace it with
> "dlts_enabled"

Sounds good; I'll keep that in mind next time this code bites me.

[pthatcher1, 2016-12-01 22:57:12]

lgtm w/nits

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector_unittest.cc:105: const bool kDefaultSecureRequired
= true;
kDefaultSrtpRequired?

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/api/statscollector...
File webrtc/api/statscollector_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/api/statscollector...
webrtc/api/statscollector_unittest.cc:54: const bool kDefaultSecureRequired =
true;
kDefaultSrtpRequired?

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:107: bool srtp_required() const { return srtp_required_; }
Could you leave a comment that this is only public for testing?

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/channelmanager_...
File webrtc/pc/channelmanager_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/channelmanager_...
webrtc/pc/channelmanager_unittest.cc:25: const bool kDefaultSecureRequired =
true;
kDefaultSrtpRequired?

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:777: if (secure_policy == SEC_REQUIRED &&
offer->cryptos().empty()) {
Could you put a TODO on the SecurePolicy enum to remove it?

[Taylor Brandstetter, 2016-12-02 00:42:32]

I just realized, adding a new parameter to CreateVoiceChannel etc. is going to
break applications that use ChannelManager directly. I added overloads for each
method for backwards compatibility; is this ok or would you suggest something
else?

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector_unittest.cc:105: const bool kDefaultSecureRequired
= true;
On 2016/12/01 22:57:12, pthatcher1 wrote:
> kDefaultSrtpRequired?

Done.

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/api/statscollector...
File webrtc/api/statscollector_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/api/statscollector...
webrtc/api/statscollector_unittest.cc:54: const bool kDefaultSecureRequired =
true;
On 2016/12/01 22:57:12, pthatcher1 wrote:
> kDefaultSrtpRequired?

Done.

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:107: bool srtp_required() const { return srtp_required_; }
On 2016/12/01 22:57:12, pthatcher1 wrote:
> Could you leave a comment that this is only public for testing?

Done.

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/channelmanager_...
File webrtc/pc/channelmanager_unittest.cc (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/channelmanager_...
webrtc/pc/channelmanager_unittest.cc:25: const bool kDefaultSecureRequired =
true;
On 2016/12/01 22:57:12, pthatcher1 wrote:
> kDefaultSrtpRequired?

Done.

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2537343003/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:777: if (secure_policy == SEC_REQUIRED &&
offer->cryptos().empty()) {
On 2016/12/01 22:57:12, pthatcher1 wrote:
> Could you put a TODO on the SecurePolicy enum to remove it?

Done.

[pthatcher1, 2016-12-05 23:37:57]

lgtm

[Taylor Brandstetter, 2016-12-13 18:50:47]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-12-13 18:50:47]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2537343003/#ps100001 (title:
"Merge with master and clean up CreateDataChannel")

[commit-bot: I haz the power, 2016-12-13 18:50:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-13 19:29:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481655047215500,
"parent_rev": "3ca75ea7c7a0ccd791221b4478dc8aeed14f6907", "commit_rev":
"46762d0068821343058fe7c5c9f3caf13d778d01"}

[commit-bot: I haz the power, 2016-12-13 19:29:15]

Description was changed from

==========
Removing "crypto_required" from MediaContentDescription.

"Crypto required" is a property of the PeerConnection of construction
time; it has nothing to do with SDP. So I'm moving it out of
MediaContentDescription and putting it in the BaseChannel constructor
instead. This is more intuitive, and provides the added assurance that
"secure_required_" can't be flipped from "true" to "false".

BUG=None
==========

to

==========
Removing "crypto_required" from MediaContentDescription.

"Crypto required" is a property of the PeerConnection of construction
time; it has nothing to do with SDP. So I'm moving it out of
MediaContentDescription and putting it in the BaseChannel constructor
instead. This is more intuitive, and provides the added assurance that
"secure_required_" can't be flipped from "true" to "false".

BUG=None

Review-Url: https://codereview.webrtc.org/2537343003
==========

[commit-bot: I haz the power, 2016-12-13 19:29:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-13 19:29:23]

Description was changed from

==========
Removing "crypto_required" from MediaContentDescription.

"Crypto required" is a property of the PeerConnection of construction
time; it has nothing to do with SDP. So I'm moving it out of
MediaContentDescription and putting it in the BaseChannel constructor
instead. This is more intuitive, and provides the added assurance that
"secure_required_" can't be flipped from "true" to "false".

BUG=None

Review-Url: https://codereview.webrtc.org/2537343003
==========

to

==========
Removing "crypto_required" from MediaContentDescription.

"Crypto required" is a property of the PeerConnection of construction
time; it has nothing to do with SDP. So I'm moving it out of
MediaContentDescription and putting it in the BaseChannel constructor
instead. This is more intuitive, and provides the added assurance that
"secure_required_" can't be flipped from "true" to "false".

BUG=None

Committed: https://crrev.com/7af91ddd6bda4f411d0025c5d5a2d32be540a34c
Cr-Commit-Position: refs/heads/master@{#15579}
==========

[commit-bot: I haz the power, 2016-12-13 19:29:24]

Patchset 6 (id:??) landed as
https://crrev.com/7af91ddd6bda4f411d0025c5d5a2d32be540a34c
Cr-Commit-Position: refs/heads/master@{#15579}