Rename PepperP2PChannel to PepperTransportSocketAdaptor

Rename PepperP2PChannel to PepperTransportSocketAdaptor. Implement net::SocketStream interface.

BUG=51198
TEST=None

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

[Wez, 2011-08-02 23:13:24]

I have two issues with this CL:

1.  I think we need separate stream vs datagram adapters, although probably
derived from a common base, since the Pepper Transport interface is common
between the two.

2.  We should use "required" callbacks throughout, since they are the
"preferred" kind in Pepper, and they make calling code simpler (usually).

Other than those, looks good. :)

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

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:53: type == STREAM ?
kTcpProtocol : kUdpProtocol));
See previous comments regarding stream vs datagram sockets.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:75:
&PepperTransportSocketAdaptor::OnRead)));
Is there any need for this to be an optional callback?

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:86:
net::CompletionCallback* callback) {
nit: Indentation.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:94:
&PepperTransportSocketAdaptor::OnWrite)));
No need to be optional?

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:106: NOTIMPLEMENTED();
Do we need to have a bug for adding buffer-size controls to P2P?

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:119: // This will return
false when the GetNextAddress() returns an
nit: Don't need "the".

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:121: // API is not
supported.
nit: "Particularly it is useful ... " -> "This helps us ..."

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:123: return
net::ERR_FAILED;
Can we return a more helpful error in this case?  Perhaps ProcessCandidates()
could return net::OK on success, or an error to return on failure?

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:134: NOTIMPLEMENTED();
How does one disconnect a Pepper stream transport?

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:171: return true;
nit: NOTIMPLEMENTED(), too?

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:198:
&PepperTransportSocketAdaptor::OnNextAddress));
Simplify this by making it use a required callback, so that it's just a chain of
calls to OnNextAddress(), each dispatching GetNextAddress() until done?  Then
there's no need for |get_address_pending_|, and ProcessCandidates becomes part
of OnNextAddress.

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

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:5: #ifndef
REMOTING_PROTOCOL_PEPPER_TRANSPORT_SOCKET_ADAPTOR_H_
nit: adapter seems to be the more common spelling in the Chrome codebase, e.g.
PseudoTcpAdapter.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:26: // This class implements
net::StreamSocket interface the Pepper P2P
nit: Couple of missing words from this comment.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:34: };
If this adaptor implements net::StreamSocket, it should implicitly be a
stream-adaptor, not a dual stream/datagram adaptor.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:36: // Observer is used to
notify about new outgoing candidates and
nit: They're "local" candidates, I think, not necessarily "outgoing".

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:37: // deletion of the
adapter.
nit: adapter vs adaptor.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:43: virtual void
OnChannelNewCandidate(const std::string& candidate) = 0;
nit: NewCandidate->NewLocalCandidate?

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:48: ChannelType type,
See above.

[Sergey Ulanov, 2011-08-03 00:02:16]

1. Removed support for UDP sockets. We don't use UDP sockets anyway.
2. I think it's better to keep some of the callbacks as optional, I explained
below why.

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

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:53: type == STREAM ?
kTcpProtocol : kUdpProtocol));
On 2011/08/02 23:13:24, Wez wrote:
> See previous comments regarding stream vs datagram sockets.

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:75:
&PepperTransportSocketAdaptor::OnRead)));
On 2011/08/02 23:13:24, Wez wrote:
> Is there any need for this to be an optional callback?
Callbacks for the upper layer (Socket interface) are optional, so required
callbacks wouldn't simplify this code much. It's better to have them optional
here because otherwise there is higher change that we will have bugs in the
calling code, i.e. it not handling optional callbacks properly. optional
callbacks may also be a bit faster.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:86:
net::CompletionCallback* callback) {
On 2011/08/02 23:13:24, Wez wrote:
> nit: Indentation.

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:94:
&PepperTransportSocketAdaptor::OnWrite)));
On 2011/08/02 23:13:24, Wez wrote:
> No need to be optional?

Same as Read()

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:106: NOTIMPLEMENTED();
On 2011/08/02 23:13:24, Wez wrote:
> Do we need to have a bug for adding buffer-size controls to P2P?
crbug.com/91439

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:119: // This will return
false when the GetNextAddress() returns an
On 2011/08/02 23:13:24, Wez wrote:
> nit: Don't need "the".

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:121: // API is not
supported.
On 2011/08/02 23:13:24, Wez wrote:
> nit: "Particularly it is useful ... " -> "This helps us ..."

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:123: return
net::ERR_FAILED;
On 2011/08/02 23:13:24, Wez wrote:
> Can we return a more helpful error in this case?  Perhaps ProcessCandidates()
> could return net::OK on success, or an error to return on failure?

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:134: NOTIMPLEMENTED();
On 2011/08/02 23:13:24, Wez wrote:
> How does one disconnect a Pepper stream transport?
Just delete it.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:171: return true;
On 2011/08/02 23:13:24, Wez wrote:
> nit: NOTIMPLEMENTED(), too?

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:198:
&PepperTransportSocketAdaptor::OnNextAddress));
On 2011/08/02 23:13:24, Wez wrote:
> Simplify this by making it use a required callback, so that it's just a chain
of
> calls to OnNextAddress(), each dispatching GetNextAddress() until done?  Then
> there's no need for |get_address_pending_|, and ProcessCandidates becomes part
> of OnNextAddress.
That's not possible with GetNextAddress(). Semantic of GetNextAddress() is
different from Read() and Write(): when the callback is called it does NOT save
the candidate, instead GetNextAddress() should be called again. In other words
it can return candidates only synchronously. It's implemented this way because
it's difficult to manage lifetime of C++ wrappers for calls like this.

