TransportController refactoring

TransportController refactoring.

Getting rid of TransportProxy, and in its place adding a
TransportController class which will facilitate access to and manage
the lifetimes of Transports. These Transports will now be accessed
solely from the worker thread, simplifying their implementation.

This refactoring also pulls Transport-related code out of BaseSession.
Which means that BaseChannels will now rely on the TransportController
interface to create channels, rather than BaseSession.

This CL also adds some unit tests, and does some renaming.
For example, from "CandidateReady" to "CandidateGathered".

Committed: https://crrev.com/9af63f473e1d0d6c47a741a046c41642dfc1c178
Cr-Commit-Position: refs/heads/master@{#9993}

[Taylor Brandstetter, 2015-08-05 21:47:40]

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

[Taylor Brandstetter, 2015-08-05 21:47:41]

Only ~2000 lines, have fun. ;)

By the way, there's a comment in webrtcsession_unittest.cc about something being
deleted with your CL; the CL I'm referring to is
https://codereview.webrtc.org/1263663002/ , which I assume will be approved
before this one. When that happens I'll merge it and remove the TODO comment.

[pthatcher1, 2015-08-10 20:40:17]

I read through all the files *except* the big three (transportcontroller.h,
transportcontroller.cc, transportcontroller_unittest.cc).  I'll have to try that
in a second round.  Just too much for the first pass.

It's looking quite good so far.  Just small stuff so far.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector.cc:706: cert.accept())) {
Would it make more sense for GetIdentity and GetRemoteCertificate to be on the
TransportController?

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:702: bool
WebRtcSession::StartCandidatesAllocation() {
And while we're in here, we might as well call this StartGatheringCandidates.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:728: return
transport_controller()->GetSslRole(role);
We should probably still leave the TODO about the different transports possibly
having different roles.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:837: // Remove transport channels if
MediaContentDescription is rejected.
I think you mean Remove channels, not transport channels.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1478:
cricket::TransportController::ConnectionState state) {
It seems like this enum type should still be a TransportChannel ConnectionState,
with the understanding that the TransportController is aggregating them.  Why do
we have the enum on TransportController?

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1489:
PeerConnectionInterface::kIceConnectionDisconnected);
Should it not go to checking, or maybe disconnected then checking?

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1513: std::set<std::string> transport_names;
Could just be a vector instead of a set.

Actually, this pattern comes up so often in the code, can we just add a method
called channels() that returns all the non-null channels so that we can do
something like this?

for (BaseChannel* channel: channels()) {
  cricket::TransportStats stats;
  if (transport_controller()->GetStats(channel->transport_name(), &stats)) {
    ReportBestConnectionState(stats);
    ReportNegotiatedCiphers(stats);
  }
}

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1554: ProcessNewLocalCandidate(transport_name,
candidates);
Could we just put the ProcessNewLocalCandidate code right here?  Why have a
separate method?

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:245: rtc::SSLIdentity** identity);
Can you run "git cl format"?  The style here looks off.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:373: void
OnTransportControllerCandidatesAllocationStarted();
While we're in here, can we change this to a GatheringState with a
GatheringStateChanged, where the GatherState has "gathering" and "complete".  

