Channel adapter for Pepper P2P Transport API. (issue7200037)
ser...@chromium.org
Description:
Channel adapter for Pepper P2P Transport API.
The new adapter will be used to implement protocol on top
of the Transport API.
BUG=51198
TEST=None
Please review this at http://codereview.chromium.org/7200037/
SVN Base: svn://svn.chromium.org/chrome/trunk/src
Affected files:
A remoting/protocol/pepper_p2p_channel.h
A remoting/protocol/pepper_p2p_channel.cc
M remoting/remoting.gyp
w...@chromium.org
a few
semantics questions.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc
File remoting/protocol/pepper_p2p_channel.cc (right):
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode18
remoting/protocol/pepper_p2p_channel.cc:18: const char
kPepperUdpProtocol[] = "udp";
nit: Should this be kPepper[Transport]UdpProtocolName?
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode21
remoting/protocol/pepper_p2p_channel.cc:21: int MapPepperError(int
result) {
nit: PPErrorToNetError?
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode53
remoting/protocol/pepper_p2p_channel.cc:53: void
PepperP2PChannel::AddRemoteCandidate(const std::string& candidate) {
DCHECK(CalledOnValidThread())?
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode117
remoting/protocol/pepper_p2p_channel.cc:117: LOG(ERROR) <<
"GetNextAddress() returned an error " << result;
Are there any failure modes for GetNextAddress() than that the transport
interface isn't available?
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode139
remoting/protocol/pepper_p2p_channel.cc:139: callback->Run(result);
The net::Socket interface allows for the Socket being deleted by a
completion callback; are these callback being driven from jingle so as
to allow that? (this was an issue we discussed for PseudoTcpAdaptor)
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h
File remoting/protocol/pepper_p2p_channel.h (right):
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h#newcode21
remoting/protocol/pepper_p2p_channel.h:21:
Missing "namespace protocol" here?
Does this code really belong under remoting/protocol, or under
jingle/glue?
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h#newcode24
remoting/protocol/pepper_p2p_channel.h:24: class PepperP2PChannel :
public base::NonThreadSafe,
Since this implements net::Socket for Pepper P2P, shouldn't it be
PepperP2PSocket?
nit: Also, Pepper functions and classes tend to abbreviate Pepper to PP,
which helps avoid scarily long names.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h#newcode28
remoting/protocol/pepper_p2p_channel.h:28: // here?
This comment is on a typedef; are you saying that the callback signature
should accept a cricket::Candidate, or something else? Or does the
comment apply generally to the interface?
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h#newcode35
remoting/protocol/pepper_p2p_channel.h:35: void AddRemoteCandidate(const
std::string& candidate);
Document this API?
ser...@chromium.org
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc
File remoting/protocol/pepper_p2p_channel.cc (right):
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode18
remoting/protocol/pepper_p2p_channel.cc:18: const char
kPepperUdpProtocol[] = "udp";
On 2011/06/20 22:19:31, Wez wrote:
> nit: Should this be kPepper[Transport]UdpProtocolName?
Done.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode21
remoting/protocol/pepper_p2p_channel.cc:21: int MapPepperError(int
result) {
On 2011/06/20 22:19:31, Wez wrote:
> nit: PPErrorToNetError?
Done.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode53
remoting/protocol/pepper_p2p_channel.cc:53: void
PepperP2PChannel::AddRemoteCandidate(const std::string& candidate) {
On 2011/06/20 22:19:31, Wez wrote:
> DCHECK(CalledOnValidThread())?
Done.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode117
remoting/protocol/pepper_p2p_channel.cc:117: LOG(ERROR) <<
"GetNextAddress() returned an error " << result;
On 2011/06/20 22:19:31, Wez wrote:
> Are there any failure modes for GetNextAddress() than that the
transport
> interface isn't available?
Yes, it returns PP_ERROR_NOINTERFACE in that case. Added Init() method
to handle it properly.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.cc#newcode139
remoting/protocol/pepper_p2p_channel.cc:139: callback->Run(result);
On 2011/06/20 22:19:31, Wez wrote:
> The net::Socket interface allows for the Socket being deleted by a
completion
> callback; are these callback being driven from jingle so as to allow
that?
> (this was an issue we discussed for PseudoTcpAdaptor)
Yes, Pepper implementation should be able to handle deletion from a
callback.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h
File remoting/protocol/pepper_p2p_channel.h (right):
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h#newcode21
remoting/protocol/pepper_p2p_channel.h:21:
On 2011/06/20 22:19:31, Wez wrote:
> Missing "namespace protocol" here?
> Does this code really belong under remoting/protocol, or under
jingle/glue?
It has nothing to do with libjingle, and is unlikely to be useful for
anything beside chromoting, so I'm putting it remoting/protocol.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h#newcode24
remoting/protocol/pepper_p2p_channel.h:24: class PepperP2PChannel :
public base::NonThreadSafe,
On 2011/06/20 22:19:31, Wez wrote:
> Since this implements net::Socket for Pepper P2P, shouldn't it be
> PepperP2PSocket?
"Channel" in the name is supposed to show that this class implements a
chromotocol channel on top of the P2P API. Fact that we use net::Socket
for channels is an implementation details. In future it may also provide
some other functionality beside implementing net::Socket.
> nit: Also, Pepper functions and classes tend to abbreviate Pepper to
PP, which
> helps avoid scarily long names.
PPP2PChannel - doesn't sound good to me,
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h#newcode28
remoting/protocol/pepper_p2p_channel.h:28: // here?
On 2011/06/20 22:19:31, Wez wrote:
> This comment is on a typedef; are you saying that the callback
signature should
> accept a cricket::Candidate, or something else? Or does the comment
apply
> generally to the interface?
It applies to the the whole class, and to this callback in particular.
Rephrased
it to make it easier to understand.
http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p_channel.h#newcode35
remoting/protocol/pepper_p2p_channel.h:35: void AddRemoteCandidate(const
std::string& candidate);
On 2011/06/20 22:19:31, Wez wrote:
> Document this API?
Done.