Make P2PTransportChannel inherit from IceTransportInternal.

Make P2PTransportChannel inherit from IceTransportInternal.

Make P2PTransportChannel inherit from IceTransportInternal instead of
TransportChannelImpl and TransportChannel, so that the DTLS-related methods can
be separated from P2PTransportChannel.

BUG=none

Review-Url: https://codereview.webrtc.org/2590063002
Cr-Commit-Position: refs/heads/master@{#15743}
Committed: https://chromium.googlesource.com/external/webrtc/+/12749d89d9f004cc1961aa6ece6600cc14c57ac5

[Zhi Huang, 2016-12-20 02:29:08]

Patchset #1 (id:1) has been deleted

[Zhi Huang, 2016-12-20 02:29:15]

Patchset #1 (id:20001) has been deleted

[Taylor Brandstetter, 2016-12-20 03:37:25]

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

[Taylor Brandstetter, 2016-12-20 03:37:26]

This is great; I just noticed a few minor things.

By the way, we may want to double check that this doesn't break chromium or
anything else before submitting.

Also, I think we can leave the design doc link out of the CL description since
it's only visible internally.

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/faketrans...
File webrtc/p2p/base/faketransportcontroller.h (right):

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:134: // If false, it affects |dest| as
well.
Should update this comment

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:136: LOG(INFO) << "here " <<
STATE_INIT;
I think this log statement and the one below are left over from debugging

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
File webrtc/p2p/base/icetransportinternal.h (right):

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:28: enum class TransportState {
Sorry for not catching this earlier, but: if this is only used to represent an
ICE state, can we call it IceTransportState? And leave a TODO to replace it with
IceConnectionState later
(https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnectionint...)

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:47: virtual ~IceTransportInternal(){};
Does this destructor need a body?

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:129: // Emitted whenever the transport
channel state changed.
transport channel -> transport

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:136: virtual const std::string
debug_name() const {
nit: Doesn't need to return a const string.

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:156: }
Is this meant to override debug_string?

[Zhi Huang, 2016-12-20 20:17:43]

Please take another look. Thanks.

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/faketrans...
File webrtc/p2p/base/faketransportcontroller.h (right):

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:134: // If false, it affects |dest| as
well.
On 2016/12/20 03:37:26, Taylor Brandstetter wrote:
> Should update this comment

Done.

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:136: LOG(INFO) << "here " <<
STATE_INIT;
On 2016/12/20 03:37:26, Taylor Brandstetter wrote:
> I think this log statement and the one below are left over from debugging

Aha, yep, this is for debugging.

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
File webrtc/p2p/base/icetransportinternal.h (right):

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:28: enum class TransportState {
On 2016/12/20 03:37:26, Taylor Brandstetter wrote:
> Sorry for not catching this earlier, but: if this is only used to represent an
> ICE state, can we call it IceTransportState? And leave a TODO to replace it
with
> IceConnectionState later
>
(https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnectionint...)

Done.

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:47: virtual ~IceTransportInternal(){};
On 2016/12/20 03:37:26, Taylor Brandstetter wrote:
> Does this destructor need a body?

Done.

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:129: // Emitted whenever the transport
channel state changed.
On 2016/12/20 03:37:26, Taylor Brandstetter wrote:
> transport channel -> transport

Done.

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:136: virtual const std::string
debug_name() const {
On 2016/12/20 03:37:26, Taylor Brandstetter wrote:
> nit: Doesn't need to return a const string.

I meant to override the PacketTransportInterface::debug_name().
I'll change it to 
 
   const std::string debug_name() const override {...}

Or do you think it's better to change the method signature in
PacketTransportInterface?

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:156: }
On 2016/12/20 03:37:26, Taylor Brandstetter wrote:
> Is this meant to override debug_string?

This is only used for libjingle's logging. (LOG_J).

[Taylor Brandstetter, 2016-12-20 20:27:15]

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
File webrtc/p2p/base/icetransportinternal.h (right):

https://codereview.webrtc.org/2590063002/diff/40001/webrtc/p2p/base/icetransp...
webrtc/p2p/base/icetransportinternal.h:136: virtual const std::string
debug_name() const {
On 2016/12/20 20:17:43, Zhi Huang wrote:
> On 2016/12/20 03:37:26, Taylor Brandstetter wrote:
> > nit: Doesn't need to return a const string.
> 
> I meant to override the PacketTransportInterface::debug_name().
> I'll change it to 
>  
>    const std::string debug_name() const override {...}
> 
> Or do you think it's better to change the method signature in
> PacketTransportInterface?

Ah, I see. I think in PacketTransportInterface it was meant to return a const
ref. That's fine, although that would mean you can't construct the debug name in
this method, it would need to be stored as a member variable. Which means the
implementation should move to P2PTransportChannel/FakeIceTransport.

Or you can just return a string by value; since it's only used for debug log
statements, I don't think the performance overhead is important.

[Taylor Brandstetter, 2016-12-20 23:50:01]

Description was changed from

==========
Make the P2PTransportChannel depend on the IceTransportInternal.

Make the P2PTransportChannel depend on the IceTransportInternal instead of the
TransportChannelImpl and TransportChannel So that the Dtls related methods can
be separated from the P2PTransportChannel.

Related Design Doc:
https://docs.google.com/document/d/1g9RA0s4RV7hFAcWiAM2b6H5ZohAVpBqcYienEDj6I...

BUG=none
==========

to

==========
Make P2PTransportChannel inherit from IceTransportInternal.

Make P2PTransportChannel inherit from IceTransportInternal instead of
TransportChannelImpl and TransportChannel, so that the DTLS-related methods can
be separated from P2PTransportChannel.

BUG=none
==========

[Taylor Brandstetter, 2016-12-20 23:50:45]

lgtm. I also changed the CL description a bit

[Zhi Huang, 2016-12-21 00:06:05]

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 00:06:12]

Dry run: 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-21 01:16:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 01:16:57]

Dry run: This issue passed the CQ dry run.

[Zhi Huang, 2016-12-21 18:24:37]

The CQ bit was checked by zhihuang@webrtc.org

[commit-bot: I haz the power, 2016-12-21 18:24:42]

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-21 18:26:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482344677525820,
"parent_rev": "eec99094e3a6ac099043907b9689f2d9e2822ac7", "commit_rev":
"12749d89d9f004cc1961aa6ece6600cc14c57ac5"}

[commit-bot: I haz the power, 2016-12-21 18:26:22]

Description was changed from

==========
Make P2PTransportChannel inherit from IceTransportInternal.

Make P2PTransportChannel inherit from IceTransportInternal instead of
TransportChannelImpl and TransportChannel, so that the DTLS-related methods can
be separated from P2PTransportChannel.

BUG=none
==========

to

==========
Make P2PTransportChannel inherit from IceTransportInternal.

Make P2PTransportChannel inherit from IceTransportInternal instead of
TransportChannelImpl and TransportChannel, so that the DTLS-related methods can
be separated from P2PTransportChannel.

BUG=none

Review-Url: https://codereview.webrtc.org/2590063002
Cr-Commit-Position: refs/heads/master@{#15743}
Committed:
https://chromium.googlesource.com/external/webrtc/+/12749d89d9f004cc1961aa6ec...
==========

[commit-bot: I haz the power, 2016-12-21 18:26:23]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/12749d89d9f004cc1961aa6ec...

[kjellander_webrtc, 2016-12-22 07:51:40]

A revert of this CL (patchset #3 id:80001) has been created in
https://codereview.webrtc.org/2594343002/ by kjellander@webrtc.org.

The reason for reverting is: Breaks Chromium WebRTC FYI bots:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Builder/bui...
The error was masked by another breaking change that was committer earlier. This
is the first build showing the error..