Add abstract interfaces for the transport layer.

Add abstract interfaces for the transport layer.

Renamed PepperChannel to Transport and added TransportFactory. The new 
interfaces abstract transport layer for the signaling layer.

BUG=110485, 109630
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121094

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121176

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

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_sessio...
File remoting/protocol/pepper_session_manager.h (right):

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_sessio...
remoting/protocol/pepper_session_manager.h:48: scoped_ptr<TransportFactory>
transport_factory);
Why have the TransportFactory passed in here, given that PepperSessionManager is
Pepper-specific?  Is PepperSessionManager going to become more generic?

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
File remoting/protocol/pepper_transport_factory.cc (right):

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_factory.cc:232: NotifyConnectFailed();
Should return here, rather than continuing to authenticate?

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
File remoting/protocol/pepper_transport_factory.h (right):

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_factory.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Wrong year. ;)

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_factory.h:6: #define
REMOTING_PROTOCOL_PEPPER_TRANSPORT_CHANNEL_FACTORY_H_
Wrong guard #define

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:5: // This file defins interface for peer-to-peer
transport. There are
nit: ... defins ...  ->  ... defines the ...

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:6: // two type of transport: StreamTransport and
DatagramTransport. They
nit: two type ... -> two types ...

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:7: // both must be created using TransportFactory
instances and they
nit: both must ... -> must both ...

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:15: // teardown of the corresponding Transport
object.
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.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:49: // Called when the channel generates new
candidate.
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()?

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:50: virtual void OnChannelCandidate(Transport*
transport,
Name OnChannelCandidate consistently with the class, i.e. OnTransportCandidate,
or rename Transport?

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:53: // Called when the channel is being deleted.
nit: ... is being .. -> ... is about to be ...

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:63: scoped_ptr<ChannelAuthenticator>
authenticator) = 0;
Document the parameters.  Specifically, callers should understand the role of
the ChannelAuthenticator.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:68: // Name of the channel.
What does the "name" of the channel actually mean?  What's it useful for?

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:119: #endif  // REMOTING_PROTOCOL_TRANSPORT_H_
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]

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_sessio...
File remoting/protocol/pepper_session_manager.h (right):

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_sessio...
remoting/protocol/pepper_session_manager.h:48: scoped_ptr<TransportFactory>
transport_factory);
On 2012/02/07 01:17:37, Wez wrote:
> Why have the TransportFactory passed in here, given that PepperSessionManager
is
> Pepper-specific?  Is PepperSessionManager going to become more generic?

Yes. The idea is to make this class generic and not pepper-specific. Once that's
done and I migrate host to this code and remove JingleSessionManager, I'll
rename this class to JingleSesssionManager.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
File remoting/protocol/pepper_transport_factory.cc (right):

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_factory.cc:232: NotifyConnectFailed();
On 2012/02/07 01:17:37, Wez wrote:
> Should return here, rather than continuing to authenticate?

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
File remoting/protocol/pepper_transport_factory.h (right):

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_factory.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/07 01:17:37, Wez wrote:
> Wrong year. ;)

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_factory.h:6: #define
REMOTING_PROTOCOL_PEPPER_TRANSPORT_CHANNEL_FACTORY_H_
On 2012/02/07 01:17:37, Wez wrote:
> Wrong guard #define

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:5: // This file defins interface for peer-to-peer
transport. There are
On 2012/02/07 01:17:37, Wez wrote:
> nit: ... defins ...  ->  ... defines the ... 

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:6: // two type of transport: StreamTransport and
DatagramTransport. They
On 2012/02/07 01:17:37, Wez wrote:
> nit: two type ... -> two types ...

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:7: // both must be created using TransportFactory
instances and they
On 2012/02/07 01:17:37, Wez wrote:
> nit: both must ... -> must both ...

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:15: // teardown of the corresponding Transport
object.
On 2012/02/07 01:17:37, Wez wrote:
> 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.

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.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:49: // Called when the channel generates new
candidate.
On 2012/02/07 01:17:37, Wez wrote:
> 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()?

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:50: virtual void OnChannelCandidate(Transport*
transport,
On 2012/02/07 01:17:37, Wez wrote:
> Name OnChannelCandidate consistently with the class, i.e.
OnTransportCandidate,
> or rename Transport?

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:53: // Called when the channel is being deleted.
On 2012/02/07 01:17:37, Wez wrote:
> nit: ... is being .. -> ... is about to be ...

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:63: scoped_ptr<ChannelAuthenticator>
authenticator) = 0;
On 2012/02/07 01:17:37, Wez wrote:
> Document the parameters.  Specifically, callers should understand the role of
> the ChannelAuthenticator.

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:68: // Name of the channel.
On 2012/02/07 01:17:37, Wez wrote:
> What does the "name" of the channel actually mean?  What's it useful for?

