P2P Sockets host implementation.

P2P Sockets host implementation.

Currently only UDP sockets on Mac and Linux are supported. Also added 
--enable-p2papi flag. The flags just enables IPC for P2P.

BUG=None
TEST=None

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

[awong, 2011-02-28 23:37:46]

Here's my first round of comments.

Brett, FYI, Sergey's in london.  If you're able to bubble up the review
priority, that'd help avoid unnecessary 24-hour round-trips.

Thanks,
Albert

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_sockets_host.cc (right):

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:9: // Currently P2P Sockets
are not implemented for Windows yet.
Should this be in a p2p_sockets_host_posix.cc file or something?

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:47: return false;
This will leak ifc.ifc_buf won't it?

Does it make sense to keep a scoped_array to this location inside
GetLocalAddress, and pass the raw poitner to ifc.ifc_buf above?

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:49: CHECK(ifc.ifc_len <
static_cast<int>(64 * sizeof(struct ifreq)));
maybe use CHECK_LT?

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:77: return true;
return false?

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:102: }
// namespace

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:182: OnError();
I don't think socket_ is cleaned up in this situation.  state_ != STATE_OPEN.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:255: // Drop the data if we
faile to convert the address. This should
faile -> fail

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:256: // not normally happen.
Hmm...does this warrant STATE_ERROR?

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:269: const std::vector<char>&
data) {
indentation.

[brettw, 2011-03-01 05:01:08]

Sorry, am OOO all week unexpectedly. Can you have John look at the overall
architecture and filter integration?

[Sergey Ulanov, 2011-03-01 10:49:33]

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_sockets_host.cc (right):

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:9: // Currently P2P Sockets
are not implemented for Windows yet.
On 2011/02/28 23:37:46, awong wrote:
> Should this be in a p2p_sockets_host_posix.cc file or something?
Actually it needs to be split into several files. P2PSocketsHost doesn't depend
on platform, P2PSocket does. I think it is better to do it in a separate CL.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:47: return false;
On 2011/02/28 23:37:46, awong wrote:
> This will leak ifc.ifc_buf won't it?
> 
> Does it make sense to keep a scoped_array to this location inside
> GetLocalAddress, and pass the raw poitner to ifc.ifc_buf above?

Done.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:49: CHECK(ifc.ifc_len <
static_cast<int>(64 * sizeof(struct ifreq)));
On 2011/02/28 23:37:46, awong wrote:
> maybe use CHECK_LT?

Done.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:77: return true;
On 2011/02/28 23:37:46, awong wrote:
> return false?

Done.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:102: }
On 2011/02/28 23:37:46, awong wrote:
> // namespace

Done.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:182: OnError();
On 2011/02/28 23:37:46, awong wrote:
> I don't think socket_ is cleaned up in this situation.  state_ != STATE_OPEN.
Changed OnError() to close socket_ if it is open.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:255: // Drop the data if we
faile to convert the address. This should
On 2011/02/28 23:37:46, awong wrote:
> faile -> fail