gathering == allocation started
complete == allocation done

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:374: void OnTransportControllerCandidatesReady(
And can we rename this to CandidatesGathered?

"gather" is the word the standard uses for "allocted" or "ready".  We should
start using it, I think.

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
File talk/session/media/channel.cc (left):

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
talk/session/media/channel.cc:1298: media_engine_(media_engine),
Do we not need to set the media engine any more?

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
talk/session/media/channel.cc:262: }
This doesn't seem right.  If we set_transport_channel(transport_channel()), it
will decrement the ref count?  Then you could keep doing that until it explodes.

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
talk/session/media/channel.cc:280: SetReadyToSend(new_tc, new_tc->writable());
Can you put in a nice comment about why we need to set the socket options and
update the writeable and "ready to send" states when we get a new transport
channel?

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransport.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:27: class DtlsTransport : public Base {
Can you comment that everything in this class should be called on the worker
thread?

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransport.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransport.h:21: P2PTransport(const std::string& content_name,
Again, can you comment that everything should be called on the worker thread?

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransport.h:33: DISALLOW_COPY_AND_ASSIGN(P2PTransport);
Why is this private?

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:948: void P2PTransportChannel::Allocate()
{
We could probably call this Gather() now

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/rawtrans...
File webrtc/p2p/base/rawtransport.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/rawtrans...
webrtc/p2p/base/rawtransport.h:23: class RawTransport : public Transport {
FYI, my CL just deletes this.

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:131: kGatheringDone,
Done => Complete

Also, this enum should probably live outside the scope of Transport.  Just call
it cricket::IceGatheringState.

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:262: void AddRemoteCandidate(const Candidate&
candidate);
Why do we need both of these?

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/p2p.gyp
File webrtc/p2p/p2p.gyp (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/p2p.gyp#newco...
webrtc/p2p/p2p.gyp:76: 'base/transportcontroller.h',
Sadly, you'll need to prepare a chromium CL to update its .gyp and BUILD.gn
files as well because you'll need to submit this, break the FYI bots, and then
fix the FYI bots with your update to the .gyp and BUILD.gn files.

[Taylor Brandstetter, 2015-08-11 01:20:08]

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector.cc:706: cert.accept())) {
On 2015/08/10 20:40:16, pthatcher1 wrote:
> Would it make more sense for GetIdentity and GetRemoteCertificate to be on the
> TransportController?

They ARE methods on the TransportController. But the thing is, WebRtcSession
needs to map a content name to a transport name. Also, WebRtcSession doesn't
directly expose the TransportController interface to StatsCollector.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:702: bool
WebRtcSession::StartCandidatesAllocation() {
On 2015/08/10 20:40:16, pthatcher1 wrote:
> And while we're in here, we might as well call this StartGatheringCandidates.

Done.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:728: return
transport_controller()->GetSslRole(role);
On 2015/08/10 20:40:16, pthatcher1 wrote:
> We should probably still leave the TODO about the different transports
possibly
> having different roles.

I left it, it just moved to transportcontroller.cc.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:837: // Remove transport channels if
MediaContentDescription is rejected.
On 2015/08/10 20:40:16, pthatcher1 wrote:
> I think you mean Remove channels, not transport channels.

Done.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1478:
cricket::TransportController::ConnectionState state) {
On 2015/08/10 20:40:16, pthatcher1 wrote:
> It seems like this enum type should still be a TransportChannel
ConnectionState,
> with the understanding that the TransportController is aggregating them.  Why
do
> we have the enum on TransportController?

Because it has a slightly different meaning than the state in TransportChannel,
and has an additional value ("Connected"). However, I at least moved it to the
"cricket" scope in transport.h, along with the gathering state. Note that once
the "talk" and "webrtc" trees are combined, we can probably just get rid of
those enums and use the ones in PeerConnectionInterface.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1489:
PeerConnectionInterface::kIceConnectionDisconnected);
On 2015/08/10 20:40:16, pthatcher1 wrote:
> Should it not go to checking, or maybe disconnected then checking?

This is the behavior we had before so I kept it. And it seems to match the W3C
spec.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1513: std::set<std::string> transport_names;
On 2015/08/10 20:40:16, pthatcher1 wrote:
> Could just be a vector instead of a set.
> 
> Actually, this pattern comes up so often in the code, can we just add a method
> called channels() that returns all the non-null channels so that we can do
> something like this?
> 
> for (BaseChannel* channel: channels()) {
>   cricket::TransportStats stats;
>   if (transport_controller()->GetStats(channel->transport_name(), &stats)) {
>     ReportBestConnectionState(stats);
>     ReportNegotiatedCiphers(stats);
>   }
> }

If BUNDLE is enabled, I assumed we wouldn't want to call 'GetStats("audio",
&stats)' three times, hence the use of a set. Or DO we want that behavior?

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1554: ProcessNewLocalCandidate(transport_name,
candidates);
On 2015/08/10 20:40:16, pthatcher1 wrote:
> Could we just put the ProcessNewLocalCandidate code right here?  Why have a
> separate method?

Done.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:245: rtc::SSLIdentity** identity);
On 2015/08/10 20:40:16, pthatcher1 wrote:
> Can you run "git cl format"?  The style here looks off.

Done.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:373: void
OnTransportControllerCandidatesAllocationStarted();
On 2015/08/10 20:40:17, pthatcher1 wrote:
> While we're in here, can we change this to a GatheringState with a
> GatheringStateChanged, where the GatherState has "gathering" and "complete".  
> 
> gathering == allocation started
> complete == allocation done

Done.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:374: void OnTransportControllerCandidatesReady(
On 2015/08/10 20:40:16, pthatcher1 wrote:
> And can we rename this to CandidatesGathered?
> 
> "gather" is the word the standard uses for "allocted" or "ready".  We should
> start using it, I think.

Done.

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
File talk/session/media/channel.cc (left):

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
talk/session/media/channel.cc:1298: media_engine_(media_engine),
On 2015/08/10 20:40:17, pthatcher1 wrote:
> Do we not need to set the media engine any more?

Good catch... I must have deleted that line on accident when handling a merge
conflict.

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
talk/session/media/channel.cc:262: }
On 2015/08/10 20:40:17, pthatcher1 wrote:
> This doesn't seem right.  If we set_transport_channel(transport_channel()), it
> will decrement the ref count?  Then you could keep doing that until it
explodes.

I did this because "CreateTransportChannel_w" above will increment the ref
count. But probably a better solution would be to not call
CreateTransportChannel_w in the first place unless necessary. I added that logic
to SetTransportChannels_w.

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
talk/session/media/channel.cc:280: SetReadyToSend(new_tc, new_tc->writable());
On 2015/08/10 20:40:17, pthatcher1 wrote:
> Can you put in a nice comment about why we need to set the socket options and
> update the writeable and "ready to send" states when we get a new transport
> channel?

Done.

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransport.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:27: class DtlsTransport : public Base {
On 2015/08/10 20:40:17, pthatcher1 wrote:
> Can you comment that everything in this class should be called on the worker
> thread?

Done. Added comment to its base class (Transport) as well.

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransport.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransport.h:21: P2PTransport(const std::string& content_name,
On 2015/08/10 20:40:17, pthatcher1 wrote:
> Again, can you comment that everything should be called on the worker thread?

Done.

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransport.h:33: DISALLOW_COPY_AND_ASSIGN(P2PTransport);
On 2015/08/10 20:40:17, pthatcher1 wrote:
> Why is this private?

DISALLOW_COPY_AND_ASSIGN needs to be in the private section since its purpose is
to make the copy constructor and assignment operator private. "git cl lint"
caught this problem.

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:948: void P2PTransportChannel::Allocate()
{
On 2015/08/10 20:40:17, pthatcher1 wrote:
> We could probably call this Gather() now

Done (I'll call it StartGatheringCandidates for clarity/consistency)

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/rawtrans...
File webrtc/p2p/base/rawtransport.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/rawtrans...
webrtc/p2p/base/rawtransport.h:23: class RawTransport : public Transport {
On 2015/08/10 20:40:17, pthatcher1 wrote:
> FYI, my CL just deletes this.

Acknowledged.

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:131: kGatheringDone,
On 2015/08/10 20:40:17, pthatcher1 wrote:
> Done => Complete
> 
> Also, this enum should probably live outside the scope of Transport.  Just
call
> it cricket::IceGatheringState.

Done.

https://codereview.webrtc.org/1246913005/diff/340001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:262: void AddRemoteCandidate(const Candidate&
candidate);
On 2015/08/10 20:40:17, pthatcher1 wrote:
> Why do we need both of these?

I'm not sure; this was left over from your original CL. I'll remove it, it
appears unnecessary.

[pthatcher1, 2015-08-18 22:32:47]

Comments on your updates. 

I like the candidate gathering changes.

I still need to get through the TransportController itself.  I still haven't
made it.

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector.cc:706: cert.accept())) {
On 2015/08/11 01:20:06, Taylor Brandstetter wrote:
> On 2015/08/10 20:40:16, pthatcher1 wrote:
> > Would it make more sense for GetIdentity and GetRemoteCertificate to be on
the
> > TransportController?
> 
> They ARE methods on the TransportController. But the thing is, WebRtcSession
> needs to map a content name to a transport name. Also, WebRtcSession doesn't
> directly expose the TransportController interface to StatsCollector.

Ah, so the transport_iter.second.content_name is the content name, not the
transport name, and the proxy_to_transport_ (now misnamed) converts from
content_name to the transport_name.  

That all seems like a big mess, but it looks like you're retaining the existing
behavior in a clean way.  We'll have to shave the stats yak another time.

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/340001/talk/session/media/chann...
talk/session/media/channel.cc:262: }
On 2015/08/11 01:20:07, Taylor Brandstetter wrote:
> On 2015/08/10 20:40:17, pthatcher1 wrote:
> > This doesn't seem right.  If we set_transport_channel(transport_channel()),
it
> > will decrement the ref count?  Then you could keep doing that until it
> explodes.
> 
> I did this because "CreateTransportChannel_w" above will increment the ref
> count. But probably a better solution would be to not call
> CreateTransportChannel_w in the first place unless necessary. I added that
logic
> to SetTransportChannels_w.

I think it's right to decrement the pointer of the old_tc if old_tc != new_tc. 
But if they are the same and we return early, we shouldn't change the ref count.

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:799: StartGatheringCandidates();
Here's where that code can be inlined.  Just delete the comment of kicking off
allocation.  That happens inside of UpdateSessionState.

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:2014: ice_observer_->OnIceGatheringChange(
I think we need to check if(ice_observer_), in case it  is null.

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:285: // candidates allocation.
Hmmm... I'm not sure this is the way we want this to be.  The way it used to
work is that 

StartCandidatesAllocation would call
BaseSession::SpeculativelyConnectAllTransportChannels would call
Transport::ConnectChannels would call
P2PTransportChannel::Connect would call
P2PTransportChannel::Allocate

*AND*

WebRtcSession::StartCandidatesAllocation would call
WebRtcSession::UseCandidatesInSessionDescription would call
WebRtcSession::UseCandidate would call
BaseSession::OnRemoteCandidates would call
Transport::ConnectChannels would call
P2PTransportChannel::Connect would call
P2PTransportChannel::Allocate


But the second one would only happen if there were candidates in the session
description, and there might not be any.  And it's a really hacky way to start
allocation.  And there's a comment saying to remove the second code path.  

Meanwhile, BaseSession::SpeculativelyConnectAllTransportChannels is gone, so the
first part is gone too.


But there is a third way allocation starts: SetLocalDescription.  And that's
arguably the right way, and the other two shouldn't exist.  


So, long story short: I think you should just remove this method and inline the
code from it  into the once call site that exists.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:240:
set_transport_channel(transport_controller_->CreateTransportChannel_w(
Even though it's called "create", it's more like "create if necessary; if
already created, increment the ref count".  We need all of the BaseChannels to
increment the refcount.  Otherwise, it might get deleted while one of them is
still using it.

In other words, I think this if statement is wrong and we should always call
CreateTransportChannel.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:284: // Update aggregate writable/ready-to-send
state upon setting new channel
It's not really aggregate.  It's more like inherited.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:47: enum ConnectionState {
We should probably call this IceConnectionState, and we should leave a TODO to
unify it with the one PeerConnection uses.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:54: enum GatheringState {
We should probably call this IceGatheringState, and we should leave a TODO to
unify it with the one PeerConnection uses.

[pthatcher2, 2015-08-19 18:32:13]

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

[pthatcher2, 2015-08-19 18:32:16]

I finally got through the whole thing.  There's some naming/style stuff, and a
few little things.  But overall the logic looks good.

The big things is that we need a lot more unit tests.  Fortunately, I think they
should be pretty straight forward ones.   Just typing :).

Lastly, we'll want to do some good manual testing before submitting.  Trying
chrome/chrome, chrome/firefox, with and without bundle, testing an ICE restart,
maybe even forcing an ICE role conflict.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:50:
ASSERT(signaling_thread_->IsCurrent());
Is this necessary?  I don't think it is.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:54:
ASSERT(signaling_thread_->IsCurrent());
I don't think this is necessary either.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:61:
ASSERT(signaling_thread_->IsCurrent());
In fact, I don't think any of these checks to be on the signaling thread are
necessary.  These are all theoretically callable from any thread.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:102: bool
TransportController::SetLocalTransportDescription(
We could probably call this SetLocalDescription and SetRemoteDescription. 
"Transport" is redundant now.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:156: ASSERT(worker_thread_->IsCurrent());
These for the worker_thread_ *are* needed.  Only the signalling thread ones
aren't.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:168: ASSERT(transport != nullptr);
This will only protect debug builds.  For release builds, we probably still need

if (!transport) {
  return;
}

And you could combine the two:

if (!transport) {
  ASSERT(false);
  return;
}

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:176: UpdateGatheringState_w();
I think it would be more clear to put UpdateState_w and UpdateGatheringState_w
in DestroyTransport_w.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:205: case MSG_CANDIDATESREADY: {
MSG_CANDIDATESGATHERED

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:231: return transport;
{}s please

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:253:
&TransportController::OnTransportFailed_w);
Can you make this one consistent with the other with the line break?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:257:
transport->SetIdentity(identity_.get());
{}s please.  

Can can you move it up to before the signals so that the Set methods are all
together?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:258: transports_[transport_name] =
transport;
It seems like all of this stuff that happens after CreateTransport_w should just
be in CreateTransport_w.  But I just remembered that we do it like this because
unit tests can override CreateTransport_w.  Can you put a comment explaining
this so that we don't have to re-remember it every time we read this code?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:311: return false;
{} please

Also, we should leave a comment in the header that it can only be called once.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:369: return true;
That's weird.  Can you put a TODO to fix the callers so that they are smarter
than this?  The same for the other places like this.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:375: transport->ConnectChannels();
If we always ConnectChannels after SetLocalDescription is called, can we just
remove the ConnectChannels method and have Transport take care of it when we
call SetLocalDescription?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:421: transport->remote_description_set();
Can we move this logic into Transport (have a
Transport::ReadyForRemoteCandidates)?  It seems weird that the
TransportController needs to have access to local_description_set and
remote_description_set.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:433: }
We should probably just follow the same pattern we have with ice_role_:  Store
it in TransportController until we support different roles for different
transports.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:465: UpdateGatheringState_w();
It might be less efficient, but would it be more readable to just have one
UpdateAggregateStates that does all the states at once (including gathering
states)?  I could go either way.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:472: // This signal is not currently used
for anything
Then let's not listen to it.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:479: LOG(LS_WARNING) << "Repeat of role
conflic signal from Transport.";
conflic => conflict

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:500: }
Can you put these up next to others? 

(before OnTransportRoleConflict_w)

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:520: bool completed = true;
Can you call these the following?

any_failed
all_receiving
all_connected
all_completed

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:532: }
Could be more readable as:

bool all_completed = !transports_.empty();
bool all_connected = !transports_.empty();
bool all_receiving = !transports_.empty();
for (const auto& kv : transports_) {
  const Transport* transport = kv.second;
  all_completed &&= transport->Completed(); 
  all_connected &&= transport->all_channels_writable();
  all_receiving &&= transport->any_channel_receiving();
}

ConnectionState new_state = kConnectedConnecting;
if (all_completed) {
  new_state = kConnectionCompleted;
} else if (all_connected) {
  new_state = kConnectionConnected;
}



Or if you change from a map to a vector, it could be even better:

bool all_completed = !transports_.empty() && std::all_of(transports_.begin(),
transports_.end(), Transport::Completed);
bool all_connnected =!transports_.empty() && std::all_of(transports_.begin(),
transports_.end(), Transport::all_channels_writable);
bool all_receiving = !transports_.empty() && std::all_of(transports_.begin(),
transports_.end(), Transport::any_channel_receiving);

ConnectionState new_state = kConnectedConnecting;
if (all_completed) {
  new_state = kConnectionCompleted;
} else if (all_connected) {
  new_state = kConnectionConnected;
}

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:535: if (failed) {
Actually, wait.... failed can never happen.  So why do we even have it?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:547: this, MSG_CONNECTIONSTATE,
Since "connection" is a word we use for "candidate pair" inside of
P2pTransportChannel, this keeps making me misread it.  A bette r name might be
TransportState for the enum and MSG_TRANSPORTSTATE.  In fact, ORTC calls it
IceTransportState and DtlsTransportState, so that probably makes sense.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:571: if (kv.second->gathering_state() !=
kGatheringNew) {
Did you mean kGatheringGathering here?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:580: }
This could be more readable as:

bool any_gathering = false;
bool all_complete = !transport_.empty();
for (const auto& kv : transports_) {
  const Transport* transport = kv.second;
  all_complete &&= (transport->gathering_state() == kGatheringComplete);
  any_gathering ||= (transport->gathering_state() == kGatheringGathering);
}

GatheringState new_state = kGatheringNew;
if (all_complete) {
  new_state = kGatheringComplete;
} else if (any_gathering) {
  new_state = kGatheringGathering;
}


Or if you change from a map to a vector, it could be even better:


bool all_completed = !transport_.empty && std::all_of(transports_.begin(),
transports_.end(), 
  [](const Transport* t){ t->gathering_state() == kGatheringComplete; }
);
bool any_gathering = std::any_of(transports_.begin(), transports_.end(), 
  [](const Transport* t){ t->gathering_state() == kGatheringGathering; }
);
GatheringState new_state = kGatheringNew;
if (all_complete) {
  new_state = kGatheringComplete;
} else if (any_gathering) {
  new_state = kGatheringGathering;
}

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:46: bool SetIdentity(rtc::SSLIdentity*
identity);
Can you organize these into an order like so?

1.  Methods that involve TransportController itself (like signaling_thread,
worker_thread)
2.  Methods that involve all the Transports (like port_allocator,
SetSslMaxProtocolVersion)
3.  Methods that currently involve all the Transports but should probably
someday involve just one (like SetIceConnectionReceivingTimeout, SetIceRole,
SetIdentity, GetSslRole)
4.  Methods that affect a specific Transport (like GetIdentity,
GetRemoteCertificate, SetLocalDescription, SetRemoteDescription,
AddRemoteCandidates, ReadyForRemoteCandidates, GetStats)
5.  Methods that are worker-thread specific (CreateTransportChannel_w,
DestroyTransportChannel_w)

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:85: sigslot::signal1<GatheringState>
SignalGatheringState;
Can you comment on each of these about how they aggregate, and how they are
aggregated?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:102: void DestroyAllTransports_w();
Why are the above protected? Also for unit tests?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:127: bool GetStats_w(const std::string&
transport_name, TransportStats* stats);
Can you put these in the same order I suggested for the public ones?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:146: void UpdateState_w();
I'd call this UpdateAggregateStates_w() and just remove the comment.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:149: void UpdateGatheringState_w();
I'd call this UpdateAggregateGatheringState_w() and just remove the comment.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:157: int ice_receiving_timeout_ms_;
Rather than putting the default values in the constructor, you can put them
here, like so:

int ice_receiving_timeout_ms_ = -1;
IceRole ice_role_ = ICEROLE_CONTROLLING;


etc


It's one of the nice things added in C++11.

It might not work for rtc::CreateRandomId64(), though.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:158: IceRole ice_role_;
Can we do a similar re-ordering here with fields:

1.  Fields of the TransportController itself (threads, transports_)
2.  Fields that are just there to pass into transports (port_allocator_,
ssl_max_version_)
3.  Fields that are aggregate states (connection_state_, receiving_,
candidates_allocated_, gathering_state_)
4.  Fields that should someday be unique to Transports (ice_role_,
ice_role_switch_, ice_tiebreaker_), with a TODO to move them down in Transports.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:161: std::map<std::string, Transport*>
transports_;
Given that there are usually going to be 1-3 transports, I'd suggest making this
a vector rather than a map.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:170: bool candidates_allocated_;
This looks like it isn't used anywhere.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:89: }
I think we need to add a lot more tests.  Sorry, we're paying up for the fact
that this wasn't well tested before.

1.  Test that CreateTransportChannel/DeleteTransportChannel work correctly with
setting the correct values on the created transport (which is created on demand
and deleted on demand).

2.  Make sure that setting certain cross-Transport values works (ice_role,
ssl_role, identity, etc) before and after creating a transport on demand.

3.  Getting cross-Transport values works, such as ice_role, ssl_role).

4.  Getting per-Transport values works, such as stats.

5.  Setting per-Transport things works, such as SetLocal and
SetRemoteDescription.

6.  The state aggregation works properly.

7.  Candidates get signalled up properly.

8.  ICE role conflict reversing works properly.

9.  The thread hopping happens properly (that might be harder to test, not
sure).

[Taylor Brandstetter, 2015-08-25 01:04:06]

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:799: StartGatheringCandidates();
On 2015/08/18 22:32:47, pthatcher1 wrote:
> Here's where that code can be inlined.  Just delete the comment of kicking off
> allocation.  That happens inside of UpdateSessionState.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:2014: ice_observer_->OnIceGatheringChange(
On 2015/08/18 22:32:47, pthatcher1 wrote:
> I think we need to check if(ice_observer_), in case it  is null.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:285: // candidates allocation.
On 2015/08/18 22:32:47, pthatcher1 wrote:
> Hmmm... I'm not sure this is the way we want this to be.  The way it used to
> work is that 
> 
> StartCandidatesAllocation would call
> BaseSession::SpeculativelyConnectAllTransportChannels would call
> Transport::ConnectChannels would call
> P2PTransportChannel::Connect would call
> P2PTransportChannel::Allocate
> 
> *AND*
> 
> WebRtcSession::StartCandidatesAllocation would call
> WebRtcSession::UseCandidatesInSessionDescription would call
> WebRtcSession::UseCandidate would call
> BaseSession::OnRemoteCandidates would call
> Transport::ConnectChannels would call
> P2PTransportChannel::Connect would call
> P2PTransportChannel::Allocate
> 
> 
> But the second one would only happen if there were candidates in the session
> description, and there might not be any.  And it's a really hacky way to start
> allocation.  And there's a comment saying to remove the second code path.  
> 
> Meanwhile, BaseSession::SpeculativelyConnectAllTransportChannels is gone, so
the
> first part is gone too.
> 
> 
> But there is a third way allocation starts: SetLocalDescription.  And that's
> arguably the right way, and the other two shouldn't exist.  
> 
> 
> So, long story short: I think you should just remove this method and inline
the
> code from it  into the once call site that exists.  

Done.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:240:
set_transport_channel(transport_controller_->CreateTransportChannel_w(
On 2015/08/18 22:32:47, pthatcher1 wrote:
> Even though it's called "create", it's more like "create if necessary; if
> already created, increment the ref count".  We need all of the BaseChannels to
> increment the refcount.  Otherwise, it might get deleted while one of them is
> still using it.
> 
> In other words, I think this if statement is wrong and we should always call
> CreateTransportChannel.

This just avoids incrementing the ref count pointlessly. If transport_name ==
transport_name_, we can be sure that we already HAVE a transport channel
identical to the one about to be given to us, and we ALREADY added to its ref
count. Imagine if you called:

SetTransportChannels("audio", true);
SetTransportChannels("audio", false);

On the first call we would create the RTP and RTCP transport channels. On the
second, we'd destroy the RTCP channel. But there'd be no need to add a ref count
to the RTP channel since it's not changing. Or at least, that's how I understood
it.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:284: // Update aggregate writable/ready-to-send
state upon setting new channel
On 2015/08/18 22:32:47, pthatcher1 wrote:
> It's not really aggregate.  It's more like inherited.

Well we have to aggregate the writable state from RTP and RTCP channels (at
least until RTCP-mux is required).

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:47: enum ConnectionState {
On 2015/08/18 22:32:47, pthatcher1 wrote:
> We should probably call this IceConnectionState, and we should leave a TODO to
> unify it with the one PeerConnection uses.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:54: enum GatheringState {
On 2015/08/18 22:32:47, pthatcher1 wrote:
> We should probably call this IceGatheringState, and we should leave a TODO to
> unify it with the one PeerConnection uses.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:50:
ASSERT(signaling_thread_->IsCurrent());
On 2015/08/19 18:32:15, pthatcher2 wrote:
> Is this necessary?  I don't think it is.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:54:
ASSERT(signaling_thread_->IsCurrent());
On 2015/08/19 18:32:15, pthatcher2 wrote:
> I don't think this is necessary either.  

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:61:
ASSERT(signaling_thread_->IsCurrent());
On 2015/08/19 18:32:15, pthatcher2 wrote:
> In fact, I don't think any of these checks to be on the signaling thread are
> necessary.  These are all theoretically callable from any thread.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:102: bool
TransportController::SetLocalTransportDescription(
On 2015/08/19 18:32:15, pthatcher2 wrote:
> We could probably call this SetLocalDescription and SetRemoteDescription. 
> "Transport" is redundant now.

We discussed this and decided we'll just leave it as it is until we replace
TransportDescription with a "parameters" class.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:156: ASSERT(worker_thread_->IsCurrent());
On 2015/08/19 18:32:15, pthatcher2 wrote:
> These for the worker_thread_ *are* needed.  Only the signalling thread ones
> aren't.

Acknowledged.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:168: ASSERT(transport != nullptr);
On 2015/08/19 18:32:15, pthatcher2 wrote:
> This will only protect debug builds.  For release builds, we probably still
need
> 
> if (!transport) {
>   return;
> }
> 
> And you could combine the two:
> 
> if (!transport) {
>   ASSERT(false);
>   return;
> }

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:176: UpdateGatheringState_w();
On 2015/08/19 18:32:15, pthatcher2 wrote:
> I think it would be more clear to put UpdateState_w and UpdateGatheringState_w
> in DestroyTransport_w.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:205: case MSG_CANDIDATESREADY: {
On 2015/08/19 18:32:15, pthatcher2 wrote:
> MSG_CANDIDATESGATHERED

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:231: return transport;
On 2015/08/19 18:32:15, pthatcher2 wrote:
> {}s please

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:253:
&TransportController::OnTransportFailed_w);
On 2015/08/19 18:32:15, pthatcher2 wrote:
> Can you make this one consistent with the other with the line break?

This is just what "git cl format" does, since "SignalFailed" is the shortest
signal name. What do you suggest?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:257:
transport->SetIdentity(identity_.get());
On 2015/08/19 18:32:15, pthatcher2 wrote:
> {}s please.  
> 
> Can can you move it up to before the signals so that the Set methods are all
> together?

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:258: transports_[transport_name] =
transport;
On 2015/08/19 18:32:15, pthatcher2 wrote:
> It seems like all of this stuff that happens after CreateTransport_w should
just
> be in CreateTransport_w.  But I just remembered that we do it like this
because
> unit tests can override CreateTransport_w.  Can you put a comment explaining
> this so that we don't have to re-remember it every time we read this code?

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:311: return false;
On 2015/08/19 18:32:15, pthatcher2 wrote:
> {} please
> 
> Also, we should leave a comment in the header that it can only be called once.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:369: return true;
On 2015/08/19 18:32:14, pthatcher2 wrote:
> That's weird.  Can you put a TODO to fix the callers so that they are smarter
> than this?  The same for the other places like this.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:375: transport->ConnectChannels();
On 2015/08/19 18:32:15, pthatcher2 wrote:
> If we always ConnectChannels after SetLocalDescription is called, can we just
> remove the ConnectChannels method and have Transport take care of it when we
> call SetLocalDescription?

Except ConnectChannels is still used for unit tests (I don't like it, but until
we refactor that's the way it will be)... I'll at least move the call to
Transport, as opposed to TransportController.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:421: transport->remote_description_set();
On 2015/08/19 18:32:15, pthatcher2 wrote:
> Can we move this logic into Transport (have a
> Transport::ReadyForRemoteCandidates)?  It seems weird that the
> TransportController needs to have access to local_description_set and
> remote_description_set.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:433: }
On 2015/08/19 18:32:15, pthatcher2 wrote:
> We should probably just follow the same pattern we have with ice_role_:  Store
> it in TransportController until we support different roles for different
> transports.

I think we already do support different roles for different transports though.
It's determined by a property of each individual transport description. Which
means this TODO comment is valid.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:465: UpdateGatheringState_w();
On 2015/08/19 18:32:15, pthatcher2 wrote:
> It might be less efficient, but would it be more readable to just have one
> UpdateAggregateStates that does all the states at once (including gathering
> states)?  I could go either way.

I generally prefer code readability/maintainability over efficiency, so sure.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:472: // This signal is not currently used
for anything
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Then let's not listen to it.  

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:479: LOG(LS_WARNING) << "Repeat of role
conflic signal from Transport.";
On 2015/08/19 18:32:15, pthatcher2 wrote:
> conflic => conflict

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:500: }
On 2015/08/19 18:32:15, pthatcher2 wrote:
> Can you put these up next to others? 
> 
> (before OnTransportRoleConflict_w)

Done. Also moved a couple other handlers so they're in the same order as signals
 are declared.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:520: bool completed = true;
On 2015/08/19 18:32:15, pthatcher2 wrote:
> Can you call these the following?
> 
> any_failed
> all_receiving
> all_connected
> all_completed

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:532: }
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Could be more readable as:
> 
> bool all_completed = !transports_.empty();
> bool all_connected = !transports_.empty();
> bool all_receiving = !transports_.empty();
> for (const auto& kv : transports_) {
>   const Transport* transport = kv.second;
>   all_completed &&= transport->Completed(); 
>   all_connected &&= transport->all_channels_writable();
>   all_receiving &&= transport->any_channel_receiving();
> }
> 
> ConnectionState new_state = kConnectedConnecting;
> if (all_completed) {
>   new_state = kConnectionCompleted;
> } else if (all_connected) {
>   new_state = kConnectionConnected;
> }
> 
> 
> 
> Or if you change from a map to a vector, it could be even better:
> 
> bool all_completed = !transports_.empty() && std::all_of(transports_.begin(),
> transports_.end(), Transport::Completed);
> bool all_connnected =!transports_.empty() && std::all_of(transports_.begin(),
> transports_.end(), Transport::all_channels_writable);
> bool all_receiving = !transports_.empty() && std::all_of(transports_.begin(),
> transports_.end(), Transport::any_channel_receiving);
> 
> ConnectionState new_state = kConnectedConnecting;
> if (all_completed) {
>   new_state = kConnectionCompleted;
> } else if (all_connected) {
>   new_state = kConnectionConnected;
> }
> 
> 

I actually prefer the former since there's less boilerplate code. I'll go with
that.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:535: if (failed) {
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Actually, wait.... failed can never happen.  So why do we even have it?

That was unintentional... I wonder how the unit tests passed? I guess I need to
add a test for that. :)

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:547: this, MSG_CONNECTIONSTATE,
On 2015/08/19 18:32:15, pthatcher2 wrote:
> Since "connection" is a word we use for "candidate pair" inside of
> P2pTransportChannel, this keeps making me misread it.  A bette r name might be
> TransportState for the enum and MSG_TRANSPORTSTATE.  In fact, ORTC calls it
> IceTransportState and DtlsTransportState, so that probably makes sense.

But weren't we going to call the enum "IceConnectionState" and later merge it
with PeerConnectionInterface::IceConnectionState? I'll call this
"MSG_ICECONNECTIONSTATE" for the time being, but let's talk about this later.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:571: if (kv.second->gathering_state() !=
kGatheringNew) {
On 2015/08/19 18:32:15, pthatcher2 wrote:
> Did you mean kGatheringGathering here?

No; consider a case where one transport is done gathering, and another hasn't
started. The aggregate state should be "gathering", not "new". We're gathering
if any transport is in any state EXCEPT for "new".

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:580: }
On 2015/08/19 18:32:15, pthatcher2 wrote:
> This could be more readable as:
> 
> bool any_gathering = false;
> bool all_complete = !transport_.empty();
> for (const auto& kv : transports_) {
>   const Transport* transport = kv.second;
>   all_complete &&= (transport->gathering_state() == kGatheringComplete);
>   any_gathering ||= (transport->gathering_state() == kGatheringGathering);
> }
> 
> GatheringState new_state = kGatheringNew;
> if (all_complete) {
>   new_state = kGatheringComplete;
> } else if (any_gathering) {
>   new_state = kGatheringGathering;
> }
> 
> 
> Or if you change from a map to a vector, it could be even better:
> 
> 
> bool all_completed = !transport_.empty && std::all_of(transports_.begin(),
> transports_.end(), 
>   [](const Transport* t){ t->gathering_state() == kGatheringComplete; }
> );
> bool any_gathering = std::any_of(transports_.begin(), transports_.end(), 
>   [](const Transport* t){ t->gathering_state() == kGatheringGathering; }
> );
> GatheringState new_state = kGatheringNew;
> if (all_complete) {
>   new_state = kGatheringComplete;
> } else if (any_gathering) {
>   new_state = kGatheringGathering;
> }

See earlier comment about UpdateState_w

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:46: bool SetIdentity(rtc::SSLIdentity*
identity);
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Can you organize these into an order like so?
> 
> 1.  Methods that involve TransportController itself (like signaling_thread,
> worker_thread)
> 2.  Methods that involve all the Transports (like port_allocator,
> SetSslMaxProtocolVersion)
> 3.  Methods that currently involve all the Transports but should probably
> someday involve just one (like SetIceConnectionReceivingTimeout, SetIceRole,
> SetIdentity, GetSslRole)
> 4.  Methods that affect a specific Transport (like GetIdentity,
> GetRemoteCertificate, SetLocalDescription, SetRemoteDescription,
> AddRemoteCandidates, ReadyForRemoteCandidates, GetStats)
> 5.  Methods that are worker-thread specific (CreateTransportChannel_w,
> DestroyTransportChannel_w)

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:85: sigslot::signal1<GatheringState>
SignalGatheringState;
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Can you comment on each of these about how they aggregate, and how they are
> aggregated?

Can I just say "refer to the W3C draft"? :)

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:102: void DestroyAllTransports_w();
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Why are the above protected? Also for unit tests?

I don't remember... I'm making them private again though.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:127: bool GetStats_w(const std::string&
transport_name, TransportStats* stats);
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Can you put these in the same order I suggested for the public ones?

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:146: void UpdateState_w();
On 2015/08/19 18:32:16, pthatcher2 wrote:
> I'd call this UpdateAggregateStates_w() and just remove the comment.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:149: void UpdateGatheringState_w();
On 2015/08/19 18:32:16, pthatcher2 wrote:
> I'd call this UpdateAggregateGatheringState_w() and just remove the comment.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:157: int ice_receiving_timeout_ms_;
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Rather than putting the default values in the constructor, you can put them
> here, like so:
> 
> int ice_receiving_timeout_ms_ = -1;
> IceRole ice_role_ = ICEROLE_CONTROLLING;
> 
> 
> etc
> 
> 
> It's one of the nice things added in C++11.
> 
> It might not work for rtc::CreateRandomId64(), though.

Ah; I'm still not used to most the C++11 features. Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:158: IceRole ice_role_;
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Can we do a similar re-ordering here with fields:
> 
> 1.  Fields of the TransportController itself (threads, transports_)
> 2.  Fields that are just there to pass into transports (port_allocator_,
> ssl_max_version_)
> 3.  Fields that are aggregate states (connection_state_, receiving_,
> candidates_allocated_, gathering_state_)
> 4.  Fields that should someday be unique to Transports (ice_role_,
> ice_role_switch_, ice_tiebreaker_), with a TODO to move them down in
Transports.

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:161: std::map<std::string, Transport*>
transports_;
On 2015/08/19 18:32:16, pthatcher2 wrote:
> Given that there are usually going to be 1-3 transports, I'd suggest making
this
> a vector rather than a map.

But given that the most common operation is looking up a transport by name,
isn't a map the most convenient structure?

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:170: bool candidates_allocated_;
On 2015/08/19 18:32:16, pthatcher2 wrote:
> This looks like it isn't used anywhere.

Yep, that got replaced by gathering_state_. Removed.

[pthatcher1, 2015-08-25 18:40:39]

I like how this is coming together.  It feels like there are just a few minor
things and some unit tests needed.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:240:
set_transport_channel(transport_controller_->CreateTransportChannel_w(
On 2015/08/25 01:04:05, Taylor Brandstetter wrote:
> On 2015/08/18 22:32:47, pthatcher1 wrote:
> > Even though it's called "create", it's more like "create if necessary; if
> > already created, increment the ref count".  We need all of the BaseChannels
to
> > increment the refcount.  Otherwise, it might get deleted while one of them
is
> > still using it.
> > 
> > In other words, I think this if statement is wrong and we should always call
> > CreateTransportChannel.
> 
> This just avoids incrementing the ref count pointlessly. If transport_name ==
> transport_name_, we can be sure that we already HAVE a transport channel
> identical to the one about to be given to us, and we ALREADY added to its ref
> count. Imagine if you called:
> 
> SetTransportChannels("audio", true);
> SetTransportChannels("audio", false);
> 
> On the first call we would create the RTP and RTCP transport channels. On the
> second, we'd destroy the RTCP channel. But there'd be no need to add a ref
count
> to the RTP channel since it's not changing. Or at least, that's how I
understood
> it.

If you call:

SetTransportChannels("audio", true);
SetTransportChannels("audio", false);

Assuming we are not bundling and not using RTCP-mux, then two separate transport
channels will be created, and each will need their ref count incremented.  It's
the transport channel getting incremented, not the transport.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:284: // Update aggregate writable/ready-to-send
state upon setting new channel
On 2015/08/25 01:04:05, Taylor Brandstetter wrote:
> On 2015/08/18 22:32:47, pthatcher1 wrote:
> > It's not really aggregate.  It's more like inherited.
> 
> Well we have to aggregate the writable state from RTP and RTCP channels (at
> least until RTCP-mux is required).

That's a good point.  Maybe mention that in the comment, so we can update it
when we remove RTCP-non-mux (just say "aggregate writable/ready-to-send state
between RTP and RTCP").

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:253:
&TransportController::OnTransportFailed_w);
On 2015/08/25 01:04:05, Taylor Brandstetter wrote:
> On 2015/08/19 18:32:15, pthatcher2 wrote:
> > Can you make this one consistent with the other with the line break?
> 
> This is just what "git cl format" does, since "SignalFailed" is the shortest
> signal name. What do you suggest?

Just go with "git cl format".

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:85: sigslot::signal1<GatheringState>
SignalGatheringState;
On 2015/08/25 01:04:06, Taylor Brandstetter wrote:
> On 2015/08/19 18:32:16, pthatcher2 wrote:
> > Can you comment on each of these about how they aggregate, and how they are
> > aggregated?
> 
> Can I just say "refer to the W3C draft"? :)

That's not very useful for someone trying to read the code :).


Just something like:

"If all are connected => connected, else ..."
"If all are done gathering => done gathering, else ..."

Etc.  Hopefully it won't be too long.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:161: std::map<std::string, Transport*>
transports_;
On 2015/08/25 01:04:06, Taylor Brandstetter wrote:
> On 2015/08/19 18:32:16, pthatcher2 wrote:
> > Given that there are usually going to be 1-3 transports, I'd suggest making
> this
> > a vector rather than a map.
> 
> But given that the most common operation is looking up a transport by name,
> isn't a map the most convenient structure?

If you have a GetTransport(name) method, it doesn't really matter how often you
call it.  The convenience is the same.  

But if it's easier to keep the code as is to land it, feel free to keep it a
map.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:279: if (connections_.size() == 0) {
Could you change this to .empty() while you're here?

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:239: bool had_connection_ = false;
Can you put a short comment explaining something like "if id ever had
!connections_.empty()"?  Otherwise, it could be ambiguous with best_connection_.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:206: ConnectChannels();
Can you comment that this kicks off allocation?

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:47: // TODO(deadbeef): unify with
PeerConnectionInterface::IceConnectionState
unify => Unify

Here and below

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:54: kIceConnectionCompleted,
I found out last week that the right style for enums is now
ICE_CONNECTION_CONNECTING, etc, instead of kIceConnectionConnecting.

So we can choose to change this style now or put in the TODO to do so when we
unify with PeerConnectionInterface::IceConnectionState.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.h (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:41: STATE_NEW,
We usually use the name STATE_INIT for states like this (the external API names
of "new" is a little different).

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:42: STATE_CONNECTING,
Can you comment here when we'd be NEW but not CONNECTING?  Basically, before
SetLocalDescription is called?

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:515: }
A good way to write that comment I was talking about would be to turn this into
English.

[Taylor Brandstetter, 2015-08-25 20:39:55]

Added more comments where necessary. Also merged with your GICE removal.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:240:
set_transport_channel(transport_controller_->CreateTransportChannel_w(
On 2015/08/25 18:40:38, pthatcher1 wrote:
> On 2015/08/25 01:04:05, Taylor Brandstetter wrote:
> > On 2015/08/18 22:32:47, pthatcher1 wrote:
> > > Even though it's called "create", it's more like "create if necessary; if
> > > already created, increment the ref count".  We need all of the
BaseChannels
> to
> > > increment the refcount.  Otherwise, it might get deleted while one of them
> is
> > > still using it.
> > > 
> > > In other words, I think this if statement is wrong and we should always
call
> > > CreateTransportChannel.
> > 
> > This just avoids incrementing the ref count pointlessly. If transport_name
==
> > transport_name_, we can be sure that we already HAVE a transport channel
> > identical to the one about to be given to us, and we ALREADY added to its
ref
> > count. Imagine if you called:
> > 
> > SetTransportChannels("audio", true);
> > SetTransportChannels("audio", false);
> > 
> > On the first call we would create the RTP and RTCP transport channels. On
the
> > second, we'd destroy the RTCP channel. But there'd be no need to add a ref
> count
> > to the RTP channel since it's not changing. Or at least, that's how I
> understood
> > it.
> 
> If you call:
> 
> SetTransportChannels("audio", true);
> SetTransportChannels("audio", false);
> 
> Assuming we are not bundling and not using RTCP-mux, then two separate
transport
> channels will be created, and each will need their ref count incremented. 
It's
> the transport channel getting incremented, not the transport.  
> 

*Initially* we'll add a ref count to each channel, yes. But on the second call
('SetTransportChannels("audio", false);'), there's no need to add a ref count to
the RTP channel, since we already have a reference to it, and already
incremented its ref count. If we did that, then when destroying the BaseChannel,
the RTP channel would be left with a ref count of 1, and wouldn't be destroyed
even though nothing is referencing it.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:284: // Update aggregate writable/ready-to-send
state upon setting new channel
On 2015/08/25 18:40:38, pthatcher1 wrote:
> On 2015/08/25 01:04:05, Taylor Brandstetter wrote:
> > On 2015/08/18 22:32:47, pthatcher1 wrote:
> > > It's not really aggregate.  It's more like inherited.
> > 
> > Well we have to aggregate the writable state from RTP and RTCP channels (at
> > least until RTCP-mux is required).
> 
> That's a good point.  Maybe mention that in the comment, so we can update it
> when we remove RTCP-non-mux (just say "aggregate writable/ready-to-send state
> between RTP and RTCP").

Done.

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:85: sigslot::signal1<GatheringState>
SignalGatheringState;
On 2015/08/25 18:40:38, pthatcher1 wrote:
> On 2015/08/25 01:04:06, Taylor Brandstetter wrote:
> > On 2015/08/19 18:32:16, pthatcher2 wrote:
> > > Can you comment on each of these about how they aggregate, and how they
are
> > > aggregated?
> > 
> > Can I just say "refer to the W3C draft"? :)
> 
> That's not very useful for someone trying to read the code :).
> 
> 
> Just something like:
> 
> "If all are connected => connected, else ..."
> "If all are done gathering => done gathering, else ..."
> 
> Etc.  Hopefully it won't be too long.

Done.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:279: if (connections_.size() == 0) {
On 2015/08/25 18:40:39, pthatcher1 wrote:
> Could you change this to .empty() while you're here?

Done.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:239: bool had_connection_ = false;
On 2015/08/25 18:40:39, pthatcher1 wrote:
> Can you put a short comment explaining something like "if id ever had
> !connections_.empty()"?  Otherwise, it could be ambiguous with
best_connection_.

Done.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:206: ConnectChannels();
On 2015/08/25 18:40:39, pthatcher1 wrote:
> Can you comment that this kicks off allocation?

Done.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:47: // TODO(deadbeef): unify with
PeerConnectionInterface::IceConnectionState
On 2015/08/25 18:40:39, pthatcher1 wrote:
> unify => Unify
> 
> Here and below

Done.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:54: kIceConnectionCompleted,
On 2015/08/25 18:40:39, pthatcher1 wrote:
> I found out last week that the right style for enums is now
> ICE_CONNECTION_CONNECTING, etc, instead of kIceConnectionConnecting.
> 
> So we can choose to change this style now or put in the TODO to do so when we
> unify with PeerConnectionInterface::IceConnectionState.
> 

Done.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.h (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:41: STATE_NEW,
On 2015/08/25 18:40:39, pthatcher1 wrote:
> We usually use the name STATE_INIT for states like this (the external API
names
> of "new" is a little different).

Done.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:42: STATE_CONNECTING,
On 2015/08/25 18:40:39, pthatcher1 wrote:
> Can you comment here when we'd be NEW but not CONNECTING?  Basically, before
> SetLocalDescription is called?

Done.

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1246913005/diff/470001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:515: }
On 2015/08/25 18:40:39, pthatcher1 wrote:
> A good way to write that comment I was talking about would be to turn this
into
> English.  

Acknowledged.

[pthatcher1, 2015-08-25 21:19:49]

OK, I think the code looks good.  It's just down to unit tests now.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:240:
set_transport_channel(transport_controller_->CreateTransportChannel_w(
On 2015/08/25 20:39:54, Taylor Brandstetter wrote:
> On 2015/08/25 18:40:38, pthatcher1 wrote:
> > On 2015/08/25 01:04:05, Taylor Brandstetter wrote:
> > > On 2015/08/18 22:32:47, pthatcher1 wrote:
> > > > Even though it's called "create", it's more like "create if necessary;
if
> > > > already created, increment the ref count".  We need all of the
> BaseChannels
> > to
> > > > increment the refcount.  Otherwise, it might get deleted while one of
them
> > is
> > > > still using it.
> > > > 
> > > > In other words, I think this if statement is wrong and we should always
> call
> > > > CreateTransportChannel.
> > > 
> > > This just avoids incrementing the ref count pointlessly. If transport_name
> ==
> > > transport_name_, we can be sure that we already HAVE a transport channel
> > > identical to the one about to be given to us, and we ALREADY added to its
> ref
> > > count. Imagine if you called:
> > > 
> > > SetTransportChannels("audio", true);
> > > SetTransportChannels("audio", false);
> > > 
> > > On the first call we would create the RTP and RTCP transport channels. On
> the
> > > second, we'd destroy the RTCP channel. But there'd be no need to add a ref
> > count
> > > to the RTP channel since it's not changing. Or at least, that's how I
> > understood
> > > it.
> > 
> > If you call:
> > 
> > SetTransportChannels("audio", true);
> > SetTransportChannels("audio", false);
> > 
> > Assuming we are not bundling and not using RTCP-mux, then two separate
> transport
> > channels will be created, and each will need their ref count incremented. 
> It's
> > the transport channel getting incremented, not the transport.  
> > 
> 
> *Initially* we'll add a ref count to each channel, yes. But on the second call
> ('SetTransportChannels("audio", false);'), there's no need to add a ref count
to
> the RTP channel, since we already have a reference to it, and already
> incremented its ref count. If we did that, then when destroying the
BaseChannel,
> the RTP channel would be left with a ref count of 1, and wouldn't be destroyed
> even though nothing is referencing it.

OK, I think I get what you're saying now.  I misread your example as something
else the last time.  And I must have misread the code the first time through,
because it makes sense to me this time.

What you're saying is if we basically enable bundle twice (with the same
transport name), we need to make sure we don't increment the refcount twice.  

I think we could write that in a slightly more readable way (that a slow code
reader like myself can handle) with an early return.  How about something like
this?


if (!rtcp_transport_channel_ && rtcp) {
  LOG(LS_WARNING) << "Ignoring request for RTCP transport channel because we
don't support disabling RTCP-mux once enabled."
  return false;
}

if (transport_name == transport_name_) {
  // Nothing to do
  return true;
}

set_transport_channel(transport_controller_->CreateTransportChannel_w(
  transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP));
if (!transport_channel()) {
  return false;
}
if (rtcp) {
  set_rtcp_transport_channel(transport_controller_->CreateTransportChannel_w(
    transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP));
  if (!rtcp_transport_channel()) {
    return false;
  }
} else {
  set_rtcp_transport_channel(nullptr);
}

transport_name_ = transport_name;
return true;

[Taylor Brandstetter, 2015-08-27 22:15:56]

deadbeef@webrtc.org changed reviewers:
+ juberti@webrtc.org
- pthatcher@chromium.org

[Taylor Brandstetter, 2015-08-27 22:15:57]

Finished adding unit tests. Also cleaned some minor stuff up and did another
merge.

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/talk/session/media/chann...
talk/session/media/channel.cc:240:
set_transport_channel(transport_controller_->CreateTransportChannel_w(
On 2015/08/25 21:19:49, pthatcher1 wrote:
> On 2015/08/25 20:39:54, Taylor Brandstetter wrote:
> > On 2015/08/25 18:40:38, pthatcher1 wrote:
> > > On 2015/08/25 01:04:05, Taylor Brandstetter wrote:
> > > > On 2015/08/18 22:32:47, pthatcher1 wrote:
> > > > > Even though it's called "create", it's more like "create if necessary;
> if
> > > > > already created, increment the ref count".  We need all of the
> > BaseChannels
> > > to
> > > > > increment the refcount.  Otherwise, it might get deleted while one of
> them
> > > is
> > > > > still using it.
> > > > > 
> > > > > In other words, I think this if statement is wrong and we should
always
> > call
> > > > > CreateTransportChannel.
> > > > 
> > > > This just avoids incrementing the ref count pointlessly. If
transport_name
> > ==
> > > > transport_name_, we can be sure that we already HAVE a transport channel
> > > > identical to the one about to be given to us, and we ALREADY added to
its
> > ref
> > > > count. Imagine if you called:
> > > > 
> > > > SetTransportChannels("audio", true);
> > > > SetTransportChannels("audio", false);
> > > > 
> > > > On the first call we would create the RTP and RTCP transport channels.
On
> > the
> > > > second, we'd destroy the RTCP channel. But there'd be no need to add a
ref
> > > count
> > > > to the RTP channel since it's not changing. Or at least, that's how I
> > > understood
> > > > it.
> > > 
> > > If you call:
> > > 
> > > SetTransportChannels("audio", true);
> > > SetTransportChannels("audio", false);
> > > 
> > > Assuming we are not bundling and not using RTCP-mux, then two separate
> > transport
> > > channels will be created, and each will need their ref count incremented. 
> > It's
> > > the transport channel getting incremented, not the transport.  
> > > 
> > 
> > *Initially* we'll add a ref count to each channel, yes. But on the second
call
> > ('SetTransportChannels("audio", false);'), there's no need to add a ref
count
> to
> > the RTP channel, since we already have a reference to it, and already
> > incremented its ref count. If we did that, then when destroying the
> BaseChannel,
> > the RTP channel would be left with a ref count of 1, and wouldn't be
destroyed
> > even though nothing is referencing it.
> 
> OK, I think I get what you're saying now.  I misread your example as something
> else the last time.  And I must have misread the code the first time through,
> because it makes sense to me this time.
> 
> What you're saying is if we basically enable bundle twice (with the same
> transport name), we need to make sure we don't increment the refcount twice.  
> 
> I think we could write that in a slightly more readable way (that a slow code
> reader like myself can handle) with an early return.  How about something like
> this?
> 
> 
> if (!rtcp_transport_channel_ && rtcp) {
>   LOG(LS_WARNING) << "Ignoring request for RTCP transport channel because we
> don't support disabling RTCP-mux once enabled."
>   return false;
> }
> 
> if (transport_name == transport_name_) {
>   // Nothing to do
>   return true;
> }
> 
> set_transport_channel(transport_controller_->CreateTransportChannel_w(
>   transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP));
> if (!transport_channel()) {
>   return false;
> }
> if (rtcp) {
>   set_rtcp_transport_channel(transport_controller_->CreateTransportChannel_w(
>     transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP));
>   if (!rtcp_transport_channel()) {
>     return false;
>   }
> } else {
>   set_rtcp_transport_channel(nullptr);
> }
> 
> transport_name_ = transport_name;
> return true;

I followed your suggestion and also removed the "rtcp" parameter from this
function so it becomes even more clear (I hope).

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/440001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:89: }
On 2015/08/19 18:32:16, pthatcher2 wrote:
> I think we need to add a lot more tests.  Sorry, we're paying up for the fact
> that this wasn't well tested before.
> 
> 1.  Test that CreateTransportChannel/DeleteTransportChannel work correctly
with
> setting the correct values on the created transport (which is created on
demand
> and deleted on demand).
> 
> 2.  Make sure that setting certain cross-Transport values works (ice_role,
> ssl_role, identity, etc) before and after creating a transport on demand.
> 
> 3.  Getting cross-Transport values works, such as ice_role, ssl_role).
> 
> 4.  Getting per-Transport values works, such as stats.
> 
> 5.  Setting per-Transport things works, such as SetLocal and
> SetRemoteDescription.
> 
> 6.  The state aggregation works properly.
> 
> 7.  Candidates get signalled up properly.
> 
> 8.  ICE role conflict reversing works properly.
> 
> 9.  The thread hopping happens properly (that might be harder to test, not
> sure).
> 

Done.

[pthatcher1, 2015-08-31 22:01:36]

Some minor things I found.

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:839: // doesn't support BUNDLE, and we only
should end up connecting one transport.
only should => should only

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1569: }
Can you put a comment about why we don't move to kIceConnectionConnecting here?

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1589: if (metrics_observer_) {
Can you move this block into a ReportTranportStats method?

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:272: rtc::scoped_refptr<rtc::RTCCertificate>
certificate_for_testing();
This seems a little odd.  Can you comment on why this is necessary?

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:2799:
EXPECT_NE(session_->voice_channel()->transport_channel(),
Why not use voice voice_rtp_transport_channel() and similar in all of these
places?

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (left):

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:258:
transport_desc_factory_.set_identity(NULL);
Was this intentional?  Why is it not needed any more?

https://codereview.webrtc.org/1246913005/diff/680001/talk/session/media/chann...
File talk/session/media/channel.cc (left):

https://codereview.webrtc.org/1246913005/diff/680001/talk/session/media/chann...
talk/session/media/channel.cc:268: }
Sorry, can you remind me again why we don't return early if the channel didn't
change?  It seems like if the TC is the same, then all the work below would be
useless.

https://codereview.webrtc.org/1246913005/diff/680001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/talk/session/media/chann...
talk/session/media/channel.cc:260: bool
BaseChannel::SetTransportChannels_w(const std::string& transport_name) {
Maybe we should just call this SetTransport_w, and then we wouldn't need both
SetTransport and SetTransportChannels (we could have just SetTransport).

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/base/fakenetwork.h
File webrtc/base/fakenetwork.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/base/fakenetwork....
webrtc/base/fakenetwork.h:56: if (start_count_++ == 0) {
That's a bit too subtle for me.  How about this?

start_count++;
if (start_count == 1) {
  sent_first_update_ = false;
  thread_->Post(this);
} else if (send_first_update_) {
  SignalNetworksChanged();
}

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

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:68: ASSERT(transport_.get() ==
nullptr);
I think ASSERT(!transport) would be sufficient.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/session.cc
File webrtc/p2p/base/session.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/session....
webrtc/p2p/base/session.cc:78: worker_thread_(worker_thread),
This is never used.  I think we can remove it, along with worker_thread(), which
I think is never called.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:11: #include <utility>
Can you comment what it's for?

Something like "// for Foo"

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:69: : content_name_(content_name),
Can we change content_name to just name?  In BaseChannel, it's called
transport_name, which is different from content_name.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:91: // When there is no channel created yet, we
shouldn't return true prematurely
Might be more clear as "We aren't completed until at least one channel is
complete, so if there are no channels, we aren't complete yet."

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:102: const TransportChannelImpl* channel =
iter.second.get();
Can you make it more readable with just one more line?
bool complete = (channel->writable() && ...);
if (!complete) {
 LOG(LS_INFO) << content_name() << " transport is not complete"
              << " because a channel is still incomplete.";
  return false;
}

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:174: return false;
{}s please

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:360: }
Does c++11 style for loops work here?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:361: destroyed_ = true;
Can we call this channels_destroyed_?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:496: CheckIfCompleted();
Wouldn't CheckIfCompleted check the gathering_state_?  If so, why do you need to
have the if statement?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:391: void CheckIfCompleted();
MaybeSignalCompleted would be a good name.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:415: bool remote_description_set_;
You can put "= false" here rather than in the constructor.  Same for the
gathering_state.

[pthatcher1, 2015-09-01 17:05:22]

The logic in the unit tests looks good.  It's just style/naming things I can
find.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
File webrtc/p2p/base/faketransportcontroller.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:9: */
Is there something you can do with git to make this look like a file copy/move
rather than a new file?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:36: rtc::Buffer packet;
Can you use TypedMessageData<rtc::Buffer>?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:345: PortAllocator* allocator =
nullptr)
We usually prefer separate methods/constructors rather than use default
variables.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:352: void SetAsync(bool async) {
async_ = async; }
Can you comment what this does?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:454: // Connect().
This isn't really a FakeTransportController.  It's a real TransportController
with some extra stuff for testing, and with fake TransportChannels.

I think TransportContollerForTest might be a better name, at least for the
TransportController unit tests (where it's really confusing that you make a fake
object to test a real one).

I'd normally suggest you just put this in transportcontroller_unittest.cc, but
if you also need it in channel_unittest.cc, and perhaps other tests, then you'll
need it in a shared place.


So here's my idea:
1.  Leave this here as FakeTransportController, with a comment saying something
like "This fake is unusual in that it's implemented with the real
TransportController code, but with fake TransportChannels underneath".

2.  Subclass FakeTransportController in transportcontroller_unittest.cc as
TransportControllerForTest with a comment like "The FakeTransportController
isn't really a full fake; It just fakes the transport controller", but then use
TransportControllerForTest throughout the rest of the file, to make that code
more clear/readable.

3.  Move any logic that is only needed by transportcontroller_unittest.cc but
not needed by other things into TransportControllerForTest and out of
FakeTransportController.  


Does that make sense?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:469: }
Can we just pass in the IceRole instead of the initiator boolean?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:45: // and WebRtcSession are combined
make => Make

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:53: // In current implementaion we just
return the role of first transport in the
"in the transport map" isn't really needed.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:58: // Can only be called once.
Can we call it SetLocalCertificate and GetLocalCertificate?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:107:
rtc::scoped_refptr<rtc::RTCCertificate> certificate_for_testing();
Can you explain why this is needed?  It seems awkward.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:110: virtual void OnMessage(rtc::Message*
pmsg);
Does this need to be protected?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:112: // protected and virtual so we can
override it in unit tests.
protected => Protected

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:115: // for unit tests
for => For

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:51: void
CreateTransportController(bool using_worker_thread) {
Instead of passing a bool, can you have something like
CreateTransportController() and CreateTransportControllerWithWorkerThread()?  I
think that would be more readable.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:158: rtc::Thread worker_thread_;
 // not used for most tests
Please capitalize and add a ".", like a full sentence.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:159:
rtc::scoped_ptr<FakeTransportController> transport_controller_;
I'm confused about this.  This is supposed to be a set of tests for a
TransportController, but you create a FakeTransportController.  Typically, a
"fake" doesn't share code with the "real" object, and is only used by code that
depends on the real thing but needs a fake for tests.

So how are we testing the real thing by creating a fake thing?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:165: std::map<std::string,
Candidates> candidates_;
Could you comment on the key.  Something like "// transport_name => candidates"
would be sufficient.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:378: // Test that transport gets
destroyed when it has no more channels
In general can you put "."s on your sentences?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:514: // Connect normally starts
candidate gathering
"normally"?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:526: FakeTransportChannel*
channel3 = CreateChannel("data", 1);
Instead of channel1, channel2, channel3, can you give them more meaningful
names, like "complete_than_destroy" or "complete_later", etc, so you can tell
what it's supposed to do.  You could apply the same idea to lots of unit tests.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:559: FakeTransportChannel*
channel1 = CreateChannel("audio", 1);
channel1/channel2 could be audio/video here, since this is kind of a bundle
test, that make sense.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:595: worker_thread_.Start();
I think it would make sense to make worker_thread_ a smart pointer that is
.reset() every time you call CreateTransportController with a worker thread. 
That way, you don't have to remember to call Start() with every test that uses
it.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:597: CompleteConnection();
It seems like this could use a more clear name, explaining what it does.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:616:
transport_controller_.reset(nullptr);
Why do you need to do this?

[Taylor Brandstetter, 2015-09-01 23:53:32]

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:839: // doesn't support BUNDLE, and we only
should end up connecting one transport.
On 2015/08/31 22:01:35, pthatcher1 wrote:
> only should => should only

Done.

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1569: }
On 2015/08/31 22:01:35, pthatcher1 wrote:
> Can you put a comment about why we don't move to kIceConnectionConnecting
here?

Done. This will all get cleaned up later in a separate CL, by the way. I just
wanted to maintain the current behavior as much as possible in this CL.

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1589: if (metrics_observer_) {
On 2015/08/31 22:01:35, pthatcher1 wrote:
> Can you move this block into a ReportTranportStats method?

Done.

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:272: rtc::scoped_refptr<rtc::RTCCertificate>
certificate_for_testing();
On 2015/08/31 22:01:35, pthatcher1 wrote:
> This seems a little odd.  Can you comment on why this is necessary?

It's used for a unit test. This is just part of a merge from this CL from hbos:
https://codereview.webrtc.org/1312643004

I'll change it to return a const ref though, since that's what it did before.

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:2799:
EXPECT_NE(session_->voice_channel()->transport_channel(),
On 2015/08/31 22:01:35, pthatcher1 wrote:
> Why not use voice voice_rtp_transport_channel() and similar in all of these
> places?

We should; done.

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (left):

https://codereview.webrtc.org/1246913005/diff/680001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:258:
transport_desc_factory_.set_identity(NULL);
On 2015/08/31 22:01:35, pthatcher1 wrote:
> Was this intentional?  Why is it not needed any more?

Yes, I removed it intentionally. And it was never needed in the first place.
Previously, TransportDescriptionFactory held a raw pointer and didn't acquire a
reference, so calling "set_identity" right before destroying it accomplished
nothing.

NOW it acquires a reference, but this is still not necessary since it uses a
smart pointer which will release the reference automatically.

https://codereview.webrtc.org/1246913005/diff/680001/talk/session/media/chann...
File talk/session/media/channel.cc (left):

https://codereview.webrtc.org/1246913005/diff/680001/talk/session/media/chann...
talk/session/media/channel.cc:268: }
On 2015/08/31 22:01:35, pthatcher1 wrote:
> Sorry, can you remind me again why we don't return early if the channel didn't
> change?  It seems like if the TC is the same, then all the work below would be
> useless.

It should now be impossible for "set_transport_channel" to be called with the
same channel. This was broken in the first place (if you recall the discussion
earlier about a dangling reference count), so returning early only ever worked
if both "old_tc" and "new_tc" are nullptr. I'll add that condition back in, and
add an ASSERT for the other, to make things more clear.

https://codereview.webrtc.org/1246913005/diff/680001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/talk/session/media/chann...
talk/session/media/channel.cc:260: bool
BaseChannel::SetTransportChannels_w(const std::string& transport_name) {
On 2015/08/31 22:01:35, pthatcher1 wrote:
> Maybe we should just call this SetTransport_w, and then we wouldn't need both
> SetTransport and SetTransportChannels (we could have just SetTransport).

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/base/fakenetwork.h
File webrtc/base/fakenetwork.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/base/fakenetwork....
webrtc/base/fakenetwork.h:56: if (start_count_++ == 0) {
On 2015/08/31 22:01:35, pthatcher1 wrote:
> That's a bit too subtle for me.  How about this?
> 
> start_count++;
> if (start_count == 1) {
>   sent_first_update_ = false;
>   thread_->Post(this);
> } else if (send_first_update_) {
>   SignalNetworksChanged();
> }

Done.

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

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:68: ASSERT(transport_.get() ==
nullptr);
On 2015/08/31 22:01:36, pthatcher1 wrote:
> I think ASSERT(!transport) would be sufficient.

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
File webrtc/p2p/base/faketransportcontroller.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:9: */
On 2015/09/01 17:05:21, pthatcher1 wrote:
> Is there something you can do with git to make this look like a file copy/move
> rather than a new file?

In my local history that's how it appears. But at some point, the combined
changes must have exceeded some threshold. The only way I can think to fix this
is to submit two separate CLs, one just renaming this file and one with the rest
of the TransportController CL. Unless you know of some git-cl magic...

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:36: rtc::Buffer packet;
On 2015/09/01 17:05:21, pthatcher1 wrote:
> Can you use TypedMessageData<rtc::Buffer>?

I don't think that would be ideal, since it would involve an extra buffer copy.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:345: PortAllocator* allocator =
nullptr)
On 2015/09/01 17:05:21, pthatcher1 wrote:
> We usually prefer separate methods/constructors rather than use default
> variables.  

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:352: void SetAsync(bool async) {
async_ = async; }
On 2015/09/01 17:05:21, pthatcher1 wrote:
> Can you comment what this does?

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:454: // Connect().
On 2015/09/01 17:05:21, pthatcher1 wrote:
> This isn't really a FakeTransportController.  It's a real TransportController
> with some extra stuff for testing, and with fake TransportChannels.
> 
> I think TransportContollerForTest might be a better name, at least for the
> TransportController unit tests (where it's really confusing that you make a
fake
> object to test a real one).
> 
> I'd normally suggest you just put this in transportcontroller_unittest.cc, but
> if you also need it in channel_unittest.cc, and perhaps other tests, then
you'll
> need it in a shared place.
> 
> 
> So here's my idea:
> 1.  Leave this here as FakeTransportController, with a comment saying
something
> like "This fake is unusual in that it's implemented with the real
> TransportController code, but with fake TransportChannels underneath".
> 
> 2.  Subclass FakeTransportController in transportcontroller_unittest.cc as
> TransportControllerForTest with a comment like "The FakeTransportController
> isn't really a full fake; It just fakes the transport controller", but then
use
> TransportControllerForTest throughout the rest of the file, to make that code
> more clear/readable.
> 
> 3.  Move any logic that is only needed by transportcontroller_unittest.cc but
> not needed by other things into TransportControllerForTest and out of
> FakeTransportController.  
> 
> 
> Does that make sense?

This is part of why I had that conversation with you about how
TransportController and Transport should just take a TransportChannelFactory,
and then work the same way regardless of whether they have a real or fake
transport channel. But if I recall correctly, we decided to hold off on that
until we made the separation between an IceTransport and DtlsTransport.

In the meantime, I'll follow your suggestions. Though there's nothing to be done
for #3, so I'll just leave it as a typedef.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/faketran...
webrtc/p2p/base/faketransportcontroller.h:469: }
On 2015/09/01 17:05:21, pthatcher1 wrote:
> Can we just pass in the IceRole instead of the initiator boolean?  

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/session.cc
File webrtc/p2p/base/session.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/session....
webrtc/p2p/base/session.cc:78: worker_thread_(worker_thread),
On 2015/08/31 22:01:36, pthatcher1 wrote:
> This is never used.  I think we can remove it, along with worker_thread(),
which
> I think is never called.

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:11: #include <utility>
On 2015/08/31 22:01:36, pthatcher1 wrote:
> Can you comment what it's for?
> 
> Something like "// for Foo"

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:69: : content_name_(content_name),
On 2015/08/31 22:01:36, pthatcher1 wrote:
> Can we change content_name to just name?  In BaseChannel, it's called
> transport_name, which is different from content_name.  

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:91: // When there is no channel created yet, we
shouldn't return true prematurely
On 2015/08/31 22:01:36, pthatcher1 wrote:
> Might be more clear as "We aren't completed until at least one channel is
> complete, so if there are no channels, we aren't complete yet."

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:102: const TransportChannelImpl* channel =
iter.second.get();
On 2015/08/31 22:01:36, pthatcher1 wrote:
> Can you make it more readable with just one more line?
> bool complete = (channel->writable() && ...);
> if (!complete) {
>  LOG(LS_INFO) << content_name() << " transport is not complete"
>               << " because a channel is still incomplete.";
>   return false;
> }

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:174: return false;
On 2015/08/31 22:01:36, pthatcher1 wrote:
> {}s please

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:360: }
On 2015/08/31 22:01:36, pthatcher1 wrote:
> Does c++11 style for loops work here?

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:361: destroyed_ = true;
On 2015/08/31 22:01:36, pthatcher1 wrote:
> Can we call this channels_destroyed_?

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:496: CheckIfCompleted();
On 2015/08/31 22:01:36, pthatcher1 wrote:
> Wouldn't CheckIfCompleted check the gathering_state_?  If so, why do you need
to
> have the if statement?

Mainly to be consistent with the prior logic, and avoid a bunch of logspam
(there's already plenty). I'll add a comment clarifying this.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:391: void CheckIfCompleted();
On 2015/08/31 22:01:36, pthatcher1 wrote:
> MaybeSignalCompleted would be a good name.

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:415: bool remote_description_set_;
On 2015/08/31 22:01:36, pthatcher1 wrote:
> You can put "= false" here rather than in the constructor.  Same for the
> gathering_state.

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:45: // and WebRtcSession are combined
On 2015/09/01 17:05:21, pthatcher1 wrote:
> make => Make

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:53: // In current implementaion we just
return the role of first transport in the
On 2015/09/01 17:05:21, pthatcher1 wrote:
> "in the transport map" isn't really needed.

I think it's a little relavant, since otherwise one may think it's the first
transport created rather than the first one in the data structure. But I'll say
"first transport alphabetically" for clarity.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:58: // Can only be called once.
On 2015/09/01 17:05:21, pthatcher1 wrote:
> Can we call it SetLocalCertificate and GetLocalCertificate?

Yeah.. I'll do that on Transport etc. as well.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:107:
rtc::scoped_refptr<rtc::RTCCertificate> certificate_for_testing();
On 2015/09/01 17:05:21, pthatcher1 wrote:
> Can you explain why this is needed?  It seems awkward.

Again, this is part of a merge; I'd ask hbos

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:110: virtual void OnMessage(rtc::Message*
pmsg);
On 2015/09/01 17:05:21, pthatcher1 wrote:
> Does this need to be protected?

I guess not; moved to private.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:112: // protected and virtual so we can
override it in unit tests.
On 2015/09/01 17:05:21, pthatcher1 wrote:
> protected => Protected

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.h:115: // for unit tests
On 2015/09/01 17:05:21, pthatcher1 wrote:
> for => For

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:51: void
CreateTransportController(bool using_worker_thread) {
On 2015/09/01 17:05:22, pthatcher1 wrote:
> Instead of passing a bool, can you have something like
> CreateTransportController() and CreateTransportControllerWithWorkerThread()? 
I
> think that would be more readable.

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:158: rtc::Thread worker_thread_;
 // not used for most tests
On 2015/09/01 17:05:21, pthatcher1 wrote:
> Please capitalize and add a ".", like a full sentence.

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:159:
rtc::scoped_ptr<FakeTransportController> transport_controller_;
On 2015/09/01 17:05:22, pthatcher1 wrote:
> I'm confused about this.  This is supposed to be a set of tests for a
> TransportController, but you create a FakeTransportController.  Typically, a
> "fake" doesn't share code with the "real" object, and is only used by code
that
> depends on the real thing but needs a fake for tests.
> 
> So how are we testing the real thing by creating a fake thing?

Addressed by your other comment

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:165: std::map<std::string,
Candidates> candidates_;
On 2015/09/01 17:05:22, pthatcher1 wrote:
> Could you comment on the key.  Something like "// transport_name =>
candidates"
> would be sufficient.

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:378: // Test that transport gets
destroyed when it has no more channels
On 2015/09/01 17:05:21, pthatcher1 wrote:
> In general can you put "."s on your sentences?

I'm understanding that I should always capitalize comments, but how do I decide
when one gets a period? Usually my rule of thumb has been "sentences get
periods, sentence fragments don't", but there are some cases (like "not used for
most tests" above) that defy that rule.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:514: // Connect normally starts
candidate gathering
On 2015/09/01 17:05:22, pthatcher1 wrote:
> "normally"?

Removed. I added a comment to transportchannelimpl.h confirming that this is
what Connect is EXPECTED to do, which makes the word "normally" here redundant.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:526: FakeTransportChannel*
channel3 = CreateChannel("data", 1);
On 2015/09/01 17:05:22, pthatcher1 wrote:
> Instead of channel1, channel2, channel3, can you give them more meaningful
> names, like "complete_than_destroy" or "complete_later", etc, so you can tell
> what it's supposed to do.  You could apply the same idea to lots of unit
tests.

I tried that, but it made things less readable to me. Given that each of these
tests is so short, I feel it's easier to just read the code to see how channel
is used. Do you want to chat about this some time?

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:595: worker_thread_.Start();
On 2015/09/01 17:05:22, pthatcher1 wrote:
> I think it would make sense to make worker_thread_ a smart pointer that is
> .reset() every time you call CreateTransportController with a worker thread. 
> That way, you don't have to remember to call Start() with every test that uses
> it.

Done.

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:597: CompleteConnection();
On 2015/09/01 17:05:22, pthatcher1 wrote:
> It seems like this could use a more clear name, explaining what it does. 

Changed to "CreateChannelsAndCompleteConnectionOnWorkerThread"; somewhat verbose
but makes it pretty clear I think

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:616:
transport_controller_.reset(nullptr);
On 2015/09/01 17:05:22, pthatcher1 wrote:
> Why do you need to do this?

It's obsolete now; I'm getting rid of it.

[pthatcher1, 2015-09-02 04:22:47]

lgtm, with nits


Nice job on this CL!

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/680001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:526: FakeTransportChannel*
channel3 = CreateChannel("data", 1);
On 2015/09/01 23:53:32, Taylor Brandstetter wrote:
> On 2015/09/01 17:05:22, pthatcher1 wrote:
> > Instead of channel1, channel2, channel3, can you give them more meaningful
> > names, like "complete_than_destroy" or "complete_later", etc, so you can
tell
> > what it's supposed to do.  You could apply the same idea to lots of unit
> tests.
> 
> I tried that, but it made things less readable to me. Given that each of these
> tests is so short, I feel it's easier to just read the code to see how channel
> is used. Do you want to chat about this some time?

If it's less readable, then don't do it.

https://codereview.webrtc.org/1246913005/diff/700001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transport.cc (right):

https://codereview.webrtc.org/1246913005/diff/700001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.cc:77: // We aren't completed until at least one
channel is compolete, so if there
compolete => complete

[pthatcher1, 2015-09-02 18:51:42]

https://codereview.webrtc.org/1246913005/diff/720001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1246913005/diff/720001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:855: }
You're right, this does nothing.

Can you double-check the unit tests to make sure we cover the cases such that we
feel confident in removing this?

https://codereview.webrtc.org/1246913005/diff/720001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1019: }
else if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle) {
  return BadAnswerSdp(source, ???, err_desc);
}

[Taylor Brandstetter, 2015-09-04 19:21:59]

https://codereview.webrtc.org/1246913005/diff/720001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1246913005/diff/720001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:855: }
On 2015/09/02 18:51:42, pthatcher1 wrote:
> You're right, this does nothing.
> 
> Can you double-check the unit tests to make sure we cover the cases such that
we
> feel confident in removing this?

The unit tests were missing the scenarios where the answer-er is using
max-bundle. I added them and they pass, even with this removal.

https://codereview.webrtc.org/1246913005/diff/720001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1019: }
On 2015/09/02 18:51:42, pthatcher1 wrote:
> else if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle) {
>   return BadAnswerSdp(source, ???, err_desc);
> }

As discussed, we'll do this in a separate CL

[Taylor Brandstetter, 2015-09-04 19:38:31]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-09-04 19:38:31]

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/1246913005/#ps760001 (title:
"Removing TransportChannelProxy")

[commit-bot: I haz the power, 2015-09-04 19:38:42]

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

[Taylor Brandstetter, 2015-09-04 19:40:58]

The CQ bit was unchecked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-09-08 17:16:12]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-09-08 17:16:14]

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/1246913005/#ps800001 (title:
" ")

[commit-bot: I haz the power, 2015-09-08 17:16:24]

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

[commit-bot: I haz the power, 2015-09-08 17:17:18]

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

[commit-bot: I haz the power, 2015-09-08 17:17:19]

Try jobs failed on following builders:
  ios_arm64 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64/builds/4275)

[Taylor Brandstetter, 2015-09-08 18:27:58]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-09-08 18:27:58]

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/1246913005/#ps820001 (title:
"Merging again")

[commit-bot: I haz the power, 2015-09-08 18:28:03]

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

[commit-bot: I haz the power, 2015-09-08 18:59:36]

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

[commit-bot: I haz the power, 2015-09-08 18:59:37]

Try jobs failed on following builders:
  mac_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/9136)

[Taylor Brandstetter, 2015-09-09 00:00:40]

deadbeef@webrtc.org changed reviewers:
- juberti@webrtc.org

[Taylor Brandstetter, 2015-09-09 00:00:42]

Peter, you may want to review again (specifically patch set 44). I found a
couple bugs when investigating the Mac unit test failure, and added some
TransportController unit tests to cover some more cases.

[pthatcher1, 2015-09-09 00:52:02]

I'm going to double check the unit tests, but the code looks fine.

https://codereview.webrtc.org/1246913005/diff/840001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1246913005/diff/840001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:873: if (ice_connection_state_ ==
kIceConnectionClosed) {
Should we ASSERT(new_state == disconnected)?

[pthatcher1, 2015-09-09 06:47:52]

The only issue I see is that I think if there are no transport channels, we
should go to a disconnected state, not a connecting state.

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:527: }
Maybe we should say:

if (!HasChannels_w()) {
  new_connection_state = kIceConnectionDisconnected;
} else if (any_failed) {
  ...
} ...

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:508: // "connected" state.
Do you mean "completed" state?  That's what it's actually testing.

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:534: // "connecting".
Why "connecting"?  Shouldn't it go to "disconnected" if there are no
connections?

[Taylor Brandstetter, 2015-09-10 01:17:48]

https://codereview.webrtc.org/1246913005/diff/840001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1246913005/diff/840001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:873: if (ice_connection_state_ ==
kIceConnectionClosed) {
On 2015/09/09 00:52:02, pthatcher1 wrote:
> Should we ASSERT(new_state == disconnected)?

There's theoretically a race condition here; JS could call "close" RIGHT as
another state was entered on the worker thread. Of course, that would be rare,
but doing "ASSERT(disconnected)" would feel a bit iffy to me.

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller.cc (right):

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller.cc:527: }
On 2015/09/09 06:47:52, pthatcher1 wrote:
> Maybe we should say:
> 
> if (!HasChannels_w()) {
>   new_connection_state = kIceConnectionDisconnected;
> } else if (any_failed) {
>   ...
> } ...
> 

That's similar to what I had initially, but we decided to change it to its
current form, and imho it's good as is now. It's pretty
straightforward/readable, and uses less code than the alternative.

P.S.: There's no cricket::kIceConnectionDisconnected, since it's WebRtcSession
that tracks the disconnected/new/connecting distinction. That messiness will go
away in a separate CL, I promise.

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportcontroller_unittest.cc (right):

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:508: // "connected" state.
On 2015/09/09 06:47:52, pthatcher1 wrote:
> Do you mean "completed" state?  That's what it's actually testing.

If you look closely, we connect two transports, delete one, THEN complete the
remaining one. What I specifically wanted to test is that when deleting one, we
stay connected, rather than flipping between connecting and connected (that's
the bug I found).

We only go to "completed" at the end so we have some sort of synchronization
point, if that makes sense. To avoid putting an arbitrary "sleep X seconds and
make sure we didn't get any extraneous signals during that time". I used the
same strategy in many of these other tests.

https://codereview.webrtc.org/1246913005/diff/860001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportcontroller_unittest.cc:534: // "connecting".
On 2015/09/09 06:47:52, pthatcher1 wrote:
> Why "connecting"?  Shouldn't it go to "disconnected" if there are no
> connections?

As mentioned in the other comment, there's no cricket::disconnected state. If
not connected/complete/failed, you're just put in the "connecting" bucket and
upper layers handle the rest. This may change later.

[pthatcher1, 2015-09-10 04:05:17]

lgtm

[Taylor Brandstetter, 2015-09-18 19:44:49]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-09-18 19:44:50]

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/1246913005/#ps880001 (title:
"Merging with master (MediaController CL, among other things)")

