Remove the dependency of TransportChannel and TransportChannelImpl.

Remove the dependency of TransportChannel and TransportChannelImpl.

DtlsTransportChannelWrapper is renamed to be DtlsTransport which inherits from 
DtlsTransportInternal. There will be no concept of "channel" in p2p level. 
Both P2PTransportChannel and DtlsTransport don't depend on TransportChannel 
and TransportChannelImpl any more and they are removed in this CL.

BUG=none

Review-Url: https://codereview.webrtc.org/2606123002
Cr-Commit-Position: refs/heads/master@{#16173}
Committed: https://chromium.googlesource.com/external/webrtc/+/b2cdd93fd6ab91d453314ac2f51a849542d023d9

[Zhi Huang, 2017-01-02 20:28:16]

Patchset #1 (id:1) has been deleted

[Zhi Huang, 2017-01-03 19:28:56]

Patchset #1 (id:20001) has been deleted

[Taylor Brandstetter, 2017-01-03 23:21:58]

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

[Taylor Brandstetter, 2017-01-03 23:21:59]

Looks good; the main thing I saw that could be cleaned up is the code
duplication between FakeTransportChannel and FakeDtlsTransport. Though that
assumes the "IceTransportInternal" CLs are landed before this one.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.h:160: TransportChannelImpl*
ice_transport() override { return channel_; }
Can this be IceTransportInternal instead now (assuming this CL is landed after
the one that introduces IceTransportInternal)?

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.h:171: int GetError() override { return
channel_->GetError(); }
Since this and SetOption just pass through to the ICE channel, can they be
removed? If not, can you leave a TODO and describe what would need to be done?

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:235: bool
all_raw_channels_writable() const {
nit: Since it now makes sense, can you rename this to
"all_ice_channels_writable"?

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportinternal.h:31: DtlsTransportInternal() {}
nit: I was wrong about the destructor earlier, but I don't think the empty
constructor is needed.

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

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:49: class FakeTransportChannel :
public TransportChannelImpl,
Should this be renamed to FakeIceTransport, and inherit from
IceTransportInternal? Then we could remove some of the code here that's
duplicated in FakeDtlsTransport.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:378: void SetAsyncDelay(int delay_ms)
{ async_delay_ms_ = delay_ms; }
I think the async settings only need to go in one place (the fake ICE channel?).

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:472: 
Can this just call ice_transport_->SendPacket, and listen SignalReadPacket on
ice_transport_?

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:721: // will change when we get rid of
TransportChannelImpl.
Should update this comment.

[Zhi Huang, 2017-01-10 01:40:43]

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

[commit-bot: I haz the power, 2017-01-10 01:40:47]

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, 2017-01-10 01:44:39]

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

[commit-bot: I haz the power, 2017-01-10 01:44:40]

Dry run: Try jobs failed on following builders:
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)

[Zhi Huang, 2017-01-10 18:30:51]

Please take a look.
If possible, I think we can land this one which includes both ice transport
changes and dtls transport change.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.h:160: TransportChannelImpl*
ice_transport() override { return channel_; }
On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> Can this be IceTransportInternal instead now (assuming this CL is landed after
> the one that introduces IceTransportInternal)?

Yes, I'll merge the IceTransportInternal change into this CL.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.h:171: int GetError() override { return
channel_->GetError(); }
On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> Since this and SetOption just pass through to the ICE channel, can they be
> removed? If not, can you leave a TODO and describe what would need to be done?

