TransportController refactoring

webrtc/p2p/base/transportcontroller.h

+/*
+ *  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.
+ */
+
+#ifndef WEBRTC_P2P_BASE_TRANSPORTCONTROLLER_H_
+#define WEBRTC_P2P_BASE_TRANSPORTCONTROLLER_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "webrtc/base/sigslot.h"
+#include "webrtc/base/sslstreamadapter.h"
+#include "webrtc/p2p/base/candidate.h"
+#include "webrtc/p2p/base/transport.h"
+
+namespace rtc {
+class Thread;
+}
+
+namespace cricket {
+
+class TransportController : public sigslot::has_slots<>,
+                            public rtc::MessageHandler {
+ public:
+  TransportController(rtc::Thread* signaling_thread,
+                      rtc::Thread* worker_thread,
+                      PortAllocator* port_allocator);
+
+  virtual ~TransportController();
+
+  rtc::Thread* signaling_thread() const { return signaling_thread_; }
+  rtc::Thread* worker_thread() const { return worker_thread_; }
+  PortAllocator* port_allocator() const { return port_allocator_; }
+
+  void SetIceConnectionReceivingTimeout(int timeout_ms);
+  void SetIceRole(IceRole ice_role);
+
+  // Takes ownership of identity and passes it down to transports
+  bool SetIdentity(rtc::SSLIdentity* identity);

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

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)

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

Done.

+
+  // Caller owns returned identity
+  bool GetIdentity(const std::string& transport_name,
+                   rtc::SSLIdentity** identity);
+
+  // Caller owns returned certificate
+  bool GetRemoteCertificate(const std::string& transport_name,
+                            rtc::SSLCertificate** cert);
+
+  bool SetSslMaxProtocolVersion(rtc::SSLProtocolVersion version);
+  bool SetLocalTransportDescription(const std::string& transport_name,
+                                    const TransportDescription& tdesc,
+                                    ContentAction action,
+                                    std::string* err);
+  bool SetRemoteTransportDescription(const std::string& transport_name,
+                                     const TransportDescription& tdesc,
+                                     ContentAction action,
+                                     std::string* err);
+  bool AddRemoteCandidates(const std::string& transport_name,
+                           const Candidates& candidates,
+                           std::string* err);
+
+  bool ReadyForRemoteCandidates(const std::string& transport_name);
+
+  bool GetSslRole(rtc::SSLRole* role);
+  bool GetStats(const std::string& transport_name, TransportStats* stats);
+
+  virtual TransportChannel* CreateTransportChannel_w(
+      const std::string& transport_name,
+      int component);
+  virtual void DestroyTransportChannel_w(const std::string& transport_name,
+                                         int component);
+
+  void ConnectChannels();
+
+  // All of these signals are fired on the signalling thread.
+  sigslot::signal1<ConnectionState> SignalConnectionState;
+  sigslot::signal1<bool> SignalReceiving;
+  sigslot::signal1<GatheringState> SignalGatheringState;

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

Can you comment on each of these about how they aggregate, and how they are
aggregated?

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

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

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

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.

[Taylor Brandstetter, 2015/08/25 20:39:54]

Done.

+  // (transport_name, candidates)
+  sigslot::signal2<const std::string&, const Candidates&>
+      SignalCandidatesGathered;
+
+ protected:
+  virtual void OnMessage(rtc::Message* pmsg);
+
+  // protected and virtual so we can override it in unit tests.
+  virtual Transport* CreateTransport_w(const std::string& transport_name);
+
+  // for unit tests
+  const std::map<std::string, Transport*>& transports() { return transports_; }
+
+  Transport* GetOrCreateTransport_w(const std::string& transport_name);
+  Transport* GetTransport_w(const std::string& transport_name);
+  void DestroyTransport_w(const std::string& transport_name);
+  void DestroyAllTransports_w();

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

Why are the above protected? Also for unit tests?

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

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

+
+ private:
+  void SetIceConnectionReceivingTimeout_w(int timeout_ms);
+  void SetIceRole_w(IceRole ice_role);
+  bool SetIdentity_w(rtc::SSLIdentity* identity);
+  bool GetIdentity_w(const std::string& transport_name,
+                     rtc::SSLIdentity** identity);
+  bool GetRemoteCertificate_w(const std::string& transport_name,
+                              rtc::SSLCertificate** cert);
+  bool SetSslMaxProtocolVersion_w(rtc::SSLProtocolVersion version);
+  void ConnectChannels_w();
+  bool SetLocalTransportDescription_w(const std::string& transport_name,
+                                      const TransportDescription& tdesc,
+                                      ContentAction action,
+                                      std::string* err);
+  bool SetRemoteTransportDescription_w(const std::string& transport_name,
+                                       const TransportDescription& tdesc,
+                                       ContentAction action,
+                                       std::string* err);
+  bool AddRemoteCandidates_w(const std::string& transport_name,
+                             const Candidates& candidates,
+                             std::string* err);
+  bool ReadyForRemoteCandidates_w(const std::string& transport_name);
+  bool GetSslRole_w(rtc::SSLRole* role);
+  bool GetStats_w(const std::string& transport_name, TransportStats* stats);

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

Can you put these in the same order I suggested for the public ones?

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

Done.

+
+  // Handlers for signals from Transport.
+  void OnTransportConnecting_w(Transport* transport);
+  void OnTransportWritableState_w(Transport* transport);
+  void OnTransportReceivingState_w(Transport* transport);
+  void OnTransportGatheringState_w(Transport* transport);
+  void OnTransportRouteChange_w(Transport* transport,
+                                int component,
+                                const Candidate& candidate);
+  void OnTransportRoleConflict_w();
+  void OnTransportCompleted_w(Transport* transport);
+  void OnTransportFailed_w(Transport* transport);
+  void OnTransportCandidatesGathered_w(
+      Transport* transport,
+      const std::vector<Candidate>& candidates);
+
+  // Update our internal aggregate Transport states,
+  // and signal if they changed.
+  void UpdateState_w();

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

I'd call this UpdateAggregateStates_w() and just remove the comment.

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

Done.

+
+  // Update aggregate candidate gathering state
+  void UpdateGatheringState_w();

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

I'd call this UpdateAggregateGatheringState_w() and just remove the comment.

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

Done.

+
+  rtc::Thread* const signaling_thread_;
+  rtc::Thread* const worker_thread_;
+  PortAllocator* const port_allocator_;
+
+  // Timeout value in milliseconds for which no ICE connection receives
+  // any packets
+  int ice_receiving_timeout_ms_;

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

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.

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

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

+  IceRole ice_role_;

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

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.

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

Done.

+  rtc::scoped_ptr<rtc::SSLIdentity> identity_;
+  rtc::SSLProtocolVersion ssl_max_version_;
+  std::map<std::string, Transport*> transports_;

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

Given that there are usually going to be 1-3 transports, I'd suggest making this
a vector rather than a map.

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

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

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

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.  

+
+  // Flag which will be set to true after the first role switch
+  bool ice_role_switch_;
+  uint64 ice_tiebreaker_;
+
+  // Aggregate state for Transports
+  ConnectionState connection_state_;
+  bool receiving_;
+  bool candidates_allocated_;

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

This looks like it isn't used anywhere.

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

Yep, that got replaced by gathering_state_. Removed.

+  GatheringState gathering_state_;
+};
+
+}  // namespace cricket
+
+#endif  // WEBRTC_P2P_BASE_TRANSPORTCONTROLLER_H_