Chromoting protocol implementation based on P2P Transport API. (issue 7778022)

[ser...@chromium.org]

Reviewers: Wez,

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

Description:
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

Please review this at http://codereview.chromium.org/7778022/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M remoting/base/constants.h
M remoting/base/constants.cc
M remoting/protocol/jingle_session.cc
A remoting/protocol/pepper_channel.h
A remoting/protocol/pepper_channel.cc
A remoting/protocol/pepper_session.h
A remoting/protocol/pepper_session.cc
A remoting/protocol/pepper_session_manager.h
A remoting/protocol/pepper_session_manager.cc
A remoting/protocol/pepper_stream_channel.h
A remoting/protocol/pepper_stream_channel.cc
M remoting/protocol/pepper_transport_socket_adapter.h
M remoting/protocol/pepper_transport_socket_adapter.cc
M remoting/remoting.gyp

[w...@chromium.org]

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_channel.h
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_channel.h#newcode30
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_channel.h#newcode46
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_session.cc
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_session.cc#newcode84
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_session.cc#newcode85
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_session.cc#newcode89
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_session.h
File remoting/protocol/pepper_session.h (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_session.h#newcode43
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_session.h#newcode57
remoting/protocol/pepper_session.h:57: pp::Instance* pp_instance();
Why is this exposed?

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_session.h#newcode61
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_session.h#newcode111
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_session.h#newcode124
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_session.h#newcode141
remoting/protocol/pepper_session.h:141: std::string sid_;
What is "sid"?

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

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_session_manager.h#newcode79
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_stream_channel.cc
File remoting/protocol/pepper_stream_channel.cc (right):

http://codereview.chromium.org/7778022/diff/3001/remoting/protocol/pepper_stream_channel.cc#newcode106
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_transport_socket_adapter.cc
File remoting/protocol/pepper_transport_socket_adapter.cc (right):

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

http://codereview.chromium.org/7778022/