Add GetBoundAddress to PPB_UDPSocket_Private

GetBoundAddress will return the address that Bind actually bound to.  If there has not been a successful call to Bind, this method will return an error.

BUG=no bug
TEST=udp tests

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

[mtilburg1, 2012-01-24 14:17:24]

Trung/Yuzhu-

A small change to the bind API that is necessary to retrieve the port we are
bound to.  Needed for UDP.

Thanks!

-mike

[yzshen1, 2012-01-27 18:41:07]

https://chromiumcodereview.appspot.com/9212047/diff/3002/ppapi/api/private/pp...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/9212047/diff/3002/ppapi/api/private/pp...
ppapi/api/private/ppb_udp_socket_private.idl:30: [in] PP_NetAddress_Private
addr,
- Isn't the port information included in |addr| itself?
Am I missing anything?

- The int32_t is a value from pp_errors.h. All non-positive values are reserved.
0 means PP_OK here.

[mtilburg1, 2012-02-01 12:43:06]

On 2012/01/27 18:41:07, yzshen1 wrote:
>
https://chromiumcodereview.appspot.com/9212047/diff/3002/ppapi/api/private/pp...
> File ppapi/api/private/ppb_udp_socket_private.idl (right):
> 
>
https://chromiumcodereview.appspot.com/9212047/diff/3002/ppapi/api/private/pp...
> ppapi/api/private/ppb_udp_socket_private.idl:30: [in] PP_NetAddress_Private
> addr,
> - Isn't the port information included in |addr| itself?
> Am I missing anything?
> 
> - The int32_t is a value from pp_errors.h. All non-positive values are
reserved.
> 0 means PP_OK here.

no, the port information is not included in the |addr| because |addr| is
assigned SetAnyAddress, so we won't know what the port is until after the bind
occurs.  |addr| is not returned to the player(nor is it needed, we just need the
port)

I need to make a call to GetLocalAddress after we call |Listen| to get this
information.  I suppose another solution would be to return the result of the
Call to |GetLocalAddress| instead of just the port, then I can pull the port out
on the player side?  That would also solve the problem of 0 being reserved as a
return value, we wouldn't have to change that...

what do you think?

[mtilburg1, 2012-02-02 10:36:56]

Added GetBoundAddress, which will be the address (and port) that Bind actually
bound to.

[yzshen1, 2012-02-03 19:30:42]

