Exposing DTLS transport state from TransportChannel.

Exposing DTLS transport state from TransportChannel.

This is necessary in order to support the RTCPeerConnectionState enum in
the future, as well as a correct RTCIceConnectionState (which isn't a
combination ICE and DTLS state).

Committed: https://crrev.com/2b5586774ca71b19cbebc38c96d1476ccb3dfdb4
Cr-Commit-Position: refs/heads/master@{#10419}

[Taylor Brandstetter, 2015-10-20 20:09:04]

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

[pthatcher1, 2015-10-20 20:28:07]

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:580:
set_dtls_transport_state(DTLS_TRANSPORT_CONNECTING);
Please see if we can reduce down to one dtls_state_ instead of two.

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportchannel.cc (right):

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.cc:47: if (dtls_transport_state_ != state) {
Nit: I'd prefer an early return

if (dtls_transport_state_ == state) {
  return;
}

...

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportchannel.h (right):

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.h:79: }
I agree with the type name (DtlsTransportState), but I think we might want to
call this dtls_state().

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.h:85: sigslot::signal1<TransportChannel*>
SignalDtlsTransportState;
And this SignalDtlsState.

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.h:169: void
set_dtls_transport_state(DtlsTransportState state);
And this set_dtls_state.

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.h:177: DtlsTransportState dtls_transport_state_
= DTLS_TRANSPORT_NEW;
And this dtls_state_ :).

[Taylor Brandstetter, 2015-10-21 16:28:16]

Here's my pass at merging the DTLS states. "STATE_NONE" becomes "!dtls_active_"
(which I think is actually more intuitive), and "STATE_OFFERED"/"STATE_ACCEPTED"
are differentiated by whether dtls_ is NULL or not (which is less intuitive). If
you think this is too confusing, I'll go with the "state mapping" approach
instead.

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:580:
set_dtls_transport_state(DTLS_TRANSPORT_CONNECTING);
On 2015/10/20 20:28:06, pthatcher1 wrote:
> Please see if we can reduce down to one dtls_state_ instead of two.

Done.

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportchannel.cc (right):

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.cc:47: if (dtls_transport_state_ != state) {
On 2015/10/20 20:28:06, pthatcher1 wrote:
> Nit: I'd prefer an early return
> 
> if (dtls_transport_state_ == state) {
>   return;
> }
> 
> ...

Done.

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportchannel.h (right):

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.h:79: }
On 2015/10/20 20:28:07, pthatcher1 wrote:
> I agree with the type name (DtlsTransportState), but I think we might want to
> call this dtls_state().

That's what I had originally, then I renamed it halfway through... Guess I
should go with my gut more often. :)

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.h:85: sigslot::signal1<TransportChannel*>
SignalDtlsTransportState;
On 2015/10/20 20:28:07, pthatcher1 wrote:
> And this SignalDtlsState.

Done.

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.h:169: void
set_dtls_transport_state(DtlsTransportState state);
On 2015/10/20 20:28:06, pthatcher1 wrote:
> And this set_dtls_state.

Done.

https://codereview.webrtc.org/1414363002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannel.h:177: DtlsTransportState dtls_transport_state_
= DTLS_TRANSPORT_NEW;
On 2015/10/20 20:28:07, pthatcher1 wrote:
> And this dtls_state_ :).

Done.

[pthatcher1, 2015-10-22 07:05:00]

I like it.  Just some little cleanup could be done.

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:351: result = -1;
Why not just "return -1"?

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:356: break;
Why not just "return -1"?

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:363: break;
Why not just "return -1"?

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:366: result = channel_->SendPacket(data,
size, options);
Why not just "return channel_->SendPacket(data, size, options)"?

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:369: rtc::SR_SUCCESS) ?
static_cast<int>(size) : -1;
Why not just "return ..."?

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:408: if (dtls_ && !MaybeStartDtls()) {
Why not just put 

if (!dtls_) {
  return true;
}

Into MaybeStartDtls()?  Then this could be:

ASSERT(MaybeStartDtls());

[Taylor Brandstetter, 2015-10-22 19:30:26]

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:351: result = -1;
On 2015/10/22 07:05:00, pthatcher1 wrote:
> Why not just "return -1"?

Done.

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:356: break;
On 2015/10/22 07:05:00, pthatcher1 wrote:
> Why not just "return -1"?

Done.

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:363: break;
On 2015/10/22 07:05:00, pthatcher1 wrote:
> Why not just "return -1"?

Done.

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:366: result = channel_->SendPacket(data,
size, options);
On 2015/10/22 07:05:00, pthatcher1 wrote:
> Why not just "return channel_->SendPacket(data, size, options)"?

Done.

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:369: rtc::SR_SUCCESS) ?
static_cast<int>(size) : -1;
On 2015/10/22 07:05:00, pthatcher1 wrote:
> Why not just "return ..."?

Done.

https://codereview.webrtc.org/1414363002/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.cc:408: if (dtls_ && !MaybeStartDtls()) {
On 2015/10/22 07:05:00, pthatcher1 wrote:
> Why not just put 
> 
> if (!dtls_) {
>   return true;
> }
> 
> Into MaybeStartDtls()?  Then this could be:
> 
> ASSERT(MaybeStartDtls());

Done. Though need to use VERIFY, since ASSERT is a no-op on release builds.

[pthatcher1, 2015-10-22 23:16:58]

lgtm

[Taylor Brandstetter, 2015-10-26 22:40:43]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2015-10-26 22:40:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414363002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414363002/100001

[commit-bot: I haz the power, 2015-10-27 00:23:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-27 00:23:44]

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