Expose the p2p client in content/public

This CL is in preparation for making a public way for the media/cast library to access the p2p sockets.
As of yet, there is no factory methods, that will be added in a later CL.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238553

[Alpha Left Google, 2013-10-25 20:07:51]

https://codereview.chromium.org/45183002/diff/50001/content/public/renderer/s...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/50001/content/public/renderer/s...
content/public/renderer/socket_client.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
The file name should better reflect the class defined. Also year is 2013.

https://codereview.chromium.org/45183002/diff/50001/content/public/renderer/s...
content/public/renderer/socket_client.h:22: class P2PSocketClientInterface :
Suggestion: maybe rename this to P2PSocketClient.

[hubbe, 2013-10-25 21:26:57]

https://codereview.chromium.org/45183002/diff/50001/content/public/renderer/s...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/50001/content/public/renderer/s...
content/public/renderer/socket_client.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2013/10/25 20:07:51, Alpha wrote:
> The file name should better reflect the class defined. Also year is 2013.

Done.

https://codereview.chromium.org/45183002/diff/50001/content/public/renderer/s...
content/public/renderer/socket_client.h:22: class P2PSocketClientInterface :
On 2013/10/25 20:07:51, Alpha wrote:
> Suggestion: maybe rename this to P2PSocketClient. 

Done.

[Alpha Left Google, 2013-10-25 21:28:47]

LGTM after the renaming is done.

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
This is still called socket_client.h though, should it be renamed to
p2p_socket_client.h to be precise? The other P2PXXX classes are placed under a
p2p/ directory that's why they don't have a p2p_ prefix. I think this file needs
to be more clear.

[hubbe, 2013-10-25 21:31:38]

Adding OWNERS & security review people.

[Tom Sepez, 2013-10-25 21:34:23]

On 2013/10/25 21:31:38, hubbe wrote:
> Adding OWNERS & security review people.

LGTM for p2p_messages.h

[jam, 2013-10-25 23:14:14]

I prefer not to review changes to content/public without seeing the caller,
otherwise it's hard to know how small we can trim that API. Where can I see
that?

[hubbe1, 2013-10-25 23:27:08]

The caller is not written yet. There will be some code in
chrome/render/media that glues this up with media/cast. I'll let you know
when there is something to see.

    /Hubbe



On Fri, Oct 25, 2013 at 4:14 PM, <jam@chromium.org> wrote:

> I prefer not to review changes to content/public without seeing the caller,
> otherwise it's hard to know how small we can trim that API. Where can I see
> that?
>
>
https://codereview.chromium.**org/45183002/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hubbe, 2013-11-09 00:12:50]

Rough client for this code in:

https://codereview.chromium.org/66293003/

PTAL.

     /Hubbe

On 2013/10/25 23:27:08, hubbe1 wrote:
> The caller is not written yet. There will be some code in
> chrome/render/media that glues this up with media/cast. I'll let you know
> when there is something to see.
> 
>     /Hubbe
> 
> 
> 
> On Fri, Oct 25, 2013 at 4:14 PM, <mailto:jam@chromium.org> wrote:
> 
> > I prefer not to review changes to content/public without seeing the caller,
> > otherwise it's hard to know how small we can trim that API. Where can I see
> > that?
> >
> >
>
https://codereview.chromium.**org/45183002/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[hubbe, 2013-11-12 17:59:58]

Ping? Jam?

   /Hubbe

On 2013/11/09 00:12:50, hubbe wrote:
> Rough client for this code in:
> 
> https://codereview.chromium.org/66293003/
> 
> PTAL.
> 
>      /Hubbe
> 
> On 2013/10/25 23:27:08, hubbe1 wrote:
> > The caller is not written yet. There will be some code in
> > chrome/render/media that glues this up with media/cast. I'll let you know
> > when there is something to see.
> > 
> >     /Hubbe
> > 
> > 
> > 
> > On Fri, Oct 25, 2013 at 4:14 PM, <mailto:jam@chromium.org> wrote:
> > 
> > > I prefer not to review changes to content/public without seeing the
caller,
> > > otherwise it's hard to know how small we can trim that API. Where can I
see
> > > that?
> > >
> > >
> >
>
https://codereview.chromium.**org/45183002/%253Chttps://codereview.chromium.o...>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Alpha Left Google, 2013-11-13 23:25:27]

Please fix the CL description to be the following format:

<title>
<empty line>
<description>

Lines should be less than 72 chars each.

[hubbe, 2013-11-19 02:44:51]

Ping?

[jam, 2013-11-20 23:26:24]

where's the factory method?

