Added CPP wrapper for PPB_TCPServerSocket_Private.

Added CPP wrapper for PPB_TCPServerSocket_Private.

BUG=
TEST=PPAPI(NaCl?)Test.TestTCPServerSocketPrivate


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

[brettw, 2012-03-05 18:30:50]

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
File ppapi/cpp/private/tcp_server_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:22: static bool IsAvailable();
We don't normally add this type of function for resources.

Yuzhu: do you know why we have this for The TCPSocketPrivate? Are you OK
removing it for both places?

[yzshen1, 2012-03-05 18:53:13]

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
File ppapi/cpp/private/tcp_server_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:22: static bool IsAvailable();
It is used to ease Pepper Flash developing: 
Usually, we put new features using an interface into Pepper Flash, and we want
it to still work on older browsers that don't support the interface, e.g.,
beta/stable channel. Because we don't want to maintain several Pepper Flash
branches.

We could use GetBrowserInterface() to check as well. But IsAvailable() seems
easier. The user don't need to deal with interface names directly.


On 2012/03/05 18:30:50, brettw wrote:
> We don't normally add this type of function for resources.
> 
> Yuzhu: do you know why we have this for The TCPSocketPrivate? Are you OK
> removing it for both places?

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:27: int32_t Accept(PP_Resource*
socket,
- How about returning a cpp wrapper object?
int32_t Accept(TCPSocketPrivate* socket, ...);

- Comment about how long the caller should keep this output parameter alive.

[brettw, 2012-03-05 21:30:42]

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
File ppapi/cpp/private/tcp_server_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:22: static bool IsAvailable();
I guess I'd say we shouldn't bother with this function unless we need it for
something. The way it's "supposed" to work is that the constructor just returns
an is_null() resource if it's not available. So unless we need this for a
convenience at a higher level, just skip it.

[yzshen1, 2012-03-05 22:49:26]

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
File ppapi/cpp/private/tcp_server_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:22: static bool IsAvailable();
Resource creation could fail because of other reasons than interface not
available. So it still seems different.

We have 7 wrapper classes that have IsAvailable. If we remove them, Trung will
probably be affected (when the next time he updates PPAPI in Flash). So I cc-ed
him.

On 2012/03/05 21:30:42, brettw wrote:
> I guess I'd say we shouldn't bother with this function unless we need it for
> something. The way it's "supposed" to work is that the constructor just
returns
> an is_null() resource if it's not available. So unless we need this for a
> convenience at a higher level, just skip it.

[ygorshenin1, 2012-03-06 11:37:52]

PTAL.

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
File ppapi/cpp/private/tcp_server_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:22: static bool IsAvailable();
IsAvailable is deleted.

On 2012/03/05 22:49:27, yzshen1 wrote:
> Resource creation could fail because of other reasons than interface not
> available. So it still seems different.
> 
> We have 7 wrapper classes that have IsAvailable. If we remove them, Trung will
> probably be affected (when the next time he updates PPAPI in Flash). So I
cc-ed
> him.
> 
> On 2012/03/05 21:30:42, brettw wrote:
> > I guess I'd say we shouldn't bother with this function unless we need it for
> > something. The way it's "supposed" to work is that the constructor just
> returns
> > an is_null() resource if it's not available. So unless we need this for a
> > convenience at a higher level, just skip it.
>

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:27: int32_t Accept(PP_Resource*
socket,
- This is possible, but will greatly complicate this wrapper, because in this
case wrapper'll have internal state: completion callback and pointer (or
pointer-to-pointer) to TCPSocketPrivate. Also, we should make
TCPSocketPrivate::TCPSocketPrivate(PP_Resource) private and make
TCPServerSocketPrivate a friend... I thought due to implementation complexity we
should not do that. For user it is very easy to wrap raw PP_Resource into
TCPSocketPrivate wrapper.

- What do you mean by saying "keep this output parameter alive"?

On 2012/03/05 18:53:13, yzshen1 wrote:
> - How about returning a cpp wrapper object?
> int32_t Accept(TCPSocketPrivate* socket, ...);
> 
> - Comment about how long the caller should keep this output parameter alive.

[viettrungluu, 2012-03-06 18:31:49]

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
File ppapi/cpp/private/tcp_server_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:22: static bool IsAvailable();
On 2012/03/05 21:30:42, brettw wrote:
> I guess I'd say we shouldn't bother with this function unless we need it for
> something. The way it's "supposed" to work is that the constructor just
returns
> an is_null() resource if it's not available. So unless we need this for a
> convenience at a higher level, just skip it.

I stipulate that if it's at all plausible that the browser not provide a given
interface, then the C++ wrapper should have an IsAvailable() method.

By the time you get to resource creation, it's typically very late in the game.
Plugin UI, etc. needs to reflect the lack of availability of certain features
much earlier.

[brettw, 2012-03-06 18:35:56]

How can I argue when Trung stipulates...

LGTM (I didn't really look again, I think Yuzhu can do the rest).

[yzshen1, 2012-03-06 19:08:41]

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
File ppapi/cpp/private/tcp_server_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:27: int32_t Accept(PP_Resource*
socket,
On 2012/03/06 11:37:52, ygorshenin1 wrote:
> - This is possible, but will greatly complicate this wrapper, because in this
> case wrapper'll have internal state: completion callback and pointer (or
> pointer-to-pointer) to TCPSocketPrivate. Also, we should make
> TCPSocketPrivate::TCPSocketPrivate(PP_Resource) private and make
> TCPServerSocketPrivate a friend... I thought due to implementation complexity
we
> should not do that. For user it is very easy to wrap raw PP_Resource into
> TCPSocketPrivate wrapper.

Please be noted that there is not a proper constructor of TCPSocketPrivate to
wrap this returned PP_Resource at the moment.

What we need here is a
TCPSocketPrivate(PASSREF, PP_Resource).
And there isn't one yet.

Unless you want the user to do:
- TCPSocketPrivate socket(the_output_pp_resource);
- Explicitly release the ref of the_output_resource.

It is very likely the user will do the wrong thing and leak reference.

I am fine with the current approach, if you insist. But you need the add new
constructor and have very clear comments about how to wrap the returned
resource.

> 
> - What do you mean by saying "keep this output parameter alive"?
If PP_OK_COMPLETIONPENDING is returned, the memory pointed to by this output
parameter should stay valid until
- the callback is called, or
- StopListening() is called.
Right?


> 
> On 2012/03/05 18:53:13, yzshen1 wrote:
> > - How about returning a cpp wrapper object?
> > int32_t Accept(TCPSocketPrivate* socket, ...);
> > 
> > - Comment about how long the caller should keep this output parameter alive.
>

[ygorshenin1, 2012-03-07 11:17:29]

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
File ppapi/cpp/private/tcp_server_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:22: static bool IsAvailable();
OK, I restored this method.

On 2012/03/06 18:31:49, viettrungluu wrote:
> On 2012/03/05 21:30:42, brettw wrote:
> > I guess I'd say we shouldn't bother with this function unless we need it for
> > something. The way it's "supposed" to work is that the constructor just
> returns
> > an is_null() resource if it's not available. So unless we need this for a
> > convenience at a higher level, just skip it.
> 
> I stipulate that if it's at all plausible that the browser not provide a given
> interface, then the C++ wrapper should have an IsAvailable() method.
> 
> By the time you get to resource creation, it's typically very late in the
game.
> Plugin UI, etc. needs to reflect the lack of availability of certain features
> much earlier.

http://codereview.chromium.org/9568007/diff/2001/ppapi/cpp/private/tcp_server...
ppapi/cpp/private/tcp_server_socket_private.h:27: int32_t Accept(PP_Resource*
socket,
On 2012/03/06 19:08:41, yzshen1 wrote:
> On 2012/03/06 11:37:52, ygorshenin1 wrote:
> > - This is possible, but will greatly complicate this wrapper, because in
this
> > case wrapper'll have internal state: completion callback and pointer (or
> > pointer-to-pointer) to TCPSocketPrivate. Also, we should make
> > TCPSocketPrivate::TCPSocketPrivate(PP_Resource) private and make
> > TCPServerSocketPrivate a friend... I thought due to implementation
complexity
> we
> > should not do that. For user it is very easy to wrap raw PP_Resource into
> > TCPSocketPrivate wrapper.
> 
> Please be noted that there is not a proper constructor of TCPSocketPrivate to
> wrap this returned PP_Resource at the moment.
> 
> What we need here is a
> TCPSocketPrivate(PASSREF, PP_Resource).
> And there isn't one yet.
> 
> Unless you want the user to do:
> - TCPSocketPrivate socket(the_output_pp_resource);
> - Explicitly release the ref of the_output_resource.
> 
> It is very likely the user will do the wrong thing and leak reference.
> 
> I am fine with the current approach, if you insist. But you need the add new
> constructor and have very clear comments about how to wrap the returned
> resource.
> 
> > 
> > - What do you mean by saying "keep this output parameter alive"?
> If PP_OK_COMPLETIONPENDING is returned, the memory pointed to by this output
> parameter should stay valid until
> - the callback is called, or
> - StopListening() is called.
> Right?
> 
> 
> > 
> > On 2012/03/05 18:53:13, yzshen1 wrote:
> > > - How about returning a cpp wrapper object?
> > > int32_t Accept(TCPSocketPrivate* socket, ...);
> > > 
> > > - Comment about how long the caller should keep this output parameter
alive.
> > 
> 

Thanks for explaining, Done.

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (left):

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:30: 
I'm still afraid, that if PPB_TCPSocket_Private interface isn't available,
|resource| will leak. What we should do in that situation?

[yzshen1, 2012-03-07 17:27:04]

lgtm

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (left):

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:30: 
You could release it using ReleaseResource() in ppapi/cpp/core.h, and you could
get the core interface from the global module instance.

On 2012/03/07 11:17:29, ygorshenin1 wrote:
> I'm still afraid, that if PPB_TCPSocket_Private interface isn't available,
> |resource| will leak. What we should do in that situation?

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.h:22: explicit TCPSocketPrivate(PassRef,
PP_Resource resource);
explicit is not necessary.

[ygorshenin1, 2012-03-07 17:40:40]

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (left):

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:30: 
Sure, but how to notify user about that?

On 2012/03/07 17:27:05, yzshen1 wrote:
> You could release it using ReleaseResource() in ppapi/cpp/core.h, and you
could
> get the core interface from the global module instance.
> 
> On 2012/03/07 11:17:29, ygorshenin1 wrote:
> > I'm still afraid, that if PPB_TCPSocket_Private interface isn't available,
> > |resource| will leak. What we should do in that situation?
>

[yzshen1, 2012-03-07 17:56:48]

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (left):

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:30: 
is_null() will be true.

If the user expects the resource to be a TCP socket while it isn't or the
interface is not available at all, losing the reference probably won't be too
bad.

On 2012/03/07 17:40:40, ygorshenin1 wrote:
> Sure, but how to notify user about that?
> 
> On 2012/03/07 17:27:05, yzshen1 wrote:
> > You could release it using ReleaseResource() in ppapi/cpp/core.h, and you
> could
> > get the core interface from the global module instance.
> > 
> > On 2012/03/07 11:17:29, ygorshenin1 wrote:
> > > I'm still afraid, that if PPB_TCPSocket_Private interface isn't available,
> > > |resource| will leak. What we should do in that situation?
> > 
>

[brettw, 2012-03-08 18:47:17]

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (left):

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:30: 
I'd just delete the if and call the default resource constructor for PassRef.
This is what we do in e.g. FileRef.

So far, we haven't guaranteed that if the resource type is incorrect and you
pass it to the constructor that you'll get an is_null() resource. In this case,
you'll just get every function failing and an error in the console that the
object type is incorrect, which should be obvious enough.

We don't really expect user code to do this much, these constructors are more
for the C++ wrapper itself, since there's not much reason for the plugin to make
a bare PP_Resource and then want to make an object out of it. So I'm less
concerned about error checking this case, and I'm also worried about the leaking
problem Yuzhu mentioned.

[ygorshenin1, 2012-03-11 09:39:33]

Thanks, PTAL.

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (left):

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:30: 
On 2012/03/08 18:47:17, brettw wrote:
> I'd just delete the if and call the default resource constructor for PassRef.
> This is what we do in e.g. FileRef.
> 
> So far, we haven't guaranteed that if the resource type is incorrect and you
> pass it to the constructor that you'll get an is_null() resource. In this
case,
> you'll just get every function failing and an error in the console that the
> object type is incorrect, which should be obvious enough.
> 
> We don't really expect user code to do this much, these constructors are more
> for the C++ wrapper itself, since there's not much reason for the plugin to
make
> a bare PP_Resource and then want to make an object out of it. So I'm less
> concerned about error checking this case, and I'm also worried about the
leaking
> problem Yuzhu mentioned.

Done.

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.h (right):

http://codereview.chromium.org/9568007/diff/12006/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.h:22: explicit TCPSocketPrivate(PassRef,
PP_Resource resource);
On 2012/03/07 17:27:05, yzshen1 wrote:
> explicit is not necessary.

Done.

[brettw, 2012-03-11 17:50:33]

lgtm

[yzshen1, 2012-03-12 17:01:52]

lgtm

[ygorshenin1, 2012-03-13 08:30:47]

I restored the old way to receiving local address because the new one doesn't
work on the Windows platform.

[commit-bot: I haz the power, 2012-03-13 08:31:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/9568007/32001

[commit-bot: I haz the power, 2012-03-13 09:45:15]

Change committed as 126367