TransportController refactoring

webrtc/p2p/base/transportcontroller.cc

+/*
+ *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/p2p/base/transportcontroller.h"
+
+#include "webrtc/base/bind.h"
+#include "webrtc/base/thread.h"
+#include "webrtc/p2p/base/dtlstransport.h"
+#include "webrtc/p2p/base/p2ptransport.h"
+
+namespace cricket {
+
+enum {
+  MSG_CONNECTIONSTATE,
+  MSG_RECEIVING,
+  MSG_GATHERINGSTATE,
+  MSG_CANDIDATESREADY,
+};
+
+struct CandidatesData : public rtc::MessageData {
+  CandidatesData(const std::string& transport_name,
+                 const Candidates& candidates)
+      : transport_name(transport_name), candidates(candidates) {}
+
+  std::string transport_name;
+  Candidates candidates;
+};
+
+TransportController::TransportController(rtc::Thread* signaling_thread,
+                                         rtc::Thread* worker_thread,
+                                         PortAllocator* port_allocator)
+    : signaling_thread_(signaling_thread),
+      worker_thread_(worker_thread),
+      port_allocator_(port_allocator),
+      ice_receiving_timeout_ms_(-1),
+      ice_role_(ICEROLE_CONTROLLING),
+      ssl_max_version_(rtc::SSL_PROTOCOL_DTLS_10),
+      ice_role_switch_(false),
+      ice_tiebreaker_(rtc::CreateRandomId64()),
+      connection_state_(kConnectionConnecting),
+      receiving_(false),
+      gathering_state_(kGatheringNew) {
+  ASSERT(signaling_thread_->IsCurrent());

[pthatcher2, 2015/08/19 18:32:15]

Is this necessary?  I don't think it is.

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+}
+
+TransportController::~TransportController() {
+  ASSERT(signaling_thread_->IsCurrent());

[pthatcher2, 2015/08/19 18:32:15]

I don't think this is necessary either.  

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

Done.

+  worker_thread_->Invoke<void>(
+      rtc::Bind(&TransportController::DestroyAllTransports_w, this));
+  signaling_thread_->Clear(this);
+}
+
+void TransportController::SetIceConnectionReceivingTimeout(int timeout_ms) {
+  ASSERT(signaling_thread_->IsCurrent());

[pthatcher2, 2015/08/19 18:32:15]

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.

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+  worker_thread_->Invoke<void>(
+      rtc::Bind(&TransportController::SetIceConnectionReceivingTimeout_w, this,
+                timeout_ms));
+}
+
+void TransportController::SetIceRole(IceRole ice_role) {
+  ASSERT(signaling_thread_->IsCurrent());
+  worker_thread_->Invoke<void>(
+      rtc::Bind(&TransportController::SetIceRole_w, this, ice_role));
+}
+
+bool TransportController::SetIdentity(rtc::SSLIdentity* identity) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(
+      rtc::Bind(&TransportController::SetIdentity_w, this, identity));
+}
+
+bool TransportController::GetIdentity(const std::string& transport_name,
+                                      rtc::SSLIdentity** identity) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(rtc::Bind(
+      &TransportController::GetIdentity_w, this, transport_name, identity));
+}
+
+bool TransportController::GetRemoteCertificate(
+    const std::string& transport_name,
+    rtc::SSLCertificate** cert) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(
+      rtc::Bind(&TransportController::GetRemoteCertificate_w, this,
+                transport_name, cert));
+}
+
+bool TransportController::SetSslMaxProtocolVersion(
+    rtc::SSLProtocolVersion version) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(rtc::Bind(
+      &TransportController::SetSslMaxProtocolVersion_w, this, version));
+}
+
+bool TransportController::SetLocalTransportDescription(

[pthatcher2, 2015/08/19 18:32:15]

We could probably call this SetLocalDescription and SetRemoteDescription. 
"Transport" is redundant now.

[Taylor Brandstetter, 2015/08/25 01:04:05]

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

+    const std::string& transport_name,
+    const TransportDescription& tdesc,
+    ContentAction action,
+    std::string* err) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(
+      rtc::Bind(&TransportController::SetLocalTransportDescription_w, this,
+                transport_name, tdesc, action, err));
+}
+
+bool TransportController::SetRemoteTransportDescription(
+    const std::string& transport_name,
+    const TransportDescription& tdesc,
+    ContentAction action,
+    std::string* err) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(
+      rtc::Bind(&TransportController::SetRemoteTransportDescription_w, this,
+                transport_name, tdesc, action, err));
+}
+
+bool TransportController::AddRemoteCandidates(const std::string& transport_name,
+                                              const Candidates& candidates,
+                                              std::string* err) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(
+      rtc::Bind(&TransportController::AddRemoteCandidates_w, this,
+                transport_name, candidates, err));
+}
+
+bool TransportController::ReadyForRemoteCandidates(
+    const std::string& transport_name) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(rtc::Bind(
+      &TransportController::ReadyForRemoteCandidates_w, this, transport_name));
+}
+
+bool TransportController::GetSslRole(rtc::SSLRole* role) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(
+      rtc::Bind(&TransportController::GetSslRole_w, this, role));
+}
+
+bool TransportController::GetStats(const std::string& transport_name,
+                                   TransportStats* stats) {
+  ASSERT(signaling_thread_->IsCurrent());
+  return worker_thread_->Invoke<bool>(
+      rtc::Bind(&TransportController::GetStats_w, this, transport_name, stats));
+}
+
+TransportChannel* TransportController::CreateTransportChannel_w(
+    const std::string& transport_name,
+    int component) {
+  ASSERT(worker_thread_->IsCurrent());

[pthatcher2, 2015/08/19 18:32:15]

These for the worker_thread_ *are* needed.  Only the signalling thread ones
aren't.

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

Acknowledged.

+
+  Transport* transport = GetOrCreateTransport_w(transport_name);
+  return transport->CreateChannel(component);
+}
+
+void TransportController::DestroyTransportChannel_w(
+    const std::string& transport_name,
+    int component) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  Transport* transport = GetTransport_w(transport_name);
+  ASSERT(transport != nullptr);

[pthatcher2, 2015/08/19 18:32:15]

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;
}

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+  transport->DestroyChannel(component);
+
+  // Just as we create a Transport when its first channel is created,
+  // we delete it when its last channel is deleted.
+  if (!transport->HasChannels()) {
+    DestroyTransport_w(transport_name);
+    UpdateState_w();
+    UpdateGatheringState_w();

[pthatcher2, 2015/08/19 18:32:15]

I think it would be more clear to put UpdateState_w and UpdateGatheringState_w
in DestroyTransport_w.

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

Done.

+  }
+}
+
+void TransportController::OnMessage(rtc::Message* pmsg) {
+  ASSERT(signaling_thread_->IsCurrent());
+
+  switch (pmsg->message_id) {
+    case MSG_CONNECTIONSTATE: {
+      rtc::TypedMessageData<ConnectionState>* data =
+          static_cast<rtc::TypedMessageData<ConnectionState>*>(pmsg->pdata);
+      SignalConnectionState(data->data());
+      delete data;
+      break;
+    }
+    case MSG_RECEIVING: {
+      rtc::TypedMessageData<bool>* data =
+          static_cast<rtc::TypedMessageData<bool>*>(pmsg->pdata);
+      SignalReceiving(data->data());
+      delete data;
+      break;
+    }
+    case MSG_GATHERINGSTATE: {
+      rtc::TypedMessageData<GatheringState>* data =
+          static_cast<rtc::TypedMessageData<GatheringState>*>(pmsg->pdata);
+      SignalGatheringState(data->data());
+      delete data;
+      break;
+    }
+    case MSG_CANDIDATESREADY: {

[pthatcher2, 2015/08/19 18:32:15]

MSG_CANDIDATESGATHERED

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

Done.

+      CandidatesData* data = static_cast<CandidatesData*>(pmsg->pdata);
+      SignalCandidatesGathered(data->transport_name, data->candidates);
+      delete data;
+      break;
+    }
+    default:
+      ASSERT(false);
+  }
+}
+
+Transport* TransportController::CreateTransport_w(
+    const std::string& transport_name) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  Transport* transport = new DtlsTransport<P2PTransport>(
+      transport_name, port_allocator(), identity_.get());
+  return transport;
+}
+
+Transport* TransportController::GetOrCreateTransport_w(
+    const std::string& transport_name) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  Transport* transport = GetTransport_w(transport_name);
+  if (transport)
+    return transport;

[pthatcher2, 2015/08/19 18:32:15]

{}s please

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+
+  transport = CreateTransport_w(transport_name);
+  transport->SetChannelReceivingTimeout(ice_receiving_timeout_ms_);
+  transport->SetIceRole(ice_role_);
+  transport->SetIceTiebreaker(ice_tiebreaker_);
+  transport->SetSslMaxProtocolVersion(ssl_max_version_);
+  transport->SignalConnecting.connect(
+      this, &TransportController::OnTransportConnecting_w);
+  transport->SignalWritableState.connect(
+      this, &TransportController::OnTransportWritableState_w);
+  transport->SignalReceivingState.connect(
+      this, &TransportController::OnTransportReceivingState_w);
+  transport->SignalGatheringState.connect(
+      this, &TransportController::OnTransportGatheringState_w);
+  transport->SignalRouteChange.connect(
+      this, &TransportController::OnTransportRouteChange_w);
+  transport->SignalRoleConflict.connect(
+      this, &TransportController::OnTransportRoleConflict_w);
+  transport->SignalCompleted.connect(
+      this, &TransportController::OnTransportCompleted_w);
+  transport->SignalFailed.connect(this,
+                                  &TransportController::OnTransportFailed_w);

[pthatcher2, 2015/08/19 18:32:15]

Can you make this one consistent with the other with the line break?

[Taylor Brandstetter, 2015/08/25 01:04:05]

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

[pthatcher1, 2015/08/25 18:40:38]

Just go with "git cl format". 

+  transport->SignalCandidatesGathered.connect(
+      this, &TransportController::OnTransportCandidatesGathered_w);
+  if (identity_)
+    transport->SetIdentity(identity_.get());

[pthatcher2, 2015/08/19 18:32:15]

{}s please.  

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

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+  transports_[transport_name] = transport;

[pthatcher2, 2015/08/19 18:32:15]

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?

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+
+  return transport;
+}
+
+Transport* TransportController::GetTransport_w(
+    const std::string& transport_name) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  auto iter = transports_.find(transport_name);
+  return (iter != transports_.end()) ? iter->second : nullptr;
+}
+
+void TransportController::DestroyTransport_w(
+    const std::string& transport_name) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  auto iter = transports_.find(transport_name);
+  if (iter != transports_.end()) {
+    delete iter->second;
+    transports_.erase(transport_name);
+  }
+}
+
+void TransportController::DestroyAllTransports_w() {
+  ASSERT(worker_thread_->IsCurrent());
+
+  for (const auto& kv : transports_) {
+    delete kv.second;
+  }
+  transports_.clear();
+}
+
+void TransportController::SetIceConnectionReceivingTimeout_w(int timeout_ms) {
+  ASSERT(worker_thread_->IsCurrent());
+  ice_receiving_timeout_ms_ = timeout_ms;
+  for (const auto& kv : transports_) {
+    kv.second->SetChannelReceivingTimeout(timeout_ms);
+  }
+}
+
+void TransportController::SetIceRole_w(IceRole ice_role) {
+  ASSERT(worker_thread_->IsCurrent());
+  ice_role_ = ice_role;
+  for (const auto& kv : transports_) {
+    kv.second->SetIceRole(ice_role_);
+  }
+}
+
+bool TransportController::SetIdentity_w(rtc::SSLIdentity* identity) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  if (identity_)
+    return false;

[pthatcher2, 2015/08/19 18:32:15]

{} please

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

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+  identity_.reset(identity);
+
+  for (const auto& kv : transports_) {
+    kv.second->SetIdentity(identity_.get());
+  }
+  return true;
+}
+
+bool TransportController::GetIdentity_w(const std::string& transport_name,
+                                        rtc::SSLIdentity** identity) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  Transport* t = GetTransport_w(transport_name);
+  if (!t) {
+    return false;
+  }
+
+  return t->GetIdentity(identity);
+}
+
+bool TransportController::GetRemoteCertificate_w(
+    const std::string& transport_name,
+    rtc::SSLCertificate** cert) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  Transport* t = GetTransport_w(transport_name);
+  if (!t) {
+    return false;
+  }
+
+  return t->GetRemoteCertificate(cert);
+}
+
+bool TransportController::SetSslMaxProtocolVersion_w(
+    rtc::SSLProtocolVersion version) {
+  ASSERT(worker_thread_->IsCurrent());
+
+  // Max SSL version can only be set before transports are created
+  if (!transports_.empty()) {
+    return false;
+  }
+
+  ssl_max_version_ = version;
+  return true;
+}
+
+bool TransportController::SetLocalTransportDescription_w(
+    const std::string& transport_name,
+    const TransportDescription& tdesc,
+    ContentAction action,
+    std::string* err) {
+  ASSERT(worker_thread()->IsCurrent());
+
+  Transport* transport = GetTransport_w(transport_name);
+  if (!transport) {
+    // If we didn't find a transport, that's not an error;
+    // it could have been deleted as a result of bundling.
+    return true;

[pthatcher2, 2015/08/19 18:32:14]

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.

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+  }
+
+  if (!transport->SetLocalTransportDescription(tdesc, action, err)) {
+    return false;
+  }
+  transport->ConnectChannels();

[pthatcher2, 2015/08/19 18:32:15]

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?

[Taylor Brandstetter, 2015/08/25 01:04:05]

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.

+  return true;
+}
+
+bool TransportController::SetRemoteTransportDescription_w(
+    const std::string& transport_name,
+    const TransportDescription& tdesc,
+    ContentAction action,
+    std::string* err) {
+  ASSERT(worker_thread()->IsCurrent());
+
+  Transport* transport = GetTransport_w(transport_name);
+  if (!transport) {
+    // If we didn't find a transport, that's not an error;
+    // it could have been deleted as a result of bundling.
+    return true;
+  }
+
+  return transport->SetRemoteTransportDescription(tdesc, action, err);
+}
+
+bool TransportController::AddRemoteCandidates_w(
+    const std::string& transport_name,
+    const Candidates& candidates,
+    std::string* err) {
+  ASSERT(worker_thread()->IsCurrent());
+
+  Transport* transport = GetTransport_w(transport_name);
+  if (!transport) {
+    // If we didn't find a transport, that's not an error;
+    // it could have been deleted as a result of bundling.
+    return true;
+  }
+
+  return transport->AddRemoteCandidates(candidates, err);
+}
+
+bool TransportController::ReadyForRemoteCandidates_w(
+    const std::string& transport_name) {
+  ASSERT(worker_thread()->IsCurrent());
+
+  Transport* transport = GetTransport_w(transport_name);
+  if (!transport) {
+    return false;
+  }
+  return transport->local_description_set() &&
+         transport->remote_description_set();

[pthatcher2, 2015/08/19 18:32:15]

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.

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

Done.

+}
+
+bool TransportController::GetSslRole_w(rtc::SSLRole* role) {
+  ASSERT(worker_thread()->IsCurrent());
+
+  // TODO(mallinath) - Return role of each transport, as role may differ from
+  // one another.
+  // In current implementaion we just return the role of first transport in the
+  // transport map.
+  for (const auto& kv : transports_) {
+    return kv.second->GetSslRole(role);
+  }

[pthatcher2, 2015/08/19 18:32:15]

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.

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

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.

+  return false;
+}
+
+bool TransportController::GetStats_w(const std::string& transport_name,
+                                     TransportStats* stats) {
+  ASSERT(worker_thread()->IsCurrent());
+
+  Transport* transport = GetTransport_w(transport_name);
+  if (!transport) {
+    return false;
+  }
+  return transport->GetStats(stats);
+}
+
+void TransportController::OnTransportConnecting_w(Transport* transport) {
+  ASSERT(worker_thread_->IsCurrent());
+  UpdateState_w();
+}
+
+void TransportController::OnTransportWritableState_w(Transport* transport) {
+  ASSERT(worker_thread_->IsCurrent());
+  UpdateState_w();
+}
+
+void TransportController::OnTransportReceivingState_w(Transport* transport) {
+  ASSERT(worker_thread_->IsCurrent());
+  UpdateState_w();
+}
+
+void TransportController::OnTransportGatheringState_w(Transport* transport) {
+  ASSERT(worker_thread_->IsCurrent());
+  UpdateGatheringState_w();

[pthatcher2, 2015/08/19 18:32:15]

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.

[Taylor Brandstetter, 2015/08/25 01:04:05]

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

+}
+
+void TransportController::OnTransportRouteChange_w(Transport* transport,
+                                                   int component,
+                                                   const Candidate& candidate) {
+  ASSERT(worker_thread_->IsCurrent());
+  // This signal is not currently used for anything

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

Then let's not listen to it.  

[Taylor Brandstetter, 2015/08/25 01:04:05]

Done.

+}
+
+void TransportController::OnTransportRoleConflict_w() {
+  ASSERT(worker_thread_->IsCurrent());
+
+  if (ice_role_switch_) {
+    LOG(LS_WARNING) << "Repeat of role conflic signal from Transport.";

[pthatcher2, 2015/08/19 18:32:15]

conflic => conflict

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

Done.

+    return;
+  }
+
+  ice_role_switch_ = true;
+  IceRole reversed_role = (ice_role_ == ICEROLE_CONTROLLING)
+                              ? ICEROLE_CONTROLLED
+                              : ICEROLE_CONTROLLING;
+  for (const auto& kv : transports_) {
+    kv.second->SetIceRole(reversed_role);
+  }
+}
+
+void TransportController::OnTransportCompleted_w(Transport* transport) {
+  ASSERT(worker_thread_->IsCurrent());
+  UpdateState_w();
+}
+
+void TransportController::OnTransportFailed_w(Transport* transport) {
+  ASSERT(worker_thread_->IsCurrent());
+  UpdateState_w();
+}

[pthatcher2, 2015/08/19 18:32:15]

Can you put these up next to others? 

(before OnTransportRoleConflict_w)

[Taylor Brandstetter, 2015/08/25 01:04:05]

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

+
+void TransportController::OnTransportCandidatesGathered_w(
+    Transport* transport,
+    const std::vector<Candidate>& candidates) {
+  ASSERT(worker_thread_->IsCurrent());
+  CandidatesData* data =
+      new CandidatesData(transport->content_name(), candidates);
+  signaling_thread_->Post(this, MSG_CANDIDATESREADY, data);
+}
+
+void TransportController::UpdateState_w() {
+  ASSERT(worker_thread_->IsCurrent());
+
+  ConnectionState new_state = kConnectionConnecting;
+  bool receiving = false;
+  // If we don't have ANY Transports, we shouldn't signal that we're completed
+  if (!transports_.empty()) {
+    bool failed = false;
+    bool connected = true;
+    bool completed = true;

[pthatcher2, 2015/08/19 18:32:15]

Can you call these the following?

any_failed
all_receiving
all_connected
all_completed

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

Done.

+    for (const auto& kv : transports_) {
+      if (!kv.second->Completed()) {
+        completed = false;
+      }
+      if (!kv.second->all_channels_writable()) {
+        connected = false;
+      }
+      if (kv.second->any_channel_receiving()) {
+        // The connection is considered receiving if at least one transport is
+        // receiving on any channel.
+        receiving = true;
+      }

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

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;
}

[Taylor Brandstetter, 2015/08/25 01:04:05]

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

+    }
+
+    if (failed) {

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

Actually, wait.... failed can never happen.  So why do we even have it?

[Taylor Brandstetter, 2015/08/25 01:04:05]

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

+      new_state = kConnectionFailed;
+    } else if (completed) {
+      new_state = kConnectionCompleted;
+    } else if (connected) {
+      new_state = kConnectionConnected;
+    }
+  }
+
+  if (connection_state_ != new_state) {
+    connection_state_ = new_state;
+    signaling_thread_->Post(
+        this, MSG_CONNECTIONSTATE,

[pthatcher2, 2015/08/19 18:32:15]

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.

[Taylor Brandstetter, 2015/08/25 01:04:05]

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.

+        new rtc::TypedMessageData<ConnectionState>(new_state));
+  }
+
+  if (receiving_ != receiving) {
+    receiving_ = receiving;
+    signaling_thread_->Post(this, MSG_RECEIVING,
+                            new rtc::TypedMessageData<bool>(receiving));
+  }
+}
+
+void TransportController::UpdateGatheringState_w() {
+  ASSERT(worker_thread_->IsCurrent());
+
+  GatheringState new_state = kGatheringNew;
+  if (!transports_.empty()) {
+    bool gathering = false;
+    bool complete = true;
+    for (const auto& kv : transports_) {
+      // Complete if ALL transports complete
+      if (kv.second->gathering_state() != kGatheringComplete) {
+        complete = false;
+      }
+      // Gathering if ANY transport started gathering
+      if (kv.second->gathering_state() != kGatheringNew) {

[pthatcher2, 2015/08/19 18:32:15]

Did you mean kGatheringGathering here?

[Taylor Brandstetter, 2015/08/25 01:04:05]

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".

+        gathering = true;
+      }
+    }
+    if (complete) {
+      new_state = kGatheringComplete;
+    } else if (gathering) {
+      new_state = kGatheringGathering;
+    }
+  }

[pthatcher2, 2015/08/19 18:32:15]

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;
}

[Taylor Brandstetter, 2015/08/25 01:04:05]

See earlier comment about UpdateState_w

+
+  if (gathering_state_ != new_state) {
+    gathering_state_ = new_state;
+    signaling_thread_->Post(
+        this, MSG_GATHERINGSTATE,
+        new rtc::TypedMessageData<GatheringState>(new_state));
+  }
+}
+
+}  // namespace cricket