Issue 7200037: Channel adapter for Pepper P2P Transport API.

Issue 7200037: Channel adapter for Pepper P2P Transport API. (Closed)

Created:
9 years, 6 months ago by Sergey Ulanov

Modified:
9 years, 6 months ago

Reviewers:
Wez, Alpha Left Google

CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org

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

Visibility:
Public.

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 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89776

Patch Set 1 #

Patch Set 2 : - #

Total comments: 18

Patch Set 3 : - #

Patch Set 4 : - #

Patch Set 5 : compile on windows #

Patch Set 6 : update DEPS #

Created: 9 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+250 lines, -0 lines)			Patch
M	remoting/protocol/DEPS	View	1 2 3 4 5	1 chunk	+2 lines, -0 lines	0 comments	Download
A	remoting/protocol/pepper_p2p_channel.h	View	1 2 3 4	1 chunk	+76 lines, -0 lines	0 comments	Download
A	remoting/protocol/pepper_p2p_channel.cc	View	1 2	1 chunk	+170 lines, -0 lines	0 comments	Download
M	remoting/remoting.gyp	View		1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Wez

LGTM for the most part; just a couple of issues with naming of things, and ...

9 years, 6 months ago (2011-06-20 22:19:30 UTC) #2

Sergey Ulanov

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 ...

9 years, 6 months ago (2011-06-21 00:23:20 UTC) #3

http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p...
File remoting/protocol/pepper_p2p_channel.cc (right):

http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p...
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...
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...
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...
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...
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...
File remoting/protocol/pepper_p2p_channel.h (right):

http://codereview.chromium.org/7200037/diff/2001/remoting/protocol/pepper_p2p...
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...
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...
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...
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.

LGTM

Expand Messages | Collapse Messages