[commit-bot: I haz the power, 2015-09-18 19:44:54]

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

[commit-bot: I haz the power, 2015-09-18 19:56:02]

Committed patchset #46 (id:880001)

[commit-bot: I haz the power, 2015-09-18 19:56:10]

Patchset 46 (id:??) landed as
https://crrev.com/9af63f473e1d0d6c47a741a046c41642dfc1c178
Cr-Commit-Position: refs/heads/master@{#9993}

[Taylor Brandstetter, 2015-09-18 20:34:25]

A revert of this CL (patchset #46 id:880001) has been created in
https://codereview.webrtc.org/1355903002/ by deadbeef@webrtc.org.

The reason for reverting is: Broke content_browsertests. Seems to be some race
condition associated with addIceCandidate..

[tommi, 2015-09-19 09:22:35]

tommi@webrtc.org changed reviewers:
+ guoweis@webrtc.org, tommi@webrtc.org

[tommi, 2015-09-19 09:22:38]

The CR for the revert appears to have been deleted and the revert commit doesn't
have any information about why the patch was reverted.

For the next time this happens, please keep the cr around and also include the
reason for reverting in the commit.  The list of commits is included when we
roll into Chromium and we can help sheriffs by not making them dig around.

(commit is here for reference:
https://chromium.googlesource.com/external/webrtc/trunk/talk.git/+/df1b109ca5...)