Chromoting protocol implementation based on P2P Transport API.

Chromoting protocol implementation based on P2P Transport API.

Then new code is not enabled yet, there are still some issues that 
need to be resolved before this code is used by default. I plan to 
make the switch in M16, as there isn't enough time left in M15.

TEST=Manual
BUG=51198
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100805

[Sergey Ulanov, 2011-08-30 21:54:33]

I apologize for the size of this CL, but I'm not sure how I can split it.

[Wez, 2011-09-01 01:08:28]

I can't see anything obviously wrong with this. :)

What does strike me is that there's a lot of duplication of things like the SSL
protocol initialization, with the JingleSession.  That suggests to me that we
could simplify things by putting the remoting-specific stuff like setting up SSL
into a single remoting-session class, and have that use a lower-level
channel-management interface implemented for each transport platform.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_cha...
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_cha...
remoting/protocol/pepper_channel.h:30: struct TransportConfig {
nit: Consider moving this out to be PepperChannelConfig?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_cha...
remoting/protocol/pepper_channel.h:46: virtual const std::string& name() = 0;
Some comments to explain these APIs would be handy.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.cc:84: LOG(ERROR) << "Received error in
response to session-initate message: \""
nit: initiate

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.cc:85: << response->Str()
Are we happy there can be nothing in the response that might be dangerous to
log?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.cc:89: // here. Parse the response stanza to
find falure reason.
nit: failure

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session.h (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:43: // connections.
Wording.  How about:

"Implements the protocol::Session interface using the Pepper P2P Transport
API."?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:57: pp::Instance* pp_instance();
Why is this exposed?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:61: // Chromotocol Session interface.
Do we need all the blank lines between the definitions below - I'm not sure it
makes it any more readable.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:111: JingleMessageReply* reply);
Do we need to re-name the JingleXXX classes to something more appropriate?
Why is this one of the only methods with a description?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:124: void SendTransportInfos();
I assume the s means we're sending multiple TransportInfo structures?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:141: std::string sid_;
What is "sid"?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session_manager.h (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session_manager.h:79: void SendReply(const
buzz::XmlElement* original_stanza,
Should this be JingleMessage?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_str...
File remoting/protocol/pepper_stream_channel.cc (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_str...
remoting/protocol/pepper_stream_channel.cc:106: // TODO(sergeyu): Add properties
for ACK delay and Neagle in the
nit: Neagle -> TCP_NODELAY

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_tra...
File remoting/protocol/pepper_transport_socket_adapter.cc (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_tra...
remoting/protocol/pepper_transport_socket_adapter.cc:154: net::IPAddressNumber
ip_address(4);
Does the content of IPAddressNumber get initialised for us?

[Sergey Ulanov, 2011-09-08 00:48:36]

I realize that there is some code duplication for SSL initialization, but it all
will go away once SSL is implemented in P2P API.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_cha...
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_cha...
remoting/protocol/pepper_channel.h:30: struct TransportConfig {
On 2011/09/01 01:08:29, Wez wrote:
> nit: Consider moving this out to be PepperChannelConfig?

Moved it of PepperChannel, but didn't rename it. It to be handy not only for
pepper channels.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_cha...
remoting/protocol/pepper_channel.h:46: virtual const std::string& name() = 0;
On 2011/09/01 01:08:29, Wez wrote:
> Some comments to explain these APIs would be handy.

Done.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.cc:84: LOG(ERROR) << "Received error in
response to session-initate message: \""
On 2011/09/01 01:08:29, Wez wrote:
> nit: initiate

Done.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.cc:85: << response->Str()
On 2011/09/01 01:08:29, Wez wrote:
> Are we happy there can be nothing in the response that might be dangerous to
> log?
The error message may contain the original session-initiate message, which
contains auth token, but it's not possible to reuse auth token, so I don't think
it's very dangerous to log it here. And we already log all signalling traffic in
our client.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.cc:89: // here. Parse the response stanza to
find falure reason.
On 2011/09/01 01:08:29, Wez wrote:
> nit: failure

Done.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session.h (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:43: // connections.
On 2011/09/01 01:08:29, Wez wrote:
> Wording.  How about:
> 
> "Implements the protocol::Session interface using the Pepper P2P Transport
> API."?

Done.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:57: pp::Instance* pp_instance();
On 2011/09/01 01:08:29, Wez wrote:
> Why is this exposed?

Done.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:61: // Chromotocol Session interface.
On 2011/09/01 01:08:29, Wez wrote:
> Do we need all the blank lines between the definitions below - I'm not sure it
> makes it any more readable.

Done.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:111: JingleMessageReply* reply);
On 2011/09/01 01:08:29, Wez wrote:
> Do we need to re-name the JingleXXX classes to something more appropriate?
JingleXXX is the right name for these, they implement jingle protocol.

> Why is this one of the only methods with a description?
Added comments for other methods where appropriate.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:124: void SendTransportInfos();
On 2011/09/01 01:08:29, Wez wrote:
> I assume the s means we're sending multiple TransportInfo structures?
Renamed it to SendTransportInfo() It sends just one transport-info message, but
the message may contain info for more than one candidate.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.h:141: std::string sid_;
On 2011/09/01 01:08:29, Wez wrote:
> What is "sid"?
renamed to session_id_

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session_manager.h (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session_manager.h:79: void SendReply(const
buzz::XmlElement* original_stanza,
On 2011/09/01 01:08:29, Wez wrote:
> Should this be JingleMessage?
No. Original XML is needed so that it can be added to reply in case we we need
to send an error. For example, |original_stanza| may not be a valid jingle
message.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_str...
File remoting/protocol/pepper_stream_channel.cc (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_str...
remoting/protocol/pepper_stream_channel.cc:106: // TODO(sergeyu): Add properties
for ACK delay and Neagle in the
On 2011/09/01 01:08:29, Wez wrote:
> nit: Neagle -> TCP_NODELAY
TODO is done now.

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_tra...
File remoting/protocol/pepper_transport_socket_adapter.cc (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_tra...
remoting/protocol/pepper_transport_socket_adapter.cc:154: net::IPAddressNumber
ip_address(4);
On 2011/09/01 01:08:29, Wez wrote:
> Does the content of IPAddressNumber get initialised for us?
yes. IPAddressNumber is a typedef for std::vector.

[Sergey Ulanov, 2011-09-09 02:26:27]

ping

[Wez, 2011-09-09 23:39:11]

LGTM with some further comments.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_ch...
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_ch...
remoting/protocol/pepper_channel.h:31: // PepperStreamChannel and
PepperDatagramChannel.
This is actually an interface defining functions common to both stream and
datagram channels, isn't it?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:26: const int kTransportInfoSendDelayMs = 2;
Why 2ms?  If the issue is that there may be several AddCandidate calls made by
the caller then zero would be as good a timeout, to cause a single info to be
sent with all of them in it?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:62: session_id_ =
base::Int64ToString(base::RandGenerator(kint64max));
Add a comment to explain why it's OK for our Id to be random, and for us not to
check that we haven't used the same Id previously.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:72:
initiate_request_->SendIq(message.ToXml());
Add a comment e.g. "Send the session-initiate stanza to the host."

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:134: const SessionConfig*
PepperSession::config() {
Check CalledOnValidThread()?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:138: void PepperSession::set_config(const
SessionConfig* config) {
Here too?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:158: void
PepperSession::set_receiver_token(const std::string& receiver_token) {
And again?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:195: *reply =
JingleMessageReply(JingleMessageReply::INVALID_SID);
Can that ever happen?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:260: // TODO(sergeyu): Pass exact rejection
reason from reply and pass it
nit: You mean "parse exact..."?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:360: // touch any channels. Needs to be done
in the end because the
nit: "... in the end ..."?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session.h (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:46: // TODO(sergeyu): Move this type and
error() method to Session class.
Isn't Session an interface, not a class?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:59: // Chromotocol Session interface.
nit: protocol::Session interface.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:94: void OnSessionInitiateResponse(const
buzz::XmlElement* response);
Probably best to add a brief comment for each of these.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:96: // Called when an error occurs. Sets
error code and closes the session.
nit: "... sets |error_| and ..."

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:128: // Sets |state_| to |new_state| and
calls state callback.
nit: state _change_ callback.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session_manager.cc (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.cc:31: // Initializes the session
client.
nit: Initializes/starts the session manager?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.cc:66: //
I think this limitation is covered by the TODO.

You might consider saving all of the details supplied, and adding a comment only
where they are actually used to explain why only the first entries are used.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.cc:124: }
nit: I think this would be easier to read as a couple of if() { ... return }
clauses, rather than if...else-if...else, even though that would duplicate the
SendReply line.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session_manager.h (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.h:42: // P2P Transport API.
It's an implementation of protocol::SessionManager, not SessionClient?

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.h:49: // SessionManager interface.
nit: I prefer to qualify interface names in comments.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_st...
File remoting/protocol/pepper_stream_channel.cc (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_st...
remoting/protocol/pepper_stream_channel.cc:29: const char kTcpProtocol[] =
"tcp";
nit: This name makes it unclear whether it means the kind of TCP protocol to
use, or it's a TCP setting, or that, as is the case, it tells Transport to use
PseudoTCP; I'd suggest kStreamTransport or kStreamTransportName instead.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_tr...
File remoting/protocol/pepper_transport_socket_adapter.cc (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_tr...
remoting/protocol/pepper_transport_socket_adapter.cc:153: // won't complain.
nit: Wording.
"We don't have a meaningful peer address, but we can't return an error, so we
return a INADDR_ANY instead."

[Sergey Ulanov, 2011-09-12 19:50:50]

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_ch...
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_ch...
remoting/protocol/pepper_channel.h:31: // PepperStreamChannel and
PepperDatagramChannel.
On 2011/09/09 23:39:12, Wez wrote:
> This is actually an interface defining functions common to both stream and
> datagram channels, isn't it?

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:26: const int kTransportInfoSendDelayMs = 2;
On 2011/09/09 23:39:12, Wez wrote:
> Why 2ms?  If the issue is that there may be several AddCandidate calls made by
> the caller then zero would be as good a timeout, to cause a single info to be
> sent with all of them in it?

Yes, this is so that we send fewer transport-info messages by packing multiple
candidates into one message. The ports are opened asynchronously in the browser
process, so we need greater than 0 value here. Added comments about it.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:62: session_id_ =
base::Int64ToString(base::RandGenerator(kint64max));
On 2011/09/09 23:39:12, Wez wrote:
> Add a comment to explain why it's OK for our Id to be random, and for us not
to
> check that we haven't used the same Id previously.

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:72:
initiate_request_->SendIq(message.ToXml());
On 2011/09/09 23:39:12, Wez wrote:
> Add a comment e.g. "Send the session-initiate stanza to the host."

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:134: const SessionConfig*
PepperSession::config() {
On 2011/09/09 23:39:12, Wez wrote:
> Check CalledOnValidThread()?

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:138: void PepperSession::set_config(const
SessionConfig* config) {
On 2011/09/09 23:39:12, Wez wrote:
> Here too?

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:158: void
PepperSession::set_receiver_token(const std::string& receiver_token) {
On 2011/09/09 23:39:12, Wez wrote:
> And again?

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:195: *reply =
JingleMessageReply(JingleMessageReply::INVALID_SID);
On 2011/09/09 23:39:12, Wez wrote:
> Can that ever happen?
Yes if somebody guesses SID and tries to spoof session messages. If we didn't
handle this case it would be possible to do at least DoS attack.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:260: // TODO(sergeyu): Pass exact rejection
reason from reply and pass it
On 2011/09/09 23:39:12, Wez wrote:
> nit: You mean "parse exact..."?

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.cc:360: // touch any channels. Needs to be done
in the end because the
On 2011/09/09 23:39:12, Wez wrote:
> nit: "... in the end ..."?
Removed the comment.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session.h (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:46: // TODO(sergeyu): Move this type and
error() method to Session class.
On 2011/09/09 23:39:12, Wez wrote:
> Isn't Session an interface, not a class?

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:59: // Chromotocol Session interface.
On 2011/09/09 23:39:12, Wez wrote:
> nit: protocol::Session interface.
Changed to "Session interface" Don't think we need to specify namespace given
that we are in protocol namespace.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:94: void OnSessionInitiateResponse(const
buzz::XmlElement* response);
On 2011/09/09 23:39:12, Wez wrote:
> Probably best to add a brief comment for each of these.

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:96: // Called when an error occurs. Sets
error code and closes the session.
On 2011/09/09 23:39:12, Wez wrote:
> nit: "... sets |error_| and ..."

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session.h:128: // Sets |state_| to |new_state| and
calls state callback.
On 2011/09/09 23:39:12, Wez wrote:
> nit: state _change_ callback.

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session_manager.cc (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.cc:31: // Initializes the session
client.
On 2011/09/09 23:39:12, Wez wrote:
> nit: Initializes/starts the session manager?
Removed this comment.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.cc:66: //
On 2011/09/09 23:39:12, Wez wrote:
> I think this limitation is covered by the TODO.
> 
> You might consider saving all of the details supplied, and adding a comment
only
> where they are actually used to explain why only the first entries are used.

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.cc:124: }
On 2011/09/09 23:39:12, Wez wrote:
> nit: I think this would be easier to read as a couple of if() { ... return }
> clauses, rather than if...else-if...else, even though that would duplicate the
> SendReply line.

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
File remoting/protocol/pepper_session_manager.h (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.h:42: // P2P Transport API.
On 2011/09/09 23:39:12, Wez wrote:
> It's an implementation of protocol::SessionManager, not SessionClient?

Done.

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_se...
remoting/protocol/pepper_session_manager.h:49: // SessionManager interface.
On 2011/09/09 23:39:12, Wez wrote:
> nit: I prefer to qualify interface names in comments.

This class is defined in remoting::protocol namespace, so I don't think that we
need to qualify interface here (but it would make sense if SessionManager was
defined in a different namespace).

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_st...
File remoting/protocol/pepper_stream_channel.cc (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_st...
remoting/protocol/pepper_stream_channel.cc:29: const char kTcpProtocol[] =
"tcp";
On 2011/09/09 23:39:12, Wez wrote:
> nit: This name makes it unclear whether it means the kind of TCP protocol to
> use, or it's a TCP setting, or that, as is the case, it tells Transport to use
> PseudoTCP; I'd suggest kStreamTransport or kStreamTransportName instead.

Just removed this constant, and replaced it with "tcp" when calling
Transport_Dev constructor. In future I plan to change this parameter to enum, so
we will no need this string

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_tr...
File remoting/protocol/pepper_transport_socket_adapter.cc (right):

http://codereview.chromium.org/7778022/diff/17001/remoting/protocol/pepper_tr...
remoting/protocol/pepper_transport_socket_adapter.cc:153: // won't complain.
On 2011/09/09 23:39:12, Wez wrote:
> nit: Wording.
> "We don't have a meaningful peer address, but we can't return an error, so we
> return a INADDR_ANY instead."

This comment was copied from pseudotcp_adapter.cc. Fixed here and in
PseudoTcpAdapter.

[commit-bot: I haz the power, 2011-09-12 20:18:27]

Try job failure for 7778022-25001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...