https://codereview.chromium.org/45183002/diff/140001/content/public/common/p2...
File content/public/common/p2p_sockets.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/common/p2...
content/public/common/p2p_sockets.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: this should be content/public/common/p2p_socket_type.h per convention (see
the other files in this directory)

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2013/10/25 21:28:48, Alpha wrote:
> This is still called socket_client.h though, should it be renamed to
> p2p_socket_client.h to be precise? The other P2PXXX classes are placed under a
> p2p/ directory that's why they don't have a p2p_ prefix. I think this file
needs
> to be more clear.

+1

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:21: // thread which is specified in
Init().
you're describing the implementation details. do we need to expose that to the
embedder?

Same for the ref counted base class and the threading. can we hide that and
expose only a single threaded interface?

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:28: class Delegate {
see http://www.chromium.org/developers/content-module/content-api for the
guidelines for the public API.

in particular, each interface should be in a separate file (i.e.
p2p_socket_client_delegate.h)

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:32: virtual void OnOpen(const
net::IPEndPoint& address) = 0;
nit: document every method

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:63: virtual void set_delegate(Delegate*
delegate) = 0;
nit: these two methods don't conform to google style naming conventions. since
they're virtual they should be GetSocketID and SetDelegate

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:72:
DISALLOW_COPY_AND_ASSIGN(P2PSocketClient);
nit: don't need this since pure virtual interfaces can't be copied

[hubbe, 2013-11-21 22:07:30]

https://codereview.chromium.org/45183002/diff/140001/content/public/common/p2...
File content/public/common/p2p_sockets.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/common/p2...
content/public/common/p2p_sockets.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2013/11/20 23:26:24, jam wrote:
> nit: this should be content/public/common/p2p_socket_type.h per convention
(see
> the other files in this directory)

Done.

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2013/11/20 23:26:24, jam wrote:
> On 2013/10/25 21:28:48, Alpha wrote:
> > This is still called socket_client.h though, should it be renamed to
> > p2p_socket_client.h to be precise? The other P2PXXX classes are placed under
a
> > p2p/ directory that's why they don't have a p2p_ prefix. I think this file
> needs
> > to be more clear.
> 
> +1

Done.

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:21: // thread which is specified in
Init().
On 2013/11/20 23:26:24, jam wrote:
> you're describing the implementation details. do we need to expose that to the
> embedder?
> 

Fixed.

> Same for the ref counted base class and the threading. can we hide that and
> expose only a single threaded interface?

I'm not sure what the proper way to handle this is actually. The implementation
uses lots of callbacks with ref-counts to prevent the object from going away
prematurely. I could potentially change the callbacks to use weak pointers, but
that seems difficult since the callbacks cross threads.

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:21: // thread which is specified in
Init().
On 2013/11/20 23:26:24, jam wrote:
> you're describing the implementation details. do we need to expose that to the
> embedder?
> 

Fixed.

> Same for the ref counted base class and the threading. can we hide that and
> expose only a single threaded interface?


Not sure how to handle this actually. The implementation uses callbacks across
threads, and the ref-counting is used to prevent the object from going away
prematurely. There are other ways to handle this, but ref-counting seems like
the cleanest. Let me know if you have a better idea.

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:28: class Delegate {
On 2013/11/20 23:26:24, jam wrote:
> see http://www.chromium.org/developers/content-module/content-api for the
> guidelines for the public API.
> 
> in particular, each interface should be in a separate file (i.e.
> p2p_socket_client_delegate.h)

Done.

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:32: virtual void OnOpen(const
net::IPEndPoint& address) = 0;
On 2013/11/20 23:26:24, jam wrote:
> nit: document every method

Done.

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:63: virtual void set_delegate(Delegate*
delegate) = 0;
On 2013/11/20 23:26:24, jam wrote:
> nit: these two methods don't conform to google style naming conventions. since
> they're virtual they should be GetSocketID and SetDelegate

Done.

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:72:
DISALLOW_COPY_AND_ASSIGN(P2PSocketClient);
On 2013/11/20 23:26:24, jam wrote:
> nit: don't need this since pure virtual interfaces can't be copied

Done.

[jam, 2013-11-23 02:03:13]

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:21: // thread which is specified in
Init().
On 2013/11/21 22:07:30, hubbe wrote:
> On 2013/11/20 23:26:24, jam wrote:
> > you're describing the implementation details. do we need to expose that to
the
> > embedder?
> > 
> 
> Fixed.
> 
> > Same for the ref counted base class and the threading. can we hide that and
> > expose only a single threaded interface?
> 
> I'm not sure what the proper way to handle this is actually. The
implementation
> uses lots of callbacks with ref-counts to prevent the object from going away
> prematurely. I could potentially change the callbacks to use weak pointers,
but
> that seems difficult since the callbacks cross threads.
> 

What I mean is that this is an interface. The implementation could be ref
counted, but does that have to be exposed on the interface?

https://codereview.chromium.org/45183002/diff/350001/content/public/renderer/...
File content/public/renderer/p2p_socket_client.h (right):

https://codereview.chromium.org/45183002/diff/350001/content/public/renderer/...
content/public/renderer/p2p_socket_client.h:57: // Create a new
P2PSocketClient(). Please note that you must called
nit: "must call"

https://codereview.chromium.org/45183002/diff/350001/content/public/renderer/...
content/public/renderer/p2p_socket_client.h:58: // Init() before using the
socket. Also, the methods on the returned
i'm curious why not combine the initialization and creation in one step?

https://codereview.chromium.org/45183002/diff/350001/content/public/renderer/...
content/public/renderer/p2p_socket_client.h:60: P2PSocketClient*
CreateP2PSocket();
nit: the convention in content/public is to put the static Create method inside
the interface

[hubbe, 2013-11-25 19:15:07]

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:21: // thread which is specified in
Init().
On 2013/11/23 02:03:14, jam wrote:
> On 2013/11/21 22:07:30, hubbe wrote:
> > On 2013/11/20 23:26:24, jam wrote:
> > > you're describing the implementation details. do we need to expose that to
> the
> > > embedder?
> > > 
> > 
> > Fixed.
> > 
> > > Same for the ref counted base class and the threading. can we hide that
and
> > > expose only a single threaded interface?
> > 
> > I'm not sure what the proper way to handle this is actually. The
> implementation
> > uses lots of callbacks with ref-counts to prevent the object from going away
> > prematurely. I could potentially change the callbacks to use weak pointers,
> but
> > that seems difficult since the callbacks cross threads.
> > 
> 
> What I mean is that this is an interface. The implementation could be ref
> counted, but does that have to be exposed on the interface?

I think so, because we expect the callers to use scoped_refptr<P2PSocketClient>
to manage the pointer(s) they get back from the factory method.

https://codereview.chromium.org/45183002/diff/350001/content/public/renderer/...
File content/public/renderer/p2p_socket_client.h (right):

https://codereview.chromium.org/45183002/diff/350001/content/public/renderer/...
content/public/renderer/p2p_socket_client.h:57: // Create a new
P2PSocketClient(). Please note that you must called
On 2013/11/23 02:03:14, jam wrote:
> nit: "must call"

Done.

https://codereview.chromium.org/45183002/diff/350001/content/public/renderer/...
content/public/renderer/p2p_socket_client.h:58: // Init() before using the
socket. Also, the methods on the returned
On 2013/11/23 02:03:14, jam wrote:
> i'm curious why not combine the initialization and creation in one step?

Done.

https://codereview.chromium.org/45183002/diff/350001/content/public/renderer/...
content/public/renderer/p2p_socket_client.h:60: P2PSocketClient*
CreateP2PSocket();
On 2013/11/23 02:03:14, jam wrote:
> nit: the convention in content/public is to put the static Create method
inside
> the interface

Done.

[jam, 2013-11-27 21:30:17]

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:21: // thread which is specified in
Init().
On 2013/11/25 19:15:07, hubbe wrote:
> On 2013/11/23 02:03:14, jam wrote:
> > On 2013/11/21 22:07:30, hubbe wrote:
> > > On 2013/11/20 23:26:24, jam wrote:
> > > > you're describing the implementation details. do we need to expose that
to
> > the
> > > > embedder?
> > > > 
> > > 
> > > Fixed.
> > > 
> > > > Same for the ref counted base class and the threading. can we hide that
> and
> > > > expose only a single threaded interface?
> > > 
> > > I'm not sure what the proper way to handle this is actually. The
> > implementation
> > > uses lots of callbacks with ref-counts to prevent the object from going
away
> > > prematurely. I could potentially change the callbacks to use weak
pointers,
> > but
> > > that seems difficult since the callbacks cross threads.
> > > 
> > 
> > What I mean is that this is an interface. The implementation could be ref
> > counted, but does that have to be exposed on the interface?
> 
> I think so, because we expect the callers to use
scoped_refptr<P2PSocketClient>
> to manage the pointer(s) they get back from the factory method.
> 

what is the ownership here? i.e. can the embedder layer just hold on to a raw
pointer?

https://codereview.chromium.org/45183002/diff/440001/content/public/renderer/...
File content/public/renderer/p2p_socket_client.h (right):

https://codereview.chromium.org/45183002/diff/440001/content/public/renderer/...
content/public/renderer/p2p_socket_client.h:27: static P2PSocketClient*
CreateP2PSocket(
nit: just Create, the "P2PSocket" in the name is redundant (i.e.
P2PSocketClient::CreateP2PSocket has 2 P2PSocket)

https://codereview.chromium.org/45183002/diff/440001/content/renderer/p2p/soc...
File content/renderer/p2p/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/440001/content/renderer/p2p/soc...
content/renderer/p2p/socket_client.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
nit: I just noticed that this file should be renamed to socket_client_impl.h
now, same for the cc file

[hubbe, 2013-11-27 23:23:01]

PTAL

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:21: // thread which is specified in
Init().
On 2013/11/27 21:30:18, jam wrote:
> On 2013/11/25 19:15:07, hubbe wrote:
> > On 2013/11/23 02:03:14, jam wrote:
> > > On 2013/11/21 22:07:30, hubbe wrote:
> > > > On 2013/11/20 23:26:24, jam wrote:
> > > > > you're describing the implementation details. do we need to expose
that
> to
> > > the
> > > > > embedder?
> > > > > 
> > > > 
> > > > Fixed.
> > > > 
> > > > > Same for the ref counted base class and the threading. can we hide
that
> > and
> > > > > expose only a single threaded interface?
> > > > 
> > > > I'm not sure what the proper way to handle this is actually. The
> > > implementation
> > > > uses lots of callbacks with ref-counts to prevent the object from going
> away
> > > > prematurely. I could potentially change the callbacks to use weak
> pointers,
> > > but
> > > > that seems difficult since the callbacks cross threads.
> > > > 
> > > 
> > > What I mean is that this is an interface. The implementation could be ref
> > > counted, but does that have to be exposed on the interface?
> > 
> > I think so, because we expect the callers to use
> scoped_refptr<P2PSocketClient>
> > to manage the pointer(s) they get back from the factory method.
> > 
> 
> what is the ownership here? i.e. can the embedder layer just hold on to a raw
> pointer?

That's not the intent, the intent is for the user to always use a scoped_refptr.
I changed the factory to return one.

https://codereview.chromium.org/45183002/diff/440001/content/public/renderer/...
File content/public/renderer/p2p_socket_client.h (right):

https://codereview.chromium.org/45183002/diff/440001/content/public/renderer/...
content/public/renderer/p2p_socket_client.h:27: static P2PSocketClient*
CreateP2PSocket(
On 2013/11/27 21:30:18, jam wrote:
> nit: just Create, the "P2PSocket" in the name is redundant (i.e.
> P2PSocketClient::CreateP2PSocket has 2 P2PSocket)

Done.

https://codereview.chromium.org/45183002/diff/440001/content/renderer/p2p/soc...
File content/renderer/p2p/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/440001/content/renderer/p2p/soc...
content/renderer/p2p/socket_client.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/11/27 21:30:18, jam wrote:
> nit: I just noticed that this file should be renamed to socket_client_impl.h
> now, same for the cc file

Done.

[jam, 2013-11-28 00:08:56]

lgtm

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
File content/public/renderer/socket_client.h (right):

https://codereview.chromium.org/45183002/diff/140001/content/public/renderer/...
content/public/renderer/socket_client.h:21: // thread which is specified in
Init().
On 2013/11/27 23:23:01, hubbe wrote:
> On 2013/11/27 21:30:18, jam wrote:
> > On 2013/11/25 19:15:07, hubbe wrote:
> > > On 2013/11/23 02:03:14, jam wrote:
> > > > On 2013/11/21 22:07:30, hubbe wrote:
> > > > > On 2013/11/20 23:26:24, jam wrote:
> > > > > > you're describing the implementation details. do we need to expose
> that
> > to
> > > > the
> > > > > > embedder?
> > > > > > 
> > > > > 
> > > > > Fixed.
> > > > > 
> > > > > > Same for the ref counted base class and the threading. can we hide
> that
> > > and
> > > > > > expose only a single threaded interface?
> > > > > 
> > > > > I'm not sure what the proper way to handle this is actually. The
> > > > implementation
> > > > > uses lots of callbacks with ref-counts to prevent the object from
going
> > away
> > > > > prematurely. I could potentially change the callbacks to use weak
> > pointers,
> > > > but
> > > > > that seems difficult since the callbacks cross threads.
> > > > > 
> > > > 
> > > > What I mean is that this is an interface. The implementation could be
ref
> > > > counted, but does that have to be exposed on the interface?
> > > 
> > > I think so, because we expect the callers to use
> > scoped_refptr<P2PSocketClient>
> > > to manage the pointer(s) they get back from the factory method.
> > > 
> > 
> > what is the ownership here? i.e. can the embedder layer just hold on to a
raw
> > pointer?
> 
> That's not the intent, the intent is for the user to always use a
scoped_refptr.
> I changed the factory to return one.

ok, so you're saying that the ownership of this class belongs to the creator? if
so, lgtm

[commit-bot: I haz the power, 2013-11-28 08:39:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/45183002/460001

[commit-bot: I haz the power, 2013-11-28 09:23:02]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-12-03 18:46:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/45183002/480001

[commit-bot: I haz the power, 2013-12-03 19:08:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/45183002/480001

[commit-bot: I haz the power, 2013-12-03 23:33:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/45183002/480001

[commit-bot: I haz the power, 2013-12-04 04:02:32]

Change committed as 238553