[Wez, 2011-08-03 00:34:48]

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

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:75:
&PepperTransportSocketAdaptor::OnRead)));
On 2011/08/03 00:02:16, sergeyu wrote:
> On 2011/08/02 23:13:24, Wez wrote:
> > Is there any need for this to be an optional callback?
> Callbacks for the upper layer (Socket interface) are optional, so required
> callbacks wouldn't simplify this code much. It's better to have them optional
> here because otherwise there is higher change that we will have bugs in the
> calling code, i.e. it not handling optional callbacks properly. optional
> callbacks may also be a bit faster.

We could verify that calling-code copes with optional callbacks by swapping out
this layer in favour of something that does optional callbacks, in tests.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:134: NOTIMPLEMENTED();
On 2011/08/03 00:02:16, sergeyu wrote:
> On 2011/08/02 23:13:24, Wez wrote:
> > How does one disconnect a Pepper stream transport?
> Just delete it.

Would it make sense to simply delete |transport_| here, in that case?

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:198:
&PepperTransportSocketAdaptor::OnNextAddress));
On 2011/08/03 00:02:16, sergeyu wrote:
> On 2011/08/02 23:13:24, Wez wrote:
> > Simplify this by making it use a required callback, so that it's just a
chain
> of
> > calls to OnNextAddress(), each dispatching GetNextAddress() until done? 
Then
> > there's no need for |get_address_pending_|, and ProcessCandidates becomes
part
> > of OnNextAddress.
> That's not possible with GetNextAddress(). Semantic of GetNextAddress() is
> different from Read() and Write(): when the callback is called it does NOT
save
> the candidate, instead GetNextAddress() should be called again. In other words
> it can return candidates only synchronously. It's implemented this way because
> it's difficult to manage lifetime of C++ wrappers for calls like this.

OK.  The GetNextAddress() interface strikes me as unfortunate for callers. :(

[Sergey Ulanov, 2011-08-03 00:43:37]

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

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:75:
&PepperTransportSocketAdaptor::OnRead)));
On 2011/08/03 00:34:48, Wez wrote:
> On 2011/08/03 00:02:16, sergeyu wrote:
> > On 2011/08/02 23:13:24, Wez wrote:
> > > Is there any need for this to be an optional callback?
> > Callbacks for the upper layer (Socket interface) are optional, so required
> > callbacks wouldn't simplify this code much. It's better to have them
optional
> > here because otherwise there is higher change that we will have bugs in the
> > calling code, i.e. it not handling optional callbacks properly. optional
> > callbacks may also be a bit faster.
> 
> We could verify that calling-code copes with optional callbacks by swapping
out
> this layer in favour of something that does optional callbacks, in tests.

Yes, but in ether case required callbacks will not simplify this code - they
will reduce code size only by one line, yet make the code a little bit less
efficient, so there is no point in using required callbacks here.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.cc:134: NOTIMPLEMENTED();
On 2011/08/03 00:34:48, Wez wrote:
> On 2011/08/03 00:02:16, sergeyu wrote:
> > On 2011/08/02 23:13:24, Wez wrote:
> > > How does one disconnect a Pepper stream transport?
> > Just delete it.
> 
> Would it make sense to simply delete |transport_| here, in that case?
Yes, we could do that, but then we would need to handle the case transport_ =
NULL everywhere. It isn't really necessary because this function is not used for
chromoting connections anyway.

[Wez, 2011-08-03 00:47:10]

OK, LGTM once the comments on the .h are addressed.

[Sergey Ulanov, 2011-08-03 00:55:23]

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

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:5: #ifndef
REMOTING_PROTOCOL_PEPPER_TRANSPORT_SOCKET_ADAPTOR_H_
On 2011/08/02 23:13:24, Wez wrote:
> nit: adapter seems to be the more common spelling in the Chrome codebase, e.g.
> PseudoTcpAdapter.

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:26: // This class implements
net::StreamSocket interface the Pepper P2P
On 2011/08/02 23:13:24, Wez wrote:
> nit: Couple of missing words from this comment.

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:34: };
On 2011/08/02 23:13:24, Wez wrote:
> If this adaptor implements net::StreamSocket, it should implicitly be a
> stream-adaptor, not a dual stream/datagram adaptor.

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:36: // Observer is used to
notify about new outgoing candidates and
On 2011/08/02 23:13:24, Wez wrote:
> nit: They're "local" candidates, I think, not necessarily "outgoing".
they are "outgoing" because they need to be sent to the remote peer. changed it
to "local candidates"

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:37: // deletion of the
adapter.
On 2011/08/02 23:13:24, Wez wrote:
> nit: adapter vs adaptor.

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:43: virtual void
OnChannelNewCandidate(const std::string& candidate) = 0;
On 2011/08/02 23:13:24, Wez wrote:
> nit: NewCandidate->NewLocalCandidate?

Done.

http://codereview.chromium.org/7549027/diff/1/remoting/protocol/pepper_transp...
remoting/protocol/pepper_transport_socket_adaptor.h:48: ChannelType type,
On 2011/08/02 23:13:24, Wez wrote:
> See above.

Done.