On 2012/02/01 12:43:06, mtilburg1 wrote:
> On 2012/01/27 18:41:07, yzshen1 wrote:
> >
>
https://chromiumcodereview.appspot.com/9212047/diff/3002/ppapi/api/private/pp...
> > File ppapi/api/private/ppb_udp_socket_private.idl (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9212047/diff/3002/ppapi/api/private/pp...
> > ppapi/api/private/ppb_udp_socket_private.idl:30: [in] PP_NetAddress_Private
> > addr,
> > - Isn't the port information included in |addr| itself?
> > Am I missing anything?
> > 
> > - The int32_t is a value from pp_errors.h. All non-positive values are
> reserved.
> > 0 means PP_OK here.
> 
> no, the port information is not included in the |addr| because |addr| is
> assigned SetAnyAddress, so we won't know what the port is until after the bind
> occurs.  |addr| is not returned to the player(nor is it needed, we just need
the
> port)
> 
> I need to make a call to GetLocalAddress after we call |Listen| to get this
> information.  I suppose another solution would be to return the result of the
> Call to |GetLocalAddress| instead of just the port, then I can pull the port
out
> on the player side?  That would also solve the problem of 0 being reserved as
a
> return value, we wouldn't have to change that...
> 
> what do you think?

This sounds good to me. Thanks!

[yzshen1, 2012-02-06 06:51:27]

Hi, Mike.

Sorry for the late reply. I didn't feel well last week and was not very
productive for a few days.

http://codereview.chromium.org/9212047/diff/10001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_udp_socket.cc (right):

http://codereview.chromium.org/9212047/diff/10001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_udp_socket.cc:115:
NetAddressPrivateImpl::IPEndPointToNetAddress(bound_address_, &addr);
nit: please check for failure as OnRecvFromCompleted does. And also make sure
that NetAddressPrivateImpl::kInvalidNetAddress is sent if |result| == false.

http://codereview.chromium.org/9212047/diff/10001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9212047/diff/10001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2040: const
PP_NetAddress_Private& bound_addr) {
Please use the same name as the declaration.

http://codereview.chromium.org/9212047/diff/10001/ppapi/api/private/ppb_udp_s...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

http://codereview.chromium.org/9212047/diff/10001/ppapi/api/private/ppb_udp_s...
ppapi/api/private/ppb_udp_socket_private.idl:31: * bound to is stored in |addr|
Nit: This comment is inaccurate. The address is not stored in |addr| upon
successful completion of Bind. It should be something like: "Returns the address
that the socket has bound to. It won't succeed unless Bind has completed
successfully."

http://codereview.chromium.org/9212047/diff/10001/ppapi/api/private/ppb_udp_s...
ppapi/api/private/ppb_udp_socket_private.idl:33: PP_Bool GetBoundAddress([in]
PP_Resource udp_socket,
You need to consider backward compatibility. This method should be in a newer
interface, say, 0.3. It has all methods from the older version, plus the new
one.

And you need to return 0.2 or 0.3 interface according to what the plugin asks
for.

http://codereview.chromium.org/9307115/ is an example of adding new methods
while maintaining backward compatibility.

http://codereview.chromium.org/9212047/diff/10001/ppapi/native_client/src/sha...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc
(right):

http://codereview.chromium.org/9212047/diff/10001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:94:
nacl_abi_size_t addr_bytes;
You have to init this to the size of |addr| right?

[mtilburg1, 2012-02-06 15:11:34]

Fixed comments from Yuzhu-  I should note, that this compiles and links, however
after my latest sync of chromium, I'm getting an unrelated crash on startup, so
I have not tested these changes yet...  The crash on startup occurs in the
compositor before the browser even loads...

https://chromiumcodereview.appspot.com/9212047/diff/10001/content/browser/ren...
File content/browser/renderer_host/pepper_udp_socket.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/10001/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.cc:115:
NetAddressPrivateImpl::IPEndPointToNetAddress(bound_address_, &addr);
On 2012/02/06 06:51:27, yzshen1 wrote:
> nit: please check for failure as OnRecvFromCompleted does. And also make sure
> that NetAddressPrivateImpl::kInvalidNetAddress is sent if |result| == false.

Done.

https://chromiumcodereview.appspot.com/9212047/diff/10001/content/renderer/pe...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/10001/content/renderer/pe...
content/renderer/pepper_plugin_delegate_impl.cc:2040: const
PP_NetAddress_Private& bound_addr) {
On 2012/02/06 06:51:27, yzshen1 wrote:
> Please use the same name as the declaration.

Done.

https://chromiumcodereview.appspot.com/9212047/diff/10001/ppapi/api/private/p...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/9212047/diff/10001/ppapi/api/private/p...
ppapi/api/private/ppb_udp_socket_private.idl:31: * bound to is stored in |addr|
On 2012/02/06 06:51:27, yzshen1 wrote:
> Nit: This comment is inaccurate. The address is not stored in |addr| upon
> successful completion of Bind. It should be something like: "Returns the
address
> that the socket has bound to. It won't succeed unless Bind has completed
> successfully."

Done.

https://chromiumcodereview.appspot.com/9212047/diff/10001/ppapi/api/private/p...
ppapi/api/private/ppb_udp_socket_private.idl:33: PP_Bool GetBoundAddress([in]
PP_Resource udp_socket,
I added M18 = 0.3.... is this correct?  When I added M17 = 0.3, generating the
header file did not have the same effect(only version 0.3 was generated, no
0.2), but I'm not M18 is correct either???
On 2012/02/06 06:51:27, yzshen1 wrote:
> You need to consider backward compatibility. This method should be in a newer
> interface, say, 0.3. It has all methods from the older version, plus the new
> one.
> 
> And you need to return 0.2 or 0.3 interface according to what the plugin asks
> for.
> 
> http://codereview.chromium.org/9307115/ is an example of adding new methods
> while maintaining backward compatibility.

https://chromiumcodereview.appspot.com/9212047/diff/10001/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc
(right):

https://chromiumcodereview.appspot.com/9212047/diff/10001/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:94:
nacl_abi_size_t addr_bytes;
On 2012/02/06 06:51:27, yzshen1 wrote:
> You have to init this to the size of |addr| right?

Done.

[yzshen1, 2012-02-06 18:12:50]

Please also add some tests for the newly added method, if possible.

Thanks!

https://chromiumcodereview.appspot.com/9212047/diff/10001/ppapi/api/private/p...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/9212047/diff/10001/ppapi/api/private/p...
ppapi/api/private/ppb_udp_socket_private.idl:33: PP_Bool GetBoundAddress([in]
PP_Resource udp_socket,
On 2012/02/06 15:11:35, mtilburg1 wrote:
> I added M18 = 0.3.... is this correct?  When I added M17 = 0.3, generating the
> header file did not have the same effect(only version 0.3 was generated, no
> 0.2), but I'm not M18 is correct either???
> On 2012/02/06 06:51:27, yzshen1 wrote:
> > You need to consider backward compatibility. This method should be in a
newer
> > interface, say, 0.3. It has all methods from the older version, plus the new
> > one.
> > 
> > And you need to return 0.2 or 0.3 interface according to what the plugin
asks
> > for.
> > 
> > http://codereview.chromium.org/9307115/ is an example of adding new methods
> > while maintaining backward compatibility.
> 

Please use M19.

https://chromiumcodereview.appspot.com/9212047/diff/9003/ppapi/native_client/...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc
(right):

https://chromiumcodereview.appspot.com/9212047/diff/9003/ppapi/native_client/...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:223:
const PPB_UDPSocket_Private* PluginUDPSocketPrivate::GetInterface() {
You need to add multiple interface support for NaCl as well.

(And also change ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc)

https://chromiumcodereview.appspot.com/9212047/diff/9003/ppapi/thunk/ppb_udp_...
File ppapi/thunk/ppb_udp_socket_private_thunk.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/9003/ppapi/thunk/ppb_udp_...
ppapi/thunk/ppb_udp_socket_private_thunk.cc:43:
EnterResource<PPB_UDPSocket_Private_API> enter(udp_socket, true);
Please be consistent with others, use EnterUDP.

[mtilburg1, 2012-02-07 12:48:53]

* Added ppapi version support to nacl
* added unit test for GetBoundAddress
* fixed existing unit tests for ppapi private udp... (most of these tests were
not getting executed)

https://chromiumcodereview.appspot.com/9212047/diff/9003/ppapi/native_client/...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc
(right):

https://chromiumcodereview.appspot.com/9212047/diff/9003/ppapi/native_client/...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:223:
const PPB_UDPSocket_Private* PluginUDPSocketPrivate::GetInterface() {
On 2012/02/06 18:12:50, yzshen1 wrote:
> You need to add multiple interface support for NaCl as well.
> 
> (And also change ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc)

Done.

https://chromiumcodereview.appspot.com/9212047/diff/9003/ppapi/thunk/ppb_udp_...
File ppapi/thunk/ppb_udp_socket_private_thunk.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/9003/ppapi/thunk/ppb_udp_...
ppapi/thunk/ppb_udp_socket_private_thunk.cc:43:
EnterResource<PPB_UDPSocket_Private_API> enter(udp_socket, true);
On 2012/02/06 18:12:50, yzshen1 wrote:
> Please be consistent with others, use EnterUDP.

Done.

[yzshen1, 2012-02-07 18:14:58]

Mostly look good. Thanks!

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc:96:
PluginUDPSocketPrivate::GetInterface_0_2(), true },
Please be consistent: GetInterface0_2. (And also line 98.)

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc
(right):

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:231:
Close,
Nit: please remove the trailing ','. (And also line 245)

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/tests/test_ud...
File ppapi/tests/test_udp_socket_private_shared.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/tests/test_ud...
ppapi/tests/test_udp_socket_private_shared.cc:98: if(
!udp_socket_private_interface_->GetBoundAddress(*socket, address))
Space before '(' but not after:
"if (!..."

[mtilburg1, 2012-02-07 22:08:14]

fixes from last review

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc:96:
PluginUDPSocketPrivate::GetInterface_0_2(), true },
On 2012/02/07 18:14:59, yzshen1 wrote:
> Please be consistent: GetInterface0_2. (And also line 98.)

Done.

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc
(right):

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:231:
Close,
On 2012/02/07 18:14:59, yzshen1 wrote:
> Nit: please remove the trailing ','. (And also line 245)

Done.

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/tests/test_ud...
File ppapi/tests/test_udp_socket_private_shared.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/24001/ppapi/tests/test_ud...
ppapi/tests/test_udp_socket_private_shared.cc:98: if(
!udp_socket_private_interface_->GetBoundAddress(*socket, address))
On 2012/02/07 18:14:59, yzshen1 wrote:
> Space before '(' but not after:
> "if (!..."

Done.

[yzshen1, 2012-02-07 22:13:40]

lgtm

[yzshen1, 2012-02-07 22:18:53]

On 2012/02/07 22:13:40, yzshen1 wrote:
> lgtm

You also need owner approval before you can submit this change.
Please see the OWNERS files to find out who are the owners, and send requests to
them. For each file that you touched, you need at least one owner's approval.

[mtilburg1, 2012-02-07 22:19:10]

Thanks Yuzhu!

Do we need to have Trung look at this too... or is it good to submit?

-mike

On Feb 7, 2012, at 11:13 PM, <yzshen@chromium.org> wrote:

> lgtm
> 
> 
> 
> https://chromiumcodereview.appspot.com/9212047/

[yzshen1, 2012-02-07 22:22:11]

On 2012/02/07 22:19:10, mtilburg1 wrote:
> Thanks Yuzhu!
> 
> Do we need to have Trung look at this too... or is it good to submit?
You could ping Trung. And as I said, you need to get owners' approval.


> 
> -mike
> 
> On Feb 7, 2012, at 11:13 PM, <mailto:yzshen@chromium.org> wrote:
> 
> > lgtm
> > 
> > 
> > 
> > https://chromiumcodereview.appspot.com/9212047/
>

[mtilburg1, 2012-02-07 22:38:45]

Adding Brett Wilson to reviewers, needed for OWNER approval...

[viettrungluu, 2012-02-07 23:03:58]

Someone qualified should also take a look at the nacl bits.

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
File content/browser/renderer_host/pepper_udp_socket.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.cc:52: // If we are successful,
get the local address we bound to.
Nit: This comment is useless; it's self-evident from the code.

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.cc:53: if (result == net::OK) {
Nit: Other single-line if statements in the file don't have braces. Be
consistent.

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.cc:115: if
(!NetAddressPrivateImpl::IPEndPointToNetAddress(bound_address_, &addr))
Shouldn't you do some other cleanup here, rather than just setting the result to
false?

In any case, this seems like a very strange place to do this. Shouldn't you do
this from where SendBindACK is called? And I'm guessing you shouldn't even try
to do IPEndPointToNetAddress() in the failure case, no?

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
File content/browser/renderer_host/pepper_udp_socket.h (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012, here and elsewhere. I don't actually care, but the presubmit check will
block on this.

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/api/private/p...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/api/private/p...
ppapi/api/private/ppb_udp_socket_private.idl:31: /* Returns the address that the
socket has bound to.  If there has
This comment doesn't actually make sense (especially the sentence beginning with
"If").

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/shared_impl/p...
File ppapi/shared_impl/private/udp_socket_private_impl.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/shared_impl/p...
ppapi/shared_impl/private/udp_socket_private_impl.cc:65: *addr = bound_addr_;
This doesn't actually satisfy the contract given in the API, and will
potentially return a garbage or stale address.

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/tests/test_ud...
File ppapi/tests/test_udp_socket_private_shared.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/tests/test_ud...
ppapi/tests/test_udp_socket_private_shared.cc:98: if
(!udp_socket_private_interface_->GetBoundAddress(*socket, address))
Please add tests that GetBoundAddress correctly *fails* in the case that it
hasn't been bound, etc.

[mtilburg1, 2012-02-08 15:27:00]

added a few more unit tests, changed SendBindACK to be more consistant with
OnRecvFromCompleted/SendRecvFromACKError.

changed copyright dates to 2012 in files touched for the first time this year.

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
File content/browser/renderer_host/pepper_udp_socket.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.cc:52: // If we are successful,
get the local address we bound to.
On 2012/02/07 23:03:58, viettrungluu wrote:
> Nit: This comment is useless; it's self-evident from the code.

Done.

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.cc:53: if (result == net::OK) {
On 2012/02/07 23:03:58, viettrungluu wrote:
> Nit: Other single-line if statements in the file don't have braces. Be
> consistent.

Done.

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.cc:115: if
(!NetAddressPrivateImpl::IPEndPointToNetAddress(bound_address_, &addr))
On 2012/02/07 23:03:58, viettrungluu wrote:
> Shouldn't you do some other cleanup here, rather than just setting the result
to
> false?
> 
> In any case, this seems like a very strange place to do this. Shouldn't you do
> this from where SendBindACK is called? And I'm guessing you shouldn't even try
> to do IPEndPointToNetAddress() in the failure case, no?

Changed to be consistant with GetRecvFromACKError...

Done.

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
File content/browser/renderer_host/pepper_udp_socket.h (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/content/browser/ren...
content/browser/renderer_host/pepper_udp_socket.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/07 23:03:58, viettrungluu wrote:
> 2012, here and elsewhere. I don't actually care, but the presubmit check will
> block on this.

Done.

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/api/private/p...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/api/private/p...
ppapi/api/private/ppb_udp_socket_private.idl:31: /* Returns the address that the
socket has bound to.  If there has
On 2012/02/07 23:03:58, viettrungluu wrote:
> This comment doesn't actually make sense (especially the sentence beginning
with
> "If").

Done.

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/shared_impl/p...
File ppapi/shared_impl/private/udp_socket_private_impl.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/shared_impl/p...
ppapi/shared_impl/private/udp_socket_private_impl.cc:65: *addr = bound_addr_;
On 2012/02/07 23:03:58, viettrungluu wrote:
> This doesn't actually satisfy the contract given in the API, and will
> potentially return a garbage or stale address.

Done.

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/tests/test_ud...
File ppapi/tests/test_udp_socket_private_shared.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/31003/ppapi/tests/test_ud...
ppapi/tests/test_udp_socket_private_shared.cc:98: if
(!udp_socket_private_interface_->GetBoundAddress(*socket, address))
On 2012/02/07 23:03:58, viettrungluu wrote:
> Please add tests that GetBoundAddress correctly *fails* in the case that it
> hasn't been bound, etc.

Done.

[dmichael (off chromium), 2012-02-08 16:05:20]

ppapi/* LGTM

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/c/private/ppb...
File ppapi/c/private/ppb_udp_socket_private.h (right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/c/private/ppb...
ppapi/c/private/ppb_udp_socket_private.h:47: * call to Bind must be called
first. Returns false if Bind fails,
nit: false->PP_FALSE

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc
(right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:108:
reinterpret_cast<PP_NetAddress_Private*>(addr));
nit: static_cast

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:111:
"success=%d\n", *success);
You should use NACL_PRId32 for int32_t, not %d.

[brettw, 2012-02-09 00:19:26]

high level LGTM

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/api/private/p...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/api/private/p...
ppapi/api/private/ppb_udp_socket_private.idl:32: * call to Bind must be called
first. Returns false if Bind fails,
false -> PP_FALSE

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc:2761: NaClSrpcError
PpbUDPSocketPrivateRpcClient::PPB_UDPSocket_Private_GetBoundAddress(
80 cols, I'd break this after the param, and don't indent the second line.

[mtilburg1, 2012-02-09 11:08:06]

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/api/private/p...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/api/private/p...
ppapi/api/private/ppb_udp_socket_private.idl:32: * call to Bind must be called
first. Returns false if Bind fails,
On 2012/02/09 00:19:26, brettw wrote:
> false -> PP_FALSE

Done.

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/c/private/ppb...
File ppapi/c/private/ppb_udp_socket_private.h (right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/c/private/ppb...
ppapi/c/private/ppb_udp_socket_private.h:47: * call to Bind must be called
first. Returns false if Bind fails,
On 2012/02/08 16:05:20, dmichael wrote:
> nit: false->PP_FALSE

Done.

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc
(right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:108:
reinterpret_cast<PP_NetAddress_Private*>(addr));
On 2012/02/08 16:05:20, dmichael wrote:
> nit: static_cast

Michael- I'm not sure this is correct, PP_NetAddress_Private is a struct that
has an int and a char[].  plus, when I change to static cast, we do not compile.

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:111:
"success=%d\n", *success);
On 2012/02/08 16:05:20, dmichael wrote:
> You should use NACL_PRId32 for int32_t, not %d.

Done.

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc (right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc:2761: NaClSrpcError
PpbUDPSocketPrivateRpcClient::PPB_UDPSocket_Private_GetBoundAddress(
On 2012/02/09 00:19:26, brettw wrote:
> 80 cols, I'd break this after the param, and don't indent the second line.

Brett, this file is generated, I cannot edit it directly....

[dmichael (off chromium), 2012-02-09 16:31:10]

lgtm

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc
(right):

https://chromiumcodereview.appspot.com/9212047/diff/32011/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:108:
reinterpret_cast<PP_NetAddress_Private*>(addr));
On 2012/02/09 11:08:06, mtilburg1 wrote:
> On 2012/02/08 16:05:20, dmichael wrote:
> > nit: static_cast
> 
> Michael- I'm not sure this is correct, PP_NetAddress_Private is a struct that
> has an int and a char[].  plus, when I change to static cast, we do not
compile.
You're right, I was assuming addr was void* without looking. Because it's char*,
you need reinterpret_cast.

(passing a stack-allocated struct and then memcpying from it would be safer to
guarantee correct alignment. But we have these reinterpret_casts all over the
place in the NaCl proxy, so SRPC already has to align it conservatively. What
you have is good.)

[viettrungluu, 2012-02-16 18:47:10]

http://codereview.chromium.org/9212047/diff/46001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_udp_socket.cc (right):

http://codereview.chromium.org/9212047/diff/46001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_udp_socket.cc:51: 
Probably you want to reset the socket in the failure cases.

http://codereview.chromium.org/9212047/diff/46001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_udp_socket.cc:52: if (result == net::OK &&
(See comment below. I guess you'll need to modify SendBindACK() to take a
PP_NetAddress_Private.)

Do something like:

scoped_ptr<WHATEVER socket_ points to> socket;
PP_NetAddress_Private bound_netaddr = NetAddressPrivateImpl::kInvalidNetAddress;

... replacing socket_ with socket ...

bool success =
    result != net::OK ||
    socket_->GetLockAddress(&bound_address_) != net::OK ||
    NetAddressPrivateImpl::IPEndPointToNetAddress(bound_address_,
bound_netaddr));
if (success)
  socket_.reset(socket.release());
SendBindACK(success, bound_netaddr);


Maybe you need to do something more in the case that IPEndPointToNetAddress()
failed (but Listen() didn't). Or, more likely, that really shouldn't happen, and
you should just CHECK/DCHECK that it succeeded (in which case you can put it in
SendBindACK() again, and SendBindACK() need only take a bool).

http://codereview.chromium.org/9212047/diff/46001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_udp_socket.cc:114: void
PepperUDPSocket::SendBindACKError() {
This is now needlessly redundant and convoluted. I think you should stick with
the old |SendBindACK()|, and do the additional |IPEndPointToNetAddress()| in the
body of |Bind()|.

[mtilburg1, 2012-02-17 12:14:23]

http://codereview.chromium.org/9212047/diff/46001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_udp_socket.cc (right):

http://codereview.chromium.org/9212047/diff/46001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_udp_socket.cc:51: 
On 2012/02/16 18:47:10, viettrungluu wrote:
> Probably you want to reset the socket in the failure cases.

Trung - I'm not sure this is necessary, since the bind has not succeeded, no
other operations will be allowed to occur.  The next time that Bind is called,
it will be reset.

http://codereview.chromium.org/9212047/diff/46001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_udp_socket.cc:52: if (result == net::OK &&
On 2012/02/16 18:47:10, viettrungluu wrote:
> (See comment below. I guess you'll need to modify SendBindACK() to take a
> PP_NetAddress_Private.)
> 
> Do something like:
> 
> scoped_ptr<WHATEVER socket_ points to> socket;
> PP_NetAddress_Private bound_netaddr =
NetAddressPrivateImpl::kInvalidNetAddress;
> 
> ... replacing socket_ with socket ...
> 
> bool success =
>     result != net::OK ||
>     socket_->GetLockAddress(&bound_address_) != net::OK ||
>     NetAddressPrivateImpl::IPEndPointToNetAddress(bound_address_,
> bound_netaddr));
> if (success)
>   socket_.reset(socket.release());
> SendBindACK(success, bound_netaddr);
> 
> 
> Maybe you need to do something more in the case that IPEndPointToNetAddress()
> failed (but Listen() didn't). Or, more likely, that really shouldn't happen,
and
> you should just CHECK/DCHECK that it succeeded (in which case you can put it
in
> SendBindACK() again, and SendBindACK() need only take a bool).

Trung-

While this method will also work, I don't agree that this is any better than
what I have.  See my response to the comment below

http://codereview.chromium.org/9212047/diff/46001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_udp_socket.cc:114: void
PepperUDPSocket::SendBindACKError() {
On 2012/02/16 18:47:10, viettrungluu wrote:
> This is now needlessly redundant and convoluted. I think you should stick with
> the old |SendBindACK()|, and do the additional |IPEndPointToNetAddress()| in
the
> body of |Bind()|.

I don't agree... we need an Error case (as we have for RecvFrom) because in the
error case we need to make sure that we don't send a garbage addr back(as you
pointed out in the last review)