These functions are from the PacketTransportInterface. But we can hide them by
implementing them in the DtlsTransportInternal.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:235: bool
all_raw_channels_writable() const {
On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> nit: Since it now makes sense, can you rename this to
> "all_ice_channels_writable"?

What about "all_ice_transport_writable", since there is no concept of "channel"
after the refactoring.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportinternal.h:31: DtlsTransportInternal() {}
On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> nit: I was wrong about the destructor earlier, but I don't think the empty
> constructor is needed.

It doesn't compile without it.

I think the macro I add, "RTC_DISALLOW_COPY_AND_ASSIGN", will delete the default
constructor of this class. But I'm not sure. I can make it protected like this
https://cs.chromium.org/chromium/src/third_party/webrtc/base/messagehandler.h...

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

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:49: class FakeTransportChannel :
public TransportChannelImpl,
On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> Should this be renamed to FakeIceTransport, and inherit from
> IceTransportInternal? Then we could remove some of the code here that's
> duplicated in FakeDtlsTransport.

Done.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:378: void SetAsyncDelay(int delay_ms)
{ async_delay_ms_ = delay_ms; }
On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> I think the async settings only need to go in one place (the fake ICE
channel?).

I'll clean up the duplication when merging the two changes.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:472: 
On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> Can this just call ice_transport_->SendPacket, and listen SignalReadPacket on
> ice_transport_?

Done.

[Taylor Brandstetter, 2017-01-10 19:43:47]

Just some minor comments.

It was a good idea combining this CL with the IceTransportInternal one (assuming
we'll able to land it); it makes the "fake transport" code a lot cleaner.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:235: bool
all_raw_channels_writable() const {
On 2017/01/10 18:30:51, Zhi Huang wrote:
> On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> > nit: Since it now makes sense, can you rename this to
> > "all_ice_channels_writable"?
> 
> What about "all_ice_transport_writable", since there is no concept of
"channel"
> after the refactoring.

Right, sounds good.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportinternal.h:31: DtlsTransportInternal() {}
On 2017/01/10 18:30:51, Zhi Huang wrote:
> On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> > nit: I was wrong about the destructor earlier, but I don't think the empty
> > constructor is needed.
> 
> It doesn't compile without it.
> 
> I think the macro I add, "RTC_DISALLOW_COPY_AND_ASSIGN", will delete the
default
> constructor of this class. But I'm not sure. I can make it protected like this
>
https://cs.chromium.org/chromium/src/third_party/webrtc/base/messagehandler.h...

Does using "RTC_DISALLOW_IMPLICIT_CONSTRUCTORS" instead work?

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportinternal.h:99: const std::string debug_name() const
override {
Like I mentioned earlier, we can remove the "const" from the return value.

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/mockicetr...
File webrtc/p2p/base/mockicetransport.h (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/mockicetr...
webrtc/p2p/base/mockicetransport.h:73: std::string transport_name_;
Looks like this is never set? Can transport_name() just return an empty string
instead?

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/packettra...
File webrtc/p2p/base/packettransportinterface.h (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/packettra...
webrtc/p2p/base/packettransportinterface.h:17: #include
"webrtc/base/asyncpacketsocket.h"
Can you leave a comment that this included for PacketOptions?

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:550: << transport;
Leftover debug message

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:798: LOG(INFO) << __FUNCTION__ << " " << (long)data;
Also here

[Zhi Huang, 2017-01-12 19:48:19]

Patchset #4 (id:100001) has been deleted

[Zhi Huang, 2017-01-12 20:04:13]

Please take a look.
Thanks.

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/40001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportinternal.h:31: DtlsTransportInternal() {}
On 2017/01/10 19:43:47, Taylor Brandstetter wrote:
> On 2017/01/10 18:30:51, Zhi Huang wrote:
> > On 2017/01/03 23:21:59, Taylor Brandstetter wrote:
> > > nit: I was wrong about the destructor earlier, but I don't think the empty
> > > constructor is needed.
> > 
> > It doesn't compile without it.
> > 
> > I think the macro I add, "RTC_DISALLOW_COPY_AND_ASSIGN", will delete the
> default
> > constructor of this class. But I'm not sure. I can make it protected like
this
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/base/messagehandler.h...
> 
> Does using "RTC_DISALLOW_IMPLICIT_CONSTRUCTORS" instead work?

This macro will declare the default constructor to be deleted constructor. It
doesn't work when constructing the subclasses. The error will be: "error: call
to deleted constructor of 'cricket::DtlsTransportInternal'"

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportinternal.h:99: const std::string debug_name() const
override {
On 2017/01/10 19:43:47, Taylor Brandstetter wrote:
> Like I mentioned earlier, we can remove the "const" from the return value.

Done.

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/mockicetr...
File webrtc/p2p/base/mockicetransport.h (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/mockicetr...
webrtc/p2p/base/mockicetransport.h:73: std::string transport_name_;
On 2017/01/10 19:43:47, Taylor Brandstetter wrote:
> Looks like this is never set? Can transport_name() just return an empty string
> instead?

A local variable won't work here because the transport_name() will return a
const reference. Unless we change the return type of transport_name().

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/packettra...
File webrtc/p2p/base/packettransportinterface.h (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/packettra...
webrtc/p2p/base/packettransportinterface.h:17: #include
"webrtc/base/asyncpacketsocket.h"
On 2017/01/10 19:43:47, Taylor Brandstetter wrote:
> Can you leave a comment that this included for PacketOptions?

Done.

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:550: << transport;
On 2017/01/10 19:43:47, Taylor Brandstetter wrote:
> Leftover debug message

Done.

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:798: LOG(INFO) << __FUNCTION__ << " " << (long)data;
On 2017/01/10 19:43:47, Taylor Brandstetter wrote:
> Also here

Done.

[Zhi Huang, 2017-01-12 22:26:05]

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

[commit-bot: I haz the power, 2017-01-12 22:26:09]

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, 2017-01-12 22:44:19]

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

[commit-bot: I haz the power, 2017-01-12 22:44:20]

Dry run: Try jobs failed on following builders:
  win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)

[Zhi Huang, 2017-01-12 22:59:36]

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

[commit-bot: I haz the power, 2017-01-12 22:59:39]

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, 2017-01-13 00:00:04]

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

[commit-bot: I haz the power, 2017-01-13 00:00:05]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-01-13 01:45:58]

lgtm. By the way, when you rebase/merge, try to upload it as a separate
patchset; that just makes it easier to see the diffs in the code review tool.

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/mockicetr...
File webrtc/p2p/base/mockicetransport.h (right):

https://codereview.webrtc.org/2606123002/diff/80001/webrtc/p2p/base/mockicetr...
webrtc/p2p/base/mockicetransport.h:73: std::string transport_name_;
On 2017/01/12 20:04:12, Zhi Huang wrote:
> On 2017/01/10 19:43:47, Taylor Brandstetter wrote:
> > Looks like this is never set? Can transport_name() just return an empty
string
> > instead?
> 
> A local variable won't work here because the transport_name() will return a
> const reference. Unless we change the return type of transport_name().

Acknowledged.

[pthatcher1, 2017-01-13 22:41:15]

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

[pthatcher1, 2017-01-13 22:41:16]

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1923: transport_name,
cricket::ICE_CANDIDATE_COMPONENT_RTP);
If it returns a DtlsTransport, then we should rename the TransportController to
be CreateDtlsTransport.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:54: IceTransportInternal* channel_;  //
owned by DtlsTransportChannelWrapper
We should rename the variable ice_transport_, or maybe just transport_.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/icetrans...
File webrtc/p2p/base/icetransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/icetrans...
webrtc/p2p/base/icetransportinternal.h:29: typedef IceTransportInternal
IceTransportInternal2;
IceTransportInternal2?  What is that?

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/icetrans...
webrtc/p2p/base/icetransportinternal.h:31: // TODO(zhihuang): replace it with
PeerConnectionInterface::IceConnectionState.
"replace it" => "Replace this"

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:344: DtlsTransportInternal* new_channel) {
We should rename this method to SetTransport or SetRtpTransport.  Basically no
longer have "transport_channel" or "TransportChannel" anywhere in this file.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:166: DtlsTransportInternal* rtcp_transport_channel() const {
We should call these rtp_dtls_transport and rtcp_dtls_transport.  Or something
with "dtls" in the name.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:232: void
DisconnectFromTransportChannel(DtlsTransportInternal* tc);
This should be ConnectToTransport and DisconnectFromTransport (not channel).

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:392: DtlsTransportInternal* rtcp_transport_channel_ =
nullptr;
Same here with the names.

[Taylor Brandstetter, 2017-01-13 23:49:55]

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/icetrans...
File webrtc/p2p/base/icetransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/icetrans...
webrtc/p2p/base/icetransportinternal.h:29: typedef IceTransportInternal
IceTransportInternal2;
On 2017/01/13 22:41:16, pthatcher1 wrote:
> IceTransportInternal2?  What is that?

Long story. Basically it's part of what we decided is the least painful  dance
for replacing "TransportChannel"/"TransportChannelImpl" with
"IceTransportInternal" given that the former are used by remoting. It will be
cleaned up in a follow up chromium CL/webrtc CL.

[Zhi Huang, 2017-01-16 10:22:14]

Patchset #8 (id:200001) has been deleted

[Zhi Huang, 2017-01-16 10:27:30]

Patchset #7 (id:180001) has been deleted

[Zhi Huang, 2017-01-16 10:38:25]

Hi,
Please take another look. I addressed the comments regarding the confusing
naming.

I was aware that the naming becomes a little bit confusing. My previous plan was
having a separate CL for renaming and clean up since the concept of "channel" is
used at many place.  

Thanks.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession.cc:1923: transport_name,
cricket::ICE_CANDIDATE_COMPONENT_RTP);
On 2017/01/13 22:41:16, pthatcher1 wrote:
> If it returns a DtlsTransport, then we should rename the TransportController
to
> be CreateDtlsTransport.

Sure. There will be no concept of "channel" in P2P level. 
Done

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:54: IceTransportInternal* channel_;  //
owned by DtlsTransportChannelWrapper
On 2017/01/13 22:41:16, pthatcher1 wrote:
> We should rename the variable ice_transport_, or maybe just transport_.

Done.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/icetrans...
File webrtc/p2p/base/icetransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/p2p/base/icetrans...
webrtc/p2p/base/icetransportinternal.h:31: // TODO(zhihuang): replace it with
PeerConnectionInterface::IceConnectionState.
On 2017/01/13 22:41:16, pthatcher1 wrote:
> "replace it" => "Replace this"

Done.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:344: DtlsTransportInternal* new_channel) {
On 2017/01/13 22:41:16, pthatcher1 wrote:
> We should rename this method to SetTransport or SetRtpTransport.  Basically no
> longer have "transport_channel" or "TransportChannel" anywhere in this file.

I'll name it "SetTransport_n".
Then the "SetTransports_n" will call "SetTransport_n".

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:166: DtlsTransportInternal* rtcp_transport_channel() const {
On 2017/01/13 22:41:16, pthatcher1 wrote:
> We should call these rtp_dtls_transport and rtcp_dtls_transport.  Or something
> with "dtls" in the name.

Done.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:232: void
DisconnectFromTransportChannel(DtlsTransportInternal* tc);
On 2017/01/13 22:41:16, pthatcher1 wrote:
> This should be ConnectToTransport and DisconnectFromTransport (not channel).

Done.

https://codereview.webrtc.org/2606123002/diff/140001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:392: DtlsTransportInternal* rtcp_transport_channel_ =
nullptr;
On 2017/01/13 22:41:16, pthatcher1 wrote:
> Same here with the names.

Done.

[pthatcher1, 2017-01-17 20:16:48]

LGTM, with nits

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/media/sctp/sctptr...
File webrtc/media/sctp/sctptransport.h (right):

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransport.h:69: cricket::DtlsTransportInternal* channel);
Can SctpTransport take just a PacketTransportInterface?  I don't see why it
needs anything DTLS specific.

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/media/sctp/sctptr...
webrtc/media/sctp/sctptransport.h:142: DtlsTransportInternal*
transport_channel_;
This file could use some channel => transport renaming, but I'm OK with you
leaving a TODO and doing it in a separate CL.

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:234: bool
all_ice_transport_writable() const {
all_ice_transports_writable()

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:238: for (const auto& channel :
channels_) {
transport : transports_

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/p2p/base/icetrans...
File webrtc/p2p/base/icetransportinternal.h (right):

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/p2p/base/icetrans...
webrtc/p2p/base/icetransportinternal.h:29: typedef IceTransportInternal
IceTransportInternal2;
Please leave a comment explaining this, even if it's temporary.

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/p2p/base/mockicet...
File webrtc/p2p/base/mockicetransport.h (right):

https://codereview.webrtc.org/2606123002/diff/240001/webrtc/p2p/base/mockicet...
webrtc/p2p/base/mockicetransport.h:73: std::string transport_name_ = "";
Does this even do anything different than the default?

[Zhi Huang, 2017-01-18 07:05:08]

Patchset #10 (id:280001) has been deleted

[Zhi Huang, 2017-01-18 07:07:26]

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

[commit-bot: I haz the power, 2017-01-18 07:07:28]

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, 2017-01-18 08:11:44]

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

[commit-bot: I haz the power, 2017-01-18 08:11:45]

Dry run: This issue passed the CQ dry run.

[Zhi Huang, 2017-01-19 02:17:59]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2017-01-19 02:18:00]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2606123002/#ps300001 (title:
"Merge")

[commit-bot: I haz the power, 2017-01-19 02:18:07]

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

[Zhi Huang, 2017-01-19 02:18:42]

The CQ bit was unchecked by zhihuang@webrtc.org

[Zhi Huang, 2017-01-19 08:42:28]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2017-01-19 08:42:28]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2606123002/#ps320001 (title:
"Merge")

[commit-bot: I haz the power, 2017-01-19 08:42:31]

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, 2017-01-19 09:48:03]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1484815348534970,
"parent_rev": "04926b82641c426d764aa6e013e133db519129db", "commit_rev":
"5aed06c8d3e0bf69f06f54ee7113f7a3ac2399c7"}

