Add abstract interfaces for the transport layer.

remoting/protocol/transport.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// This file defins interface for peer-to-peer transport. There are

[Wez, 2012/02/07 01:17:37]

nit: ... defins ...  ->  ... defines the ... 

[Sergey Ulanov, 2012/02/07 04:41:12]

Done.

+// two type of transport: StreamTransport and DatagramTransport. They

[Wez, 2012/02/07 01:17:37]

nit: two type ... -> two types ...

[Sergey Ulanov, 2012/02/07 04:41:12]

Done.

+// both must be created using TransportFactory instances and they

[Wez, 2012/02/07 01:17:37]

nit: both must ... -> must both ...

[Sergey Ulanov, 2012/02/07 04:41:12]

Done.

+// provide the same interface, except that one creates reliable stream
+// connection and the other one - unreliable datagram
+// connection. After a new transport is Initialize()'ed the Connect()
+// method must be called - which starts asynchronous creation and
+// initializtions of connection socket that can be used later to send
+// and receive data. The socket is passed to the callback specified in
+// the Connect() call. Later deletion of the transport socket causes
+// teardown of the corresponding Transport object.

[Wez, 2012/02/07 01:17:37]

I think this comment needs rewording to make it clear what these interfaces are
about.

The interfaces are more like the "connectors" we discussed with Will - they're
not "transport" instances in that they don't actually deal with data transport,
rather they're used to set up a transport/channel.

[Sergey Ulanov, 2012/02/07 04:41:12]

It would not be correct to call them connectors because they must exist while
the channel exist to process signaling messages. In other words Transport not
only sets up the channel, but also supports it during its lifetime.

I've extended this comment to make the distinction between transport and channel
socket. 

+
+#ifndef REMOTING_PROTOCOL_TRANSPORT_H_
+#define REMOTING_PROTOCOL_TRANSPORT_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/callback_forward.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
+
+namespace cricket {
+class Candidate;
+}  // namespace cricket
+
+namespace net {
+class Socket;
+class StreamSocket;
+}  // namespace net
+
+namespace remoting {
+namespace protocol {
+
+class ChannelAuthenticator;
+struct TransportConfig;
+
+class Transport : public base::NonThreadSafe {
+ public:
+  class EventHandler {
+   public:
+    EventHandler() {};
+    virtual ~EventHandler() {};
+
+    // Called when the channel generates new candidate.

[Wez, 2012/02/07 01:17:37]

It's not immediately obvious to a casual reader what a "candidate" is good for -
mention that it should be passed to the remote end and supplied to
AddRemoteCandidate()?

[Sergey Ulanov, 2012/02/07 04:41:12]

Done.

+    virtual void OnChannelCandidate(Transport* transport,

[Wez, 2012/02/07 01:17:37]

Name OnChannelCandidate consistently with the class, i.e. OnTransportCandidate,
or rename Transport?

[Sergey Ulanov, 2012/02/07 04:41:12]

Done.

+                                    const cricket::Candidate& candidate) = 0;
+
+    // Called when the channel is being deleted.

[Wez, 2012/02/07 01:17:37]

nit: ... is being .. -> ... is about to be ...

[Sergey Ulanov, 2012/02/07 04:41:12]

Done.

+    virtual void OnChannelDeleted(Transport* transport) = 0;
+  };
+
+  Transport() {}
+  virtual ~Transport() {}
+
+  virtual void Initialize(const std::string& name,
+                          const TransportConfig& config,
+                          Transport::EventHandler* event_handler,
+                          scoped_ptr<ChannelAuthenticator> authenticator) = 0;

[Wez, 2012/02/07 01:17:37]

Document the parameters.  Specifically, callers should understand the role of
the ChannelAuthenticator.

[Sergey Ulanov, 2012/02/07 04:41:12]

Done.

+
+  // Adds |candidate| received from the peer.
+  virtual void AddRemoteCandidate(const cricket::Candidate& candidate) = 0;
+
+  // Name of the channel.

[Wez, 2012/02/07 01:17:37]

What does the "name" of the channel actually mean?  What's it useful for?

[Sergey Ulanov, 2012/02/07 04:41:12]

Done.

+  virtual const std::string& name() const = 0;
+
+  // Returns true if the channel is already connected.
+  virtual bool is_connected() const = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(Transport);
+};
+
+class StreamTransport : public Transport {
+ public:
+  typedef base::Callback<void(scoped_ptr<net::StreamSocket>)> ConnectedCallback;
+
+  StreamTransport() { }
+  ~StreamTransport() { }
+
+  virtual void Connect(const ConnectedCallback& callback) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(StreamTransport);
+};
+
+class DatagramTransport : public Transport {
+ public:
+  typedef base::Callback<void(scoped_ptr<net::Socket>)> ConnectedCallback;
+
+  DatagramTransport() { }
+  ~DatagramTransport() { }
+
+  virtual void Connect(const ConnectedCallback& callback) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DatagramTransport);
+};
+
+class TransportFactory {
+ public:
+  TransportFactory() { }
+  virtual ~TransportFactory() { }
+
+  virtual scoped_ptr<StreamTransport> CreateStreamTransport() = 0;
+  virtual scoped_ptr<DatagramTransport> CreateDatagramTransport() = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TransportFactory);
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_TRANSPORT_H_

[Wez, 2012/02/07 01:17:37]

nit: Should this file be transport_factory.h rather than transport.h, to match
the name of the implementation file(s)?

[Sergey Ulanov, 2012/02/07 04:41:12]

IMHO it's better to keep it called transport as the transport interface is the
one that's more important, and TransportFactory doesn't make sense without
Transport. It also matches style of authenticator.h which also defines
AuthenticatorFactory