Thinning out the Transport class.

Thinning out the Transport class.

Connecting TransportChannelImpls directly to the TransportController,
and removing redundant signal forwarding/state aggregating code from
Transport. This brings us closer to just getting rid of Transport
entirely.

R=pthatcher@webrtc.org

Committed: https://crrev.com/c4d3a5d44c25fb42c26393b6ddc0feadd52e5e2f
Cr-Commit-Position: refs/heads/master@{#10120}

[Taylor Brandstetter, 2015-09-29 17:58:45]

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

[Taylor Brandstetter, 2015-09-29 17:58:45]

Hi Peter; I decided to split out the "Transport thinning" part of my work to
improve the ICE connection state logic, since it's pretty straightforward and
shouldn't introduce any behavioral changes.

https://codereview.webrtc.org/1380563002/diff/1/webrtc/p2p/base/transport_uni...
File webrtc/p2p/base/transport_unittest.cc (left):

https://codereview.webrtc.org/1380563002/diff/1/webrtc/p2p/base/transport_uni...
webrtc/p2p/base/transport_unittest.cc:203: // This test verifies that the
Completed and Failed states can be reached.
Note that these test cases are covered in transportcontroller_unittest.cc, so
it's fine that these are being removed.

[pthatcher1, 2015-09-29 18:12:43]

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

https://codereview.webrtc.org/1380563002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:148: TransportChannelImpl* channel =
transport->CreateChannel(component);
Would it make sense to move the ref-counting logic from Transport to
TransportController?   This part of it would be more clear, I think, and it
would simplify Transport as well.

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

https://codereview.webrtc.org/1380563002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.h:159: // Handlers for signals from
Transport.
from TransportChannel.

[Taylor Brandstetter, 2015-09-29 23:21:02]

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

https://codereview.webrtc.org/1380563002/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:148: TransportChannelImpl* channel =
transport->CreateChannel(component);
On 2015/09/29 18:12:43, pthatcher1 wrote:
> Would it make sense to move the ref-counting logic from Transport to
> TransportController?   This part of it would be more clear, I think, and it
> would simplify Transport as well.

Done.

[pthatcher1, 2015-09-29 23:50:58]

lgtm, with style nits.

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport.cc
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.cc:167: TransportChannelImpl* impl;
Can you rename "impl" to "channel"?

Here and below.

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.cc:176: .first;
Is iterator even used in this function?  Can we just remove this line?

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:170: return it->get();
Might be easier to read the other way around:

if (it != channels_.end()) {
  // Channel already exists; increment reference count and return.
  it->AddRef();
  return it->get();
}

// Need to create a new channel.
...

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:187: if (it->ref() == 0) {
Same here:

if (it->ref() > 0) {
  return;
}

channels_.erase(it);

[Taylor Brandstetter, 2015-09-30 01:01:20]

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport.cc
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.cc:167: TransportChannelImpl* impl;
On 2015/09/29 23:50:58, pthatcher1 wrote:
> Can you rename "impl" to "channel"?
> 
> Here and below.

Done.

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.cc:176: .first;
On 2015/09/29 23:50:58, pthatcher1 wrote:
> Is iterator even used in this function?  Can we just remove this line?

Done.

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:170: return it->get();
On 2015/09/29 23:50:58, pthatcher1 wrote:
> Might be easier to read the other way around:
> 
> if (it != channels_.end()) {
>   // Channel already exists; increment reference count and return.
>   it->AddRef();
>   return it->get();
> }
> 
> // Need to create a new channel.
> ...

Done.

https://codereview.webrtc.org/1380563002/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.cc:187: if (it->ref() == 0) {
On 2015/09/29 23:50:58, pthatcher1 wrote:
> Same here:
> 
> if (it->ref() > 0) {
>   return;
> }
> 
> channels_.erase(it);

Done.

[pthatcher1, 2015-09-30 05:33:57]

lgtm

[Taylor Brandstetter, 2015-09-30 17:33:14]

Committed patchset #4 (id:60001) manually as
c4d3a5d44c25fb42c26393b6ddc0feadd52e5e2f (presubmit successful).

[commit-bot: I haz the power, 2015-09-30 17:33:16]

Patchset 4 (id:??) landed as
https://crrev.com/c4d3a5d44c25fb42c26393b6ddc0feadd52e5e2f
Cr-Commit-Position: refs/heads/master@{#10120}