[commit-bot: I haz the power, 2017-01-19 09:48:07]

Description was changed from

==========
make the DtlsTransportWrapper inherit form DtlsTransportInternal

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

to

==========
make the DtlsTransportWrapper inherit form DtlsTransportInternal

BUG=none

Review-Url: https://codereview.webrtc.org/2606123002
Cr-Commit-Position: refs/heads/master@{#16160}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5aed06c8d3e0bf69f06f54ee7...
==========

[commit-bot: I haz the power, 2017-01-19 09:48:10]

Committed patchset #11 (id:320001) as
https://chromium.googlesource.com/external/webrtc/+/5aed06c8d3e0bf69f06f54ee7...

[Zhi Huang, 2017-01-19 10:33:39]

A revert of this CL (patchset #11 id:320001) has been created in
https://codereview.webrtc.org/2639203004/ by zhihuang@webrtc.org.

The reason for reverting is: Failed the memory check.
May need to fix the memory leak..

[Zhi Huang, 2017-01-19 11:28:17]

Description was changed from

==========
make the DtlsTransportWrapper inherit form DtlsTransportInternal

BUG=none

Review-Url: https://codereview.webrtc.org/2606123002
Cr-Commit-Position: refs/heads/master@{#16160}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5aed06c8d3e0bf69f06f54ee7...
==========

to

==========
make the DtlsTransportWrapper inherit form DtlsTransportInternal

BUG=none

Review-Url: https://codereview.webrtc.org/2606123002
Cr-Commit-Position: refs/heads/master@{#16160}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5aed06c8d3e0bf69f06f54ee7...
==========

[Zhi Huang, 2017-01-19 11:35:45]

Description was changed from

==========
make the DtlsTransportWrapper inherit form DtlsTransportInternal

BUG=none

Review-Url: https://codereview.webrtc.org/2606123002
Cr-Commit-Position: refs/heads/master@{#16160}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5aed06c8d3e0bf69f06f54ee7...
==========

to

==========
Remove the TransportChannel and TransportChannelImpl from WebRTC
native code.

DtlsTransportChannelWrapper is renamed to be DtlsTransport which inherits from 
DtlsTransportInternal. There will be no concept of "channel" in p2p level. 
Both P2PTransportChannel and DtlsTransport don't depend on TransportChannel 
and TransportChannelImpl any more and they are removed in this CL.

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

[Zhi Huang, 2017-01-19 11:36:48]

Description was changed from

==========
Remove the TransportChannel and TransportChannelImpl from WebRTC
native code.

DtlsTransportChannelWrapper is renamed to be DtlsTransport which inherits from 
DtlsTransportInternal. There will be no concept of "channel" in p2p level. 
Both P2PTransportChannel and DtlsTransport don't depend on TransportChannel 
and TransportChannelImpl any more and they are removed in this CL.

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

to

==========
Remove the dependency of TransportChannel and TransportChannelImpl.

DtlsTransportChannelWrapper is renamed to be DtlsTransport which inherits from 
DtlsTransportInternal. There will be no concept of "channel" in p2p level. 
Both P2PTransportChannel and DtlsTransport don't depend on TransportChannel 
and TransportChannelImpl any more and they are removed in this CL.

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

[Zhi Huang, 2017-01-19 12:12:51]

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

[commit-bot: I haz the power, 2017-01-19 12:13:03]

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

[Zhi Huang, 2017-01-19 12:13:17]

The CQ bit was unchecked by zhihuang@webrtc.org

[Zhi Huang, 2017-01-19 12:58:33]

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

[commit-bot: I haz the power, 2017-01-19 12:58:37]

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, 2017-01-19 14:31:22]

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

[commit-bot: I haz the power, 2017-01-19 14:31:23]

Dry run: This issue passed the CQ dry run.

[Zhi Huang, 2017-01-20 00:47:16]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2017-01-20 00:47:17]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2606123002/#ps340001 (title:
"Fix the memory leak.")

[commit-bot: I haz the power, 2017-01-20 00:47:25]

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

[Zhi Huang, 2017-01-20 00:47:38]

The CQ bit was unchecked by zhihuang@webrtc.org

[Zhi Huang, 2017-01-20 00:51:03]

The CQ bit was checked by zhihuang@webrtc.org

[commit-bot: I haz the power, 2017-01-20 00:51:06]

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, 2017-01-20 00:54:26]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1484873463153270,
"parent_rev": "9d643e87afa754b899d97bc336a5604e7002ad2f", "commit_rev":
"b2cdd93fd6ab91d453314ac2f51a849542d023d9"}

[commit-bot: I haz the power, 2017-01-20 00:54:30]

Description was changed from

==========
Remove the dependency of TransportChannel and TransportChannelImpl.

DtlsTransportChannelWrapper is renamed to be DtlsTransport which inherits from 
DtlsTransportInternal. There will be no concept of "channel" in p2p level. 
Both P2PTransportChannel and DtlsTransport don't depend on TransportChannel 
and TransportChannelImpl any more and they are removed in this CL.

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

to

==========
Remove the dependency of TransportChannel and TransportChannelImpl.

DtlsTransportChannelWrapper is renamed to be DtlsTransport which inherits from 
DtlsTransportInternal. There will be no concept of "channel" in p2p level. 
Both P2PTransportChannel and DtlsTransport don't depend on TransportChannel 
and TransportChannelImpl any more and they are removed in this CL.

BUG=none

Review-Url: https://codereview.webrtc.org/2606123002
Cr-Commit-Position: refs/heads/master@{#16173}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b2cdd93fd6ab91d453314ac2f...
==========

[commit-bot: I haz the power, 2017-01-20 00:54:31]

Committed patchset #12 (id:340001) as
https://chromium.googlesource.com/external/webrtc/+/b2cdd93fd6ab91d453314ac2f...