Done.

http://codereview.chromium.org/9325036/diff/1/remoting/protocol/transport.h#n...
remoting/protocol/transport.h:119: #endif  // REMOTING_PROTOCOL_TRANSPORT_H_
On 2012/02/07 01:17:37, Wez wrote:
> nit: Should this file be transport_factory.h rather than transport.h, to match
> the name of the implementation file(s)?

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

[Sergey Ulanov, 2012-02-08 19:09:53]

+Simon

[simonmorris, 2012-02-08 19:45:16]

LGTM, with nits.

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:9: // reliable stream connection and the other one
- for unreliable
Remove "- ".

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:10: // datagram connection. Transport interface
itself doesn't provide
"The transport interface..."

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:12: // net::Socket or net::SocketStream interface
which provides access to
Remove "interface", or add "the" before "net::Socket".

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:15: // creation and initializations of connection
socket that can be used
"initialization"?
"of the connection socket".

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:17: // callback specified in the Connect() call.
Transport object must
"The Transport object".

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:18: // exist during whole lifetime of the
connection socket. Later
"during the whole"

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:19: // deletion of the transport socket causes
teardown of the
This is the first mention of a transport socket. Is it different from the
connection socket?

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:68: // paramters. |authenticator| is used to
secure and authenticate the
"parameters".

[Sergey Ulanov, 2012-02-08 20:20:29]

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:9: // reliable stream connection and the other one
- for unreliable
On 2012/02/08 19:45:16, simonmorris wrote:
> Remove "- ".

Done.

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:10: // datagram connection. Transport interface
itself doesn't provide
On 2012/02/08 19:45:16, simonmorris wrote:
> "The transport interface..."

Done.

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:12: // net::Socket or net::SocketStream interface
which provides access to
On 2012/02/08 19:45:16, simonmorris wrote:
> Remove "interface", or add "the" before "net::Socket".

Done.

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:15: // creation and initializations of connection
socket that can be used
On 2012/02/08 19:45:16, simonmorris wrote:
> "initialization"?
> "of the connection socket".

Done.

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:17: // callback specified in the Connect() call.
Transport object must
On 2012/02/08 19:45:16, simonmorris wrote:
> "The Transport object".

Done.

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:18: // exist during whole lifetime of the
connection socket. Later
On 2012/02/08 19:45:16, simonmorris wrote:
> "during the whole"

Done.

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:19: // deletion of the transport socket causes
teardown of the
On 2012/02/08 19:45:16, simonmorris wrote:
> This is the first mention of a transport socket. Is it different from the
> connection socket?

Done.

http://codereview.chromium.org/9325036/diff/6005/remoting/protocol/transport....
remoting/protocol/transport.h:68: // paramters. |authenticator| is used to
secure and authenticate the
On 2012/02/08 19:45:16, simonmorris wrote:
> "parameters".

Done.

[commit-bot: I haz the power, 2012-02-08 20:21:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9325036/10001

[commit-bot: I haz the power, 2012-02-08 20:53:23]

Try job failure for 9325036-10001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-02-08 21:00:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9325036/13001

[commit-bot: I haz the power, 2012-02-08 23:57:57]

Change committed as 121094

[commit-bot: I haz the power, 2012-02-09 01:53:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9325036/16003

[commit-bot: I haz the power, 2012-02-09 03:52:21]

Change committed as 121176