First step in providing a UdpTransportChannel.

webrtc/p2p/base/udptransportchannel.h

+/*
+ *  Copyright 2016 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_UDPTRANSPORTCHANNEL_H_
+#define WEBRTC_P2P_BASE_UDPTRANSPORTCHANNEL_H_
+
+#include <memory>
+#include <string>
+
+#include "webrtc/base/optional.h"
+#include "webrtc/base/thread_checker.h"
+#include "webrtc/p2p/base/packettransportinterface.h"
+
+namespace rtc {
+class AsyncPacketSocket;
+class PhysicalSocketServer;
+class SocketAddress;
+class SocketServer;
+class Thread;
+}
+
+namespace cricket {
+
+enum UdpTransportState {
+  UDPTRANSPORT_STATE_INIT,
+  UDPTRANSPORT_STATE_CONNECTING,
+  UDPTRANSPORT_STATE_COMPLETED,
+  UDPTRANSPORT_STATE_FAILED
+};

[pthatcher1, 2016/11/04 22:58:58]

Can you use enum class, in which case you don't need the "UDPTRANSPORT_STATE"
prefix?

[johan, 2016/11/07 17:51:53]

Sounds good. Doing the enum class, and declaring that one as part of
UdpTransportChannel. 

+
+class UdpTransportChannel : public rtc::PacketTransportInterface {
+ public:
+  UdpTransportChannel(const std::string& transport_name, int component);
+  UdpTransportChannel(const std::string& transport_name,
+                      int component,

[pthatcher1, 2016/11/04 22:58:58]

I thought you said you removed the components.  Can you please remove them?

[johan, 2016/11/07 17:51:53]

Done.

+                      rtc::SocketServer* ss);
+  ~UdpTransportChannel();
+
+  const std::string debug_name() const override { return debug_name_; }
+
+  bool receiving() const override {
+    // TODO(johan): Implement method and signal.
+    return true;
+  }
+
+  bool writable() const override;
+
+  int SendPacket(const char* data,
+                 size_t len,
+                 const rtc::PacketOptions& options,
+                 int flags) override;
+
+  int SetOption(rtc::Socket::Option opt, int value) override { return 0; }
+
+  int GetError() override { return send_error_; }
+
+  UdpTransportState GetState() const {

[pthatcher1, 2016/11/04 22:58:58]

Why not just "state()"?

[johan, 2016/11/07 17:51:53]

Method name is an artifact from the TransportChannel implementation. Renaming is
ok for me.

+    RTC_DCHECK_RUN_ON(&network_thread_checker_);
+    return state_;
+  }
+
+  // Check GetState() for resulting state.

[pthatcher1, 2016/11/04 22:58:58]

I'm not sure what meaning this comment has.  

And why is this method public?

[johan, 2016/11/07 17:51:53]

It is public, because it is not called from any other method or constructor in
this class. We could consider making it private.
In that case either the constructor would have to invoke CreateSocket(), which I
think is a bad idea. Or we would need something like a public start() method
that only calls CreateSocket() to separate control logic and implementation
details.

[pthatcher1, 2016/11/09 16:34:59]

I'd be in favor of a Start method.

[johan, 2016/11/09 17:22:03]

Renamed CreateSocket to Start and rephrased comment.

+  void CreateSocket();
+
+  void SetRemoteParameters(const rtc::SocketAddress& remote);
+
+  // Returns default value, if local candidate is not set.

[pthatcher1, 2016/11/04 22:58:58]

"value, if" => "value if"

And what does "local candidate not set" mean?  Why not just say "Returns default
value if in the INIT or FAILED state"?

[johan, 2016/11/07 17:51:53]

Rephrasing the comment, as the return type changed to
rtc::Optional<rtc::SocketAddress>

+  // Consider checking GetState() before calling local_parameters().
+  const rtc::SocketAddress& local_parameters();
+
+ private:
+  void OnSocketReadPacket(rtc::AsyncPacketSocket* socket,
+                          const char* data,
+                          size_t len,
+                          const rtc::SocketAddress& remote_addr,
+                          const rtc::PacketTime& packet_time);
+  void OnSocketSentPacket(rtc::AsyncPacketSocket* socket,
+                          const rtc::SentPacket& packet);
+  void SetTransportChannelState(
+      UdpTransportState state);  // Set State and Signal.
+  UdpTransportState state_ = UDPTRANSPORT_STATE_INIT;

[pthatcher1, 2016/11/04 22:58:58]

Fields should go below methods

[johan, 2016/11/07 17:51:53]

Acknowledged.

+  bool IsLocalConsistent();
+  void UpdateState();
+  void UpdateDebugName();
+  int send_error_ = 0;
+  std::string transport_name_;
+  std::string debug_name_;
+  int component_;
+  std::unique_ptr<rtc::AsyncPacketSocket> socket_;
+  rtc::Optional<rtc::SocketAddress> local_parameters_;
+  rtc::Optional<rtc::SocketAddress> remote_parameters_;
+  rtc::SocketServer* socket_server_;
+  rtc::ThreadChecker network_thread_checker_;

[pthatcher1, 2016/11/04 22:58:58]

The fields filled by the constructor should go first, then the rest.

[johan, 2016/11/07 17:51:53]

Ok, transport_name_ and socket_server_ first.

+};
+}  // namespace cricket
+
+#endif  // WEBRTC_P2P_BASE_UDPTRANSPORTCHANNEL_H_