Done.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:256: // not normally happen.
On 2011/02/28 23:37:46, awong wrote:
> Hmm...does this warrant STATE_ERROR?
I've added NOTREACHED() here.

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:269: const std::vector<char>&
data) {
On 2011/02/28 23:37:46, awong wrote:
> indentation.

Done.

[jam, 2011-03-01 19:35:54]

http://codereview.chromium.org/6598053/diff/28/chrome/browser/renderer_host/b...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6598053/diff/28/chrome/browser/renderer_host/b...
chrome/browser/renderer_host/browser_render_process_host.cc:490: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableP2PApi)) {
nit: this file doesn't use brace brackets for single line statements

http://codereview.chromium.org/6598053/diff/28/chrome/common/p2p_sockets.h
File chrome/common/p2p_sockets.h (right):

http://codereview.chromium.org/6598053/diff/28/chrome/common/p2p_sockets.h#ne...
chrome/common/p2p_sockets.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
this should go into content\common

http://codereview.chromium.org/6598053/diff/28/chrome/common/p2p_sockets.h#ne...
chrome/common/p2p_sockets.h:10: struct P2PSocketAddress {
nit: please comment at least this header on what it's used for

http://codereview.chromium.org/6598053/diff/28/chrome/common/render_messages_...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6598053/diff/28/chrome/common/render_messages_...
chrome/common/render_messages_internal.h:1081: // IPC messages for P2P Transport
API.
please move all these into a separate file, i.e. p2p_messages.cc/h.  this file
should be in content\common.

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_sockets_host.cc (right):

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:106: class
P2PSocketsHost::P2PSocket {
i'm curious, why isn't this class defined in a separate file?  the usual style
is one class per file.  also, what about tests?

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:340: bool* message_was_ok) {
nit: spacing

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:373: P2PSocketAddress
socket_address,
nit: spacing

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_sockets_host.h (right):

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.h:32: IDMap<P2PSocket> sockets_;
DISALLOW_COPY_AND_ASSIGN?

[awong, 2011-03-01 19:57:53]

couple more nits.

Differing to John for primary review (read: don't need to wait for my LG)

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_sockets_host.cc (right):

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:9: // Currently P2P Sockets
are not implemented for Windows yet.
On 2011/03/01 10:49:33, sergeyu wrote:
> On 2011/02/28 23:37:46, awong wrote:
> > Should this be in a p2p_sockets_host_posix.cc file or something?
> Actually it needs to be split into several files. P2PSocketsHost doesn't
depend
> on platform, P2PSocket does. I think it is better to do it in a separate CL.

okay.

Add TODO then.

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:49: CHECK(ifc.ifc_len <
static_cast<int>(64 * sizeof(struct ifreq)));
Do we need to explicilty specify the global scope here?  Does other code that
works with sockets in chromium do this?

[Sergey Ulanov, 2011-03-01 22:34:16]

Split the P2PSocketsHost into multiple files

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_sockets_host.cc (right):

http://codereview.chromium.org/6598053/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:9: // Currently P2P Sockets
are not implemented for Windows yet.
On 2011/03/01 19:57:53, awong wrote:
> On 2011/03/01 10:49:33, sergeyu wrote:
> > On 2011/02/28 23:37:46, awong wrote:
> > > Should this be in a p2p_sockets_host_posix.cc file or something?
> > Actually it needs to be split into several files. P2PSocketsHost doesn't
> depend
> > on platform, P2PSocket does. I think it is better to do it in a separate CL.
> 
> okay.
> 
> Add TODO then.

Split it into multiple files.

http://codereview.chromium.org/6598053/diff/28/chrome/browser/renderer_host/b...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6598053/diff/28/chrome/browser/renderer_host/b...
chrome/browser/renderer_host/browser_render_process_host.cc:490: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableP2PApi)) {
On 2011/03/01 19:35:54, John Abd-El-Malek wrote:
> nit: this file doesn't use brace brackets for single line statements

Done.

http://codereview.chromium.org/6598053/diff/28/chrome/common/p2p_sockets.h
File chrome/common/p2p_sockets.h (right):

http://codereview.chromium.org/6598053/diff/28/chrome/common/p2p_sockets.h#ne...
chrome/common/p2p_sockets.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2011/03/01 19:35:54, John Abd-El-Malek wrote:
> this should go into content\common

Done.

http://codereview.chromium.org/6598053/diff/28/chrome/common/p2p_sockets.h#ne...
chrome/common/p2p_sockets.h:10: struct P2PSocketAddress {
On 2011/03/01 19:35:54, John Abd-El-Malek wrote:
> nit: please comment at least this header on what it's used for

Done.

http://codereview.chromium.org/6598053/diff/28/chrome/common/render_messages_...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6598053/diff/28/chrome/common/render_messages_...
chrome/common/render_messages_internal.h:1081: // IPC messages for P2P Transport
API.
On 2011/03/01 19:35:54, John Abd-El-Malek wrote:
> please move all these into a separate file, i.e. p2p_messages.cc/h.  this file
> should be in content\common.

Done.

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_sockets_host.cc (right):

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:49: ::close(fd);
On 2011/03/01 19:57:53, awong wrote:
> Do we need to explicilty specify the global scope here?  Does other code that
> works with sockets in chromium do this?
No we don't need it, I just though it looks better this way. Removed it because
other code in src/net doesn't specify scope for sockets API.

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:106: class
P2PSocketsHost::P2PSocket {
On 2011/03/01 19:35:54, John Abd-El-Malek wrote:
> i'm curious, why isn't this class defined in a separate file? 
Done.

> the usual style
> is one class per file.  also, what about tests?
I will do add unittests in a separate CL.

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:340: bool* message_was_ok) {
On 2011/03/01 19:35:54, John Abd-El-Malek wrote:
> nit: spacing

Done.

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.cc:373: P2PSocketAddress
socket_address,
On 2011/03/01 19:35:54, John Abd-El-Malek wrote:
> nit: spacing

Done.

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_sockets_host.h (right):

http://codereview.chromium.org/6598053/diff/28/content/browser/renderer_host/...
content/browser/renderer_host/p2p_sockets_host.h:32: IDMap<P2PSocket> sockets_;
On 2011/03/01 19:35:54, John Abd-El-Malek wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

[jam, 2011-03-02 00:39:24]

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_socket_host.h (right):

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
do you have your autoprops set?  these new files should all have the svn eol
style prop set to LF. see
http://dev.chromium.org/developers/coding-style#TOC-Subversion-properties

http://codereview.chromium.org/6598053/diff/31/content/common/p2p_messages.h
File content/common/p2p_messages.h (right):

http://codereview.chromium.org/6598053/diff/31/content/common/p2p_messages.h#...
content/common/p2p_messages.h:15: IPC_MESSAGE_ROUTED2(P2PMsg_OnSocketCreated,
nit: please add comments like the other ipc message files, saying which ones are
from browser-> renderer and vice versa

http://codereview.chromium.org/6598053/diff/31/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/6598053/diff/31/content/content_browser.gypi#n...
content/content_browser.gypi:12: #'content_common',
this can probably be uncommented now, as well as adding it to
chrome_browser.gypi

[jam, 2011-03-02 00:41:09]

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_socket_host.h (right):

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host.h:15: static P2PSocketHost*
Create(P2PSocketsHost* host, int routing_id, int id,
can you add a comment as to what this is, i.e. platform code implements this to
create the platform specific implementation.

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host.h:18:
P2PSocketHost(P2PSocketsHost* host, int routing_id, int id);
if only derived classes need this, it should be protected

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_socket_host_posix.h (right):

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host_posix.h:15:
P2PSocketHostPosix(P2PSocketsHost* host, int routing_id, int id);
if only the factory method is used to construct this, then the constructor
should be private

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host_posix.h:30: class ReadWatcher :
public MessageLoopForIO::Watcher {
comment please

[Sergey Ulanov, 2011-03-02 11:00:47]

addressed all comments

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_socket_host.h (right):

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/03/02 00:39:24, John Abd-El-Malek wrote:
> do you have your autoprops set?  these new files should all have the svn eol
> style prop set to LF. see
> http://dev.chromium.org/developers/coding-style#TOC-Subversion-properties

Yes, I do have these properties set. I use git, and I guess that is why these
properties are not reflected in rietveld

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host.h:15: static P2PSocketHost*
Create(P2PSocketsHost* host, int routing_id, int id,
On 2011/03/02 00:41:09, John Abd-El-Malek wrote:
> can you add a comment as to what this is, i.e. platform code implements this
to
> create the platform specific implementation.

Done.

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host.h:18:
P2PSocketHost(P2PSocketsHost* host, int routing_id, int id);
On 2011/03/02 00:41:09, John Abd-El-Malek wrote:
> if only derived classes need this, it should be protected

Done.

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
File content/browser/renderer_host/p2p_socket_host_posix.h (right):

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host_posix.h:15:
P2PSocketHostPosix(P2PSocketsHost* host, int routing_id, int id);
On 2011/03/02 00:41:09, John Abd-El-Malek wrote:
> if only the factory method is used to construct this, then the constructor
> should be private
The factory method is in a different class (P2PSocketHost), so this method has
to be public

http://codereview.chromium.org/6598053/diff/31/content/browser/renderer_host/...
content/browser/renderer_host/p2p_socket_host_posix.h:30: class ReadWatcher :
public MessageLoopForIO::Watcher {
On 2011/03/02 00:41:09, John Abd-El-Malek wrote:
> comment please

Done.

http://codereview.chromium.org/6598053/diff/31/content/common/p2p_messages.h
File content/common/p2p_messages.h (right):

http://codereview.chromium.org/6598053/diff/31/content/common/p2p_messages.h#...
content/common/p2p_messages.h:15: IPC_MESSAGE_ROUTED2(P2PMsg_OnSocketCreated,
On 2011/03/02 00:39:24, John Abd-El-Malek wrote:
> nit: please add comments like the other ipc message files, saying which ones
are
> from browser-> renderer and vice versa

Done.

http://codereview.chromium.org/6598053/diff/31/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/6598053/diff/31/content/content_browser.gypi#n...
content/content_browser.gypi:12: #'content_common',
On 2011/03/02 00:39:24, John Abd-El-Malek wrote:
> this can probably be uncommented now, as well as adding it to
> chrome_browser.gypi

Done.

[jam, 2011-03-02 17:26:35]

lgtm