Introduce PPB_UDPSocket_Dev.

Introduce PPB_UDPSocket_Dev.

This change exposes the PPB_UDPSocket_Dev interface and makes it to share the same backend as PPB_UDPSocket_Private.

It doesn't include:
- apps permission check;
- UDP socket options that PPB_UDPSocket_Private doesn't support.
These will be implemented in separate CLs.

BUG=247225
TEST=newly added test_udp_socket.{h,cc}.

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

[yzshen1, 2013-06-09 18:04:55]

Hi, Bill.

Would you please take a look?

I split most of the udp_socket_private_resource logic into udp_socket_shared,
and used it from udp_socket_resource with some in/out parameter conversion. I
aslo added test_udp_socket to test the new interface. 

Other code is mostly boilerplate.

Thanks!

[bbudge, 2013-06-10 12:59:56]

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
File ppapi/api/dev/ppb_udp_socket_dev.idl (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
ppapi/api/dev/ppb_udp_socket_dev.idl:18: // Allows the socket to share the local
address to which socket will
s/to which socket/to which it/

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
ppapi/api/dev/ppb_udp_socket_dev.idl:22: // Allows sending and receiving packets
sent to and from broadcast addresses.
s/sent//

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
ppapi/api/dev/ppb_udp_socket_dev.idl:58: * Binds to the address given by |addr|.
'addr' is a NetAddress?

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
ppapi/api/dev/ppb_udp_socket_dev.idl:67: * been called.
'addr' is a NetAddress? Here and below. Perhaps a comment about the interface in
general above?

https://codereview.chromium.org/16282005/diff/1/ppapi/cpp/dev/udp_socket_dev.h
File ppapi/cpp/dev/udp_socket_dev.h (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/cpp/dev/udp_socket_dev....
ppapi/cpp/dev/udp_socket_dev.h:41: PP_Resource* addr,
It would be nice if this could be NetAddress_Dev*. Or perhaps
CompletionCallbackWithOutput could be used.

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared.cc
File ppapi/proxy/udp_socket_shared.cc (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared...
ppapi/proxy/udp_socket_shared.cc:192: if
(!TrackedCallback::IsPending(recvfrom_callback_) || !read_buffer_) {
Is the '!read_buffer' clause really needed? You check for it in RecvFromImpl.

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared.h
File ppapi/proxy/udp_socket_shared.h (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared...
ppapi/proxy/udp_socket_shared.h:24: class PPAPI_PROXY_EXPORT UDPSocketShared:
public PluginResource {
Naming: 'Shared' is slightly confusing, since we also use it to mean 'used on
both sides of proxy'. Maybe a better name would be UDPSocketResourceBase.

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared...
ppapi/proxy/udp_socket_shared.h:26: UDPSocketShared(Connection connection,
PP_Instance instance);
Is it ever OK to instantiate this class? If not, maybe make ctor protected.

https://codereview.chromium.org/16282005/diff/1/ppapi/tests/test_udp_socket.cc
File ppapi/tests/test_udp_socket.cc (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/tests/test_udp_socket.c...
ppapi/tests/test_udp_socket.cc:77: return tcp_socket_is_available &&
It would look nicer if tcp_socket_is_available was on the next line.

[yzshen1, 2013-06-10 22:10:34]

Thanks a lot! Bill. Please take another look.

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
File ppapi/api/dev/ppb_udp_socket_dev.idl (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
ppapi/api/dev/ppb_udp_socket_dev.idl:18: // Allows the socket to share the local
address to which socket will
On 2013/06/10 12:59:56, bbudge1 wrote:
> s/to which socket/to which it/

Done.

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
ppapi/api/dev/ppb_udp_socket_dev.idl:22: // Allows sending and receiving packets
sent to and from broadcast addresses.
On 2013/06/10 12:59:56, bbudge1 wrote:
> s/sent//

Done.

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
ppapi/api/dev/ppb_udp_socket_dev.idl:58: * Binds to the address given by |addr|.
On 2013/06/10 12:59:56, bbudge1 wrote:
> 'addr' is a NetAddress?

Done.

https://codereview.chromium.org/16282005/diff/1/ppapi/api/dev/ppb_udp_socket_...
ppapi/api/dev/ppb_udp_socket_dev.idl:67: * been called.
On 2013/06/10 12:59:56, bbudge1 wrote:
> 'addr' is a NetAddress? Here and below. Perhaps a comment about the interface
in
> general above?

Done. I will also tidy up the document to use tags such as @param and @return in
a later CL. I have added a TODO.

https://codereview.chromium.org/16282005/diff/1/ppapi/cpp/dev/udp_socket_dev.h
File ppapi/cpp/dev/udp_socket_dev.h (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/cpp/dev/udp_socket_dev....
ppapi/cpp/dev/udp_socket_dev.h:41: PP_Resource* addr,
The problem is that NetAddress_Dev doesn't have a way to expose its member
pp_resource_ as an output parameter.

Using CompletionCallbackWithOutput seems a little bit confusing as well. Because
|buffer| is also an output parameter. Passing two output parameters differently
is a little bit confusing.

I added some comments about this parameter. What do you think?

On 2013/06/10 12:59:56, bbudge1 wrote:
> It would be nice if this could be NetAddress_Dev*. Or perhaps
> CompletionCallbackWithOutput could be used.

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared.cc
File ppapi/proxy/udp_socket_shared.cc (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared...
ppapi/proxy/udp_socket_shared.cc:192: if
(!TrackedCallback::IsPending(recvfrom_callback_) || !read_buffer_) {
On 2013/06/10 12:59:56, bbudge1 wrote:
> Is the '!read_buffer' clause really needed? You check for it in RecvFromImpl.

Done.

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared.h
File ppapi/proxy/udp_socket_shared.h (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared...
ppapi/proxy/udp_socket_shared.h:24: class PPAPI_PROXY_EXPORT UDPSocketShared:
public PluginResource {
Thanks! It sounds much better to be UDPSocketResourceBase.

On 2013/06/10 12:59:56, bbudge1 wrote:
> Naming: 'Shared' is slightly confusing, since we also use it to mean 'used on
> both sides of proxy'. Maybe a better name would be UDPSocketResourceBase.

https://codereview.chromium.org/16282005/diff/1/ppapi/proxy/udp_socket_shared...
ppapi/proxy/udp_socket_shared.h:26: UDPSocketShared(Connection connection,
PP_Instance instance);
Done.

On 2013/06/10 12:59:56, bbudge1 wrote:
> Is it ever OK to instantiate this class? If not, maybe make ctor protected.

https://codereview.chromium.org/16282005/diff/1/ppapi/tests/test_udp_socket.cc
File ppapi/tests/test_udp_socket.cc (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/tests/test_udp_socket.c...
ppapi/tests/test_udp_socket.cc:77: return tcp_socket_is_available &&
I thought this way conforms to the style guide?
Putting tcp_socket_is_available on the next line takes up more space and seems a
little unusual.

On 2013/06/10 12:59:56, bbudge1 wrote:
> It would look nicer if tcp_socket_is_available was on the next line.

[yzshen1, 2013-06-10 23:52:42]

Hi, David.

Do you have any better idea about how to do this? Thanks!
In udp_socket_dev.h, we have the following:

// |addr| will be set to a PPB_NetAddress_Dev resource when the method
// completes successfully. It has been added ref on behalf of the caller, so
// it could be passed into a pp::NetAddress_Dev instance by using the
// constructor with a PassRef parameter.
int32_t RecvFrom(char* buffer,
                 int32_t num_bytes,
                 PP_Resource* addr,
                 const CompletionCallback& callback);

On 2013/06/10 12:59:56, bbudge1 wrote:
> It would be nice if this could be NetAddress_Dev*. Or perhaps
> CompletionCallbackWithOutput could be used.

The problem is that NetAddress_Dev doesn't have a way to get a pointer to its
member
pp_resource_ as an output parameter.

Using CompletionCallbackWithOutput seems a little bit weird. Because |buffer| is
also an output parameter, passing two output parameters differently is a little
bit confusing.

[bbudge, 2013-06-11 00:30:38]

https://codereview.chromium.org/16282005/diff/1/ppapi/tests/test_udp_socket.cc
File ppapi/tests/test_udp_socket.cc (right):

https://codereview.chromium.org/16282005/diff/1/ppapi/tests/test_udp_socket.c...
ppapi/tests/test_udp_socket.cc:77: return tcp_socket_is_available &&
OK

[yzshen1, 2013-06-11 15:12:20]

Hi, Ben.

Would you please do an OWNER review for this?
native_client_sdk/src/build_tools/sdk_files.list

Thanks!

[dmichael (off chromium), 2013-06-11 17:03:35]

https://chromiumcodereview.appspot.com/16282005/diff/1/ppapi/cpp/dev/udp_sock...
File ppapi/cpp/dev/udp_socket_dev.h (right):

https://chromiumcodereview.appspot.com/16282005/diff/1/ppapi/cpp/dev/udp_sock...
ppapi/cpp/dev/udp_socket_dev.h:41: PP_Resource* addr,
On 2013/06/10 22:10:35, yzshen1 wrote:
> The problem is that NetAddress_Dev doesn't have a way to expose its member
> pp_resource_ as an output parameter.
Can you grant friendship from NetAddress_Dev to UDPSocket_Dev to accomplish
this? I agree with Bill it seems awkward to use PP_Resource* here.

> 
> Using CompletionCallbackWithOutput seems a little bit confusing as well.
Because
> |buffer| is also an output parameter. Passing two output parameters
differently
> is a little bit confusing.
I think it might be okay; an output buffer feels a little different to me than
other output parameters. But if you can just make NetAddress_Dev work (with
friendship), that might be better.
> 
> I added some comments about this parameter. What do you think?
> 
> On 2013/06/10 12:59:56, bbudge1 wrote:
> > It would be nice if this could be NetAddress_Dev*. Or perhaps
> > CompletionCallbackWithOutput could be used.
>

[yzshen1, 2013-06-11 17:29:58]

https://chromiumcodereview.appspot.com/16282005/diff/1/ppapi/cpp/dev/udp_sock...
File ppapi/cpp/dev/udp_socket_dev.h (right):

https://chromiumcodereview.appspot.com/16282005/diff/1/ppapi/cpp/dev/udp_sock...
ppapi/cpp/dev/udp_socket_dev.h:41: PP_Resource* addr,
On 2013/06/11 17:03:35, dmichael wrote:
> On 2013/06/10 22:10:35, yzshen1 wrote:
> > The problem is that NetAddress_Dev doesn't have a way to expose its member
> > pp_resource_ as an output parameter.
> Can you grant friendship from NetAddress_Dev to UDPSocket_Dev to accomplish
> this? I agree with Bill it seems awkward to use PP_Resource* here.

The problem is that NetAddress_Dev itself doesn't have access to its
pp_resource_ member. :/

If we want to take this direction, we have to make NetAddress_Dev a friend of
pp::Resource.

What do you think?
> 
> > 
> > Using CompletionCallbackWithOutput seems a little bit confusing as well.
> Because
> > |buffer| is also an output parameter. Passing two output parameters
> differently
> > is a little bit confusing.
> I think it might be okay; an output buffer feels a little different to me than
> other output parameters. But if you can just make NetAddress_Dev work (with
> friendship), that might be better.
> > 
> > I added some comments about this parameter. What do you think?
> > 
> > On 2013/06/10 12:59:56, bbudge1 wrote:
> > > It would be nice if this could be NetAddress_Dev*. Or perhaps
> > > CompletionCallbackWithOutput could be used.
> > 
>

[dmichael (off chromium), 2013-06-11 17:43:27]

On Tue, Jun 11, 2013 at 11:29 AM, <yzshen@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/16282005/diff/1/**
>
ppapi/cpp/dev/udp_socket_dev.h<https://chromiumcodereview.appspot.com/16282005/diff/1/ppapi/cpp/dev/udp_socket_dev.h>
> File ppapi/cpp/dev/udp_socket_dev.h (right):
>
> https://chromiumcodereview.**appspot.com/16282005/diff/1/**
>
ppapi/cpp/dev/udp_socket_dev.**h#newcode41<https://chromiumcodereview.appspot.com/16282005/diff/1/ppapi/cpp/dev/udp_socket_dev.h#newcode41>
> ppapi/cpp/dev/udp_socket_dev.**h:41: PP_Resource* addr,
> On 2013/06/11 17:03:35, dmichael wrote:
>
>> On 2013/06/10 22:10:35, yzshen1 wrote:
>> > The problem is that NetAddress_Dev doesn't have a way to expose its
>>
> member
>
>> > pp_resource_ as an output parameter.
>> Can you grant friendship from NetAddress_Dev to UDPSocket_Dev to
>>
> accomplish
>
>> this? I agree with Bill it seems awkward to use PP_Resource* here.
>>
>
> The problem is that NetAddress_Dev itself doesn't have access to its
> pp_resource_ member. :/
>
> If we want to take this direction, we have to make NetAddress_Dev a
> friend of pp::Resource.
>
> What do you think?

Or make pp_resource_ protected instead of private...  a little cleaner
code-wise, but opens us up for more mistakes. I'm surprised we haven't
tried to use a Resource out-param before this!

Another option would be to make a little struct (or use pair) that holds
the char* and the NetAddress_Dev, so that you can pass it to a
CompletionCallbackWithOutput.

I'm thinking right now that my favorite is actually to use
CompletionCallbackWithOutput<NetAddress_Dev>, and let the output buffer be
used separately. It's a little akward to allow the plugin to provide a
resource that might already be ref-counted, might not be of type
NetAddress, etc. We can't really avoid the problem at the C level. But at
C++, if we use CompletionCallbackWithOutput, I think there's less confusion
about what the developer can and should provide for that out-parameter,
because they don't pass us anything. We make sure they get the right kind
of thing passed to them. As a plugin developer, I think I wouldn't be
surprised to have my output buffer treated differently from some other
result of the function.

That's my 2 cents.



>
>
>  >
>> > Using CompletionCallbackWithOutput seems a little bit confusing as
>>
> well.
>
>> Because
>> > |buffer| is also an output parameter. Passing two output parameters
>> differently
>> > is a little bit confusing.
>> I think it might be okay; an output buffer feels a little different to
>>
> me than
>
>> other output parameters. But if you can just make NetAddress_Dev work
>>
> (with
>
>> friendship), that might be better.
>> >
>> > I added some comments about this parameter. What do you think?
>> >
>> > On 2013/06/10 12:59:56, bbudge1 wrote:
>> > > It would be nice if this could be NetAddress_Dev*. Or perhaps
>> > > CompletionCallbackWithOutput could be used.
>> >
>>
>
>
>
https://chromiumcodereview.**appspot.com/16282005/<https://chromiumcodereview...
>

[yzshen1, 2013-06-11 18:44:11]

> Or make pp_resource_ protected instead of private...  a little cleaner
> code-wise, but opens us up for more mistakes. I'm surprised we haven't
> tried to use a Resource out-param before this!
> 
> Another option would be to make a little struct (or use pair) that holds
> the char* and the NetAddress_Dev, so that you can pass it to a
> CompletionCallbackWithOutput.
> 
> I'm thinking right now that my favorite is actually to use
> CompletionCallbackWithOutput<NetAddress_Dev>, and let the output buffer be
> used separately. It's a little akward to allow the plugin to provide a
> resource that might already be ref-counted, might not be of type
> NetAddress, etc. We can't really avoid the problem at the C level. But at
> C++, if we use CompletionCallbackWithOutput, I think there's less confusion
> about what the developer can and should provide for that out-parameter,
> because they don't pass us anything. We make sure they get the right kind
> of thing passed to them. As a plugin developer, I think I wouldn't be
> surprised to have my output buffer treated differently from some other
> result of the function.
> 
> That's my 2 cents.
> 

Thanks for the input! I think you and Bill both suggested to use a
CompletionCallbackWithOutput<NetAddress_Dev>. I agree that it is probably easier
for the user this way. I have made the change.

[binji, 2013-06-12 16:54:06]

native_client_sdk lgtm

[bbudge, 2013-06-12 18:52:36]

Mostly comment wording. Otherwise, this looks good.

https://codereview.chromium.org/16282005/diff/24001/ppapi/api/dev/ppb_udp_soc...
File ppapi/api/dev/ppb_udp_socket_dev.idl (right):

https://codereview.chromium.org/16282005/diff/24001/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:31: // set, the browser doesn't guarantee
to conform to it.
s/guarantee to/guarantee it will/
Here and below.

https://codereview.chromium.org/16282005/diff/24001/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:44: PP_UDPSOCKET_OPTION_RECV_TIMEOUT = 4
Remove?

https://codereview.chromium.org/16282005/diff/24001/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:86: * already called Bind). |addr| is a
PPB_NetAddress_Dev resource indicating
A little awkward - why not use the wording from RecvFrom above? i.e.
* Performs a non-blocking sendto call on socket.
* Bind must be called first. |addr| is a PPB_NetAddress_Dev resource holding
* the target address. |callback| is invoked when SendTo completes.

https://codereview.chromium.org/16282005/diff/24001/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:103: * Possible values for |name|, |value|
and |value|'s type are described in
Maybe this would be a little clearer:
* See the PP_UDPSocketOption_Dev description for option names
* value types, and allowed values.

https://codereview.chromium.org/16282005/diff/24001/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:104: * PP_UDPSocketOption_Dev description.
If no error occurs, returns PP_OK.
s/If no error occurs, returns PP_OK/Returns PP_OK on success.

https://codereview.chromium.org/16282005/diff/24001/ppapi/proxy/udp_socket_re...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://codereview.chromium.org/16282005/diff/24001/ppapi/proxy/udp_socket_re...
ppapi/proxy/udp_socket_resource_base.cc:83: msg,
Why not construct the msg inline here?

https://codereview.chromium.org/16282005/diff/24001/ppapi/proxy/udp_socket_re...
ppapi/proxy/udp_socket_resource_base.cc:118: msg,
Same as above.

https://codereview.chromium.org/16282005/diff/24001/ppapi/proxy/udp_socket_re...
ppapi/proxy/udp_socket_resource_base.cc:154: msg,
inline?

https://codereview.chromium.org/16282005/diff/24001/ppapi/proxy/udp_socket_sh...
File ppapi/proxy/udp_socket_shared.h (right):

https://codereview.chromium.org/16282005/diff/24001/ppapi/proxy/udp_socket_sh...
ppapi/proxy/udp_socket_shared.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
Should this file be removed?

https://codereview.chromium.org/16282005/diff/24001/ppapi/tests/test_udp_sock...
File ppapi/tests/test_udp_socket.cc (right):

https://codereview.chromium.org/16282005/diff/24001/ppapi/tests/test_udp_sock...
ppapi/tests/test_udp_socket.cc:27: case PP_NETADDRESS_FAMILY_UNSPECIFIED: {
Could this case be handled by the default case below?

https://codereview.chromium.org/16282005/diff/24001/ppapi/tests/test_udp_sock...
ppapi/tests/test_udp_socket.cc:244: // |first_message| also arrived to
|server2|.
s/arrived to/arrived at/
or say 'was received by'. 'Arrived to' sounds a little strange.

https://codereview.chromium.org/16282005/diff/24001/ppapi/tests/test_udp_sock...
ppapi/tests/test_udp_socket.cc:251: // |second_message| also arrived to
|server1|.
Ditto.

https://codereview.chromium.org/16282005/diff/24001/ppapi/tests/test_udp_sock...
File ppapi/tests/test_udp_socket.h (right):

https://codereview.chromium.org/16282005/diff/24001/ppapi/tests/test_udp_sock...
ppapi/tests/test_udp_socket.h:28: std::string
SetBroadcastOptions(pp::UDPSocket_Dev* socket);
s/SetBroadcastOptions/SetBroadCastOption

[yzshen1, 2013-06-12 19:32:53]

Thanks Bill! Please take another look.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/api/dev/ppb_...
File ppapi/api/dev/ppb_udp_socket_dev.idl (right):

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_udp_socket_dev.idl:31: // set, the browser doesn't guarantee
to conform to it.
On 2013/06/12 18:52:37, bbudge1 wrote:
> s/guarantee to/guarantee it will/
> Here and below.

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_udp_socket_dev.idl:44: PP_UDPSOCKET_OPTION_RECV_TIMEOUT = 4
On 2013/06/12 18:52:37, bbudge1 wrote:
> Remove?

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_udp_socket_dev.idl:86: * already called Bind). |addr| is a
PPB_NetAddress_Dev resource indicating
Done. Thanks!
On 2013/06/12 18:52:37, bbudge1 wrote:
> A little awkward - why not use the wording from RecvFrom above? i.e.
> * Performs a non-blocking sendto call on socket.
> * Bind must be called first. |addr| is a PPB_NetAddress_Dev resource holding
> * the target address. |callback| is invoked when SendTo completes.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_udp_socket_dev.idl:103: * Possible values for |name|, |value|
and |value|'s type are described in
On 2013/06/12 18:52:37, bbudge1 wrote:
> Maybe this would be a little clearer:
> * See the PP_UDPSocketOption_Dev description for option names
> * value types, and allowed values.

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_udp_socket_dev.idl:104: * PP_UDPSocketOption_Dev description.
If no error occurs, returns PP_OK.
Thanks!

Sorry for bad comments. :) Some of them were from the private API but some of
them were introduced by me.

I am planning to have a separate CL to tidy up the document (adding tags such as
@param and clarifying some more things). I will send to you for review.

On 2013/06/12 18:52:37, bbudge1 wrote:
> s/If no error occurs, returns PP_OK/Returns PP_OK on success.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/proxy/udp_so...
File ppapi/proxy/udp_socket_resource_base.cc (right):

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/proxy/udp_so...
ppapi/proxy/udp_socket_resource_base.cc:83: msg,
On 2013/06/12 18:52:37, bbudge1 wrote:
> Why not construct the msg inline here?

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/proxy/udp_so...
ppapi/proxy/udp_socket_resource_base.cc:118: msg,
On 2013/06/12 18:52:37, bbudge1 wrote:
> Same as above.

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/proxy/udp_so...
ppapi/proxy/udp_socket_resource_base.cc:154: msg,
On 2013/06/12 18:52:37, bbudge1 wrote:
> inline?

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/proxy/udp_so...
File ppapi/proxy/udp_socket_shared.h (right):

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/proxy/udp_so...
ppapi/proxy/udp_socket_shared.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
Yeah. Thanks for catching this!

On 2013/06/12 18:52:37, bbudge1 wrote:
> Should this file be removed?

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/tests/test_u...
File ppapi/tests/test_udp_socket.cc (right):

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/tests/test_u...
ppapi/tests/test_udp_socket.cc:27: case PP_NETADDRESS_FAMILY_UNSPECIFIED: {
On 2013/06/12 18:52:37, bbudge1 wrote:
> Could this case be handled by the default case below?

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/tests/test_u...
ppapi/tests/test_udp_socket.cc:244: // |first_message| also arrived to
|server2|.
On 2013/06/12 18:52:37, bbudge1 wrote:
> s/arrived to/arrived at/
> or say 'was received by'. 'Arrived to' sounds a little strange.

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/tests/test_u...
ppapi/tests/test_udp_socket.cc:251: // |second_message| also arrived to
|server1|.
On 2013/06/12 18:52:37, bbudge1 wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/tests/test_u...
File ppapi/tests/test_udp_socket.h (right):

https://chromiumcodereview.appspot.com/16282005/diff/24001/ppapi/tests/test_u...
ppapi/tests/test_udp_socket.h:28: std::string
SetBroadcastOptions(pp::UDPSocket_Dev* socket);
On 2013/06/12 18:52:37, bbudge1 wrote:
> s/SetBroadcastOptions/SetBroadCastOption

Done.

[bbudge, 2013-06-12 23:37:42]

LGTM w/nit

https://codereview.chromium.org/16282005/diff/39001/ppapi/api/dev/ppb_udp_soc...
File ppapi/api/dev/ppb_udp_socket_dev.idl (right):

https://codereview.chromium.org/16282005/diff/39001/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:63: * resource.  A successful call to Bind
must be called first. Returns 0 if
s/A successful call to Bind must be called first./Bind must be called and
succeed first./

[yzshen1, 2013-06-13 16:40:44]

Thanks Bill!

https://chromiumcodereview.appspot.com/16282005/diff/39001/ppapi/api/dev/ppb_...
File ppapi/api/dev/ppb_udp_socket_dev.idl (right):

https://chromiumcodereview.appspot.com/16282005/diff/39001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_udp_socket_dev.idl:63: * resource.  A successful call to Bind
must be called first. Returns 0 if
On 2013/06/12 23:37:42, bbudge1 wrote:
> s/A successful call to Bind must be called first./Bind must be called and
> succeed first./

Done.

[yzshen1, 2013-06-13 16:40:44]

Thanks Bill!

https://chromiumcodereview.appspot.com/16282005/diff/39001/ppapi/api/dev/ppb_...
File ppapi/api/dev/ppb_udp_socket_dev.idl (right):

https://chromiumcodereview.appspot.com/16282005/diff/39001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_udp_socket_dev.idl:63: * resource.  A successful call to Bind
must be called first. Returns 0 if
On 2013/06/12 23:37:42, bbudge1 wrote:
> s/A successful call to Bind must be called first./Bind must be called and
> succeed first./

Done.

[commit-bot: I haz the power, 2013-06-13 16:41:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/16282005/51001

[commit-bot: I haz the power, 2013-06-13 22:43:41]

Change committed as 206183