Implement PPB_UDPSocket_Dev: part 2

Implement PPB_UDPSocket_Dev: part 2

This CL:
- adds apps permission check;
- supports UDP socket options tht PPB_UDPSocket_Private doesn' support;
- map net::Error to PP_Error. 

BUG=247225
TEST=None
TBR=brettw@chromium.org
(TBR Brett for renaming two files in content/content_browser.gypi)

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

[yzshen1, 2013-06-15 23:06:46]

Hi, Bill and Tom.

Would you please take a look?

Bill: Everything.

Tom: ppapi/proxy/ppapi_messages.h 
This change renames UDP socket private messages to be used by both PPB_UDPSocket
and PPB_UDPSocket_Private, with a few modifications:
(1) have two separate create messages to create public/private resources.
(2) the SetOption message (previously called SetBoolSocketFeature) changes the
first parameter to use an enum type and the new enum validation IPC macro; the
second parameter uses a generic base::ListValue instead of a boolean.
(3) add a reply message for SetOption which simply reports whether the operation
is successful or not.

Thanks!

[bbudge, 2013-06-17 11:58:47]

Yuzhu, would it be possible to implement SetOptions on the Plugin side of the
proxy? You would then include the Options struct as part of the Bind message. I
think it would simplify things quite a bit. I realize the original
implementation did things this way, but I can't see why it has to be that way.

https://codereview.chromium.org/16959005/diff/3001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/16959005/diff/3001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1517:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_UDPSocket_SetOptionReply)
I see that you are preserving the original order of these messages. However,
everywhere else except WebSocket in this file, each message with a reply is
followed immediately by that reply definition. I think this makes it easier to
understand the message flow. Could we reorder these to follow that convention?

[yzshen1, 2013-06-17 17:01:21]

Just realized that Tom is OOO now. Adding Cris as ppapi_messages reviewer.

Cris: Would you please take a look at ppapi_messages.h?

This change renames UDP socket private messages to be used by both PPB_UDPSocket
and PPB_UDPSocket_Private, with a few modifications:
(1) have two separate create messages to create public/private resources.
(2) the SetOption message (previously called SetBoolSocketFeature) changes the
first parameter to use an enum type and the new enum validation IPC macro; the
second parameter uses a generic base::ListValue instead of a boolean.
(3) add a reply message for SetOption which simply reports whether the operation
is successful or not.

I think this change should be safe.

Thanks!

[bbudge, 2013-06-17 18:29:45]

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:290:
SendBindError(context, PP_ERROR_FAILED);
Could we use one of your new network error codes here?

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:329:
socket_.reset(socket.release());
why not socket_.swap(socket)?

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:398:
int32_t pp_result = NetErrorToPepperError(net_result);
Rather than check pp_result two times below, could you check for an error here?
ie.
if (pp_result < 0)
  SendRecvFromError(...

That would let you simplify the if's below.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:119:
scoped_ptr<int> send_buffer_size;
Why not just use a sentinel value (say -1) to indicate that no size option has
been set? I'm assuming that negative sizes are not allowed.

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

https://codereview.chromium.org/16959005/diff/13001/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:21: // Takes effect if set before Bind().
Isn't it an error to set it after Bind?

[yzshen1, 2013-06-17 21:33:37]

Thanks a lot, Bill!

Please take another look.

https://codereview.chromium.org/16959005/diff/3001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/16959005/diff/3001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1517:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_UDPSocket_SetOptionReply)
Sounds good. Done.

On 2013/06/17 11:58:47, bbudge1 wrote:
> I see that you are preserving the original order of these messages. However,
> everywhere else except WebSocket in this file, each message with a reply is
> followed immediately by that reply definition. I think this makes it easier to
> understand the message flow. Could we reorder these to follow that convention?

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:290:
SendBindError(context, PP_ERROR_FAILED);
Changed all places. Thanks!
On 2013/06/17 18:29:45, bbudge1 wrote:
> Could we use one of your new network error codes here?

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:329:
socket_.reset(socket.release());
On 2013/06/17 18:29:45, bbudge1 wrote:
> why not socket_.swap(socket)?

Done.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:398:
int32_t pp_result = NetErrorToPepperError(net_result);
I think checking pp_result twice is necessary: line 404-405 may fail and in that
case we will reset the value of |pp_result|. Have I misunderstood anything?

On 2013/06/17 18:29:45, bbudge1 wrote:
> Rather than check pp_result two times below, could you check for an error
here?
> ie.
> if (pp_result < 0)
>   SendRecvFromError(...
> 
> That would let you simplify the if's below.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:119:
scoped_ptr<int> send_buffer_size;
Currently this class doesn't check the value. It just sends the value to the
underlying socket no matter what it is, and relies on the underlying socket to
decide whether the value is valid or not.
If it is set before the underlying socket is created, we cache the value in
|send_buffer_size| and always return PP_OK, even if it is -1. The consequence of
an invalid |send_buffer_size| is that it will fail Bind() later.

We could report a failure right away for negative values before caching the
value into |send_buffer_size|. But I imagine that some positive values may not
be valid as well (but we cannot easily tell unless we pass them into the
underlying socket). Comparing with doing a 'partial' validation in our class, I
personally think the current behavior is a little bit more consistent.

Does this sound reasonable?


On 2013/06/17 18:29:45, bbudge1 wrote:
> Why not just use a sentinel value (say -1) to indicate that no size option has
> been set? I'm assuming that negative sizes are not allowed.

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

https://codereview.chromium.org/16959005/diff/13001/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:21: // Takes effect if set before Bind().
On 2013/06/17 18:29:45, bbudge1 wrote:
> Isn't it an error to set it after Bind?

Done.

[Cris Neckar, 2013-06-17 21:45:28]

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:132:
const base::Value* value = NULL;
I'm having trouble understanding why you pass this as a ListValue and then
ensure that there is only 1 of them. Can't you just pass this as a Value and
reduce complexity?

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:159: 
How does this handle the case where one side is a 32bit process and the Other
side is 64bit? Please read an explicitly sized type.

Please validate that the integer falls within a sane range for this option. ie:
int > 0, int < some sane max, etc

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:216: if
(num_bytes > ppapi::proxy::UDPSocketResourceBase::kMaxReadSize) {
What about when num_bytes is negative? I suggest making this an unsigned type.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:219:
num_bytes = ppapi::proxy::UDPSocketResourceBase::kMaxReadSize;
don't try to recover from bad IPC parameters. We should be returning false and
killing the child process when these values are bad.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:394:
int net_result) {
Cannot pass ints across IPC messages. Please change this to a size specific type

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:421:
int net_result) {
ditto on int parameter

[bbudge, 2013-06-17 21:48:07]

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:119:
scoped_ptr<int> send_buffer_size;
I think it's better to return PP_ERROR_BADARGUMENT immediately whenever you can,
rather than PP_ERROR_FAILED when Bind() is called, since it may not be obvious
to the developer what caused the error at that point. For positive values, you
have to wait until Bind, though since the API doesn't guarantee that the browser
will conform to it, you could even ignore these errors. In fact that might be
better since we might break apps by changing these limits later on.

[yzshen1, 2013-06-18 00:28:58]

Thanks, Bill and Cris!

Please take another look!

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:132:
const base::Value* value = NULL;
The reason is base::Value doesn't have a default constructor, which is needed by
our IPC layer. (I had a comment in ppapi_messages.h.)

I think other messages also use base::ListValue to get around this, for example,
ExtensionMsg_Response:
"""
// The browser sends this message in response to all extension api calls. The
// response data (if any) is one of the base::Value subclasses, wrapped as the
// first element in a ListValue.
IPC_MESSAGE_ROUTED4(ExtensionMsg_Response,
"""

On 2013/06/17 21:45:29, Cris Neckar wrote:
> I'm having trouble understanding why you pass this as a ListValue and then
> ensure that there is only 1 of them. Can't you just pass this as a Value and
> reduce complexity?

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:159: 
On 2013/06/17 21:45:29, Cris Neckar wrote:
> How does this handle the case where one side is a 32bit process and the Other
> side is 64bit? Please read an explicitly sized type.
(I am happy to make changes. Just trying to make sure I understand your
suggestion correctly.)
base::Value doesn't have a specific sized type, it has TYPE_INTEGER which is
used as above.

I thought the IPC layer should handle the serialization/deserialization
correctly on both sides. But I don't know for sure. I will read more code.

I could change from base::ListValue to a simple structure which list all
possible options.
Please let me know if there is a better way of doing this. Thanks!

> 
> Please validate that the integer falls within a sane range for this option.
ie:
> int > 0, int < some sane max, etc
Done.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:216: if
(num_bytes > ppapi::proxy::UDPSocketResourceBase::kMaxReadSize) {
On 2013/06/17 21:45:29, Cris Neckar wrote:
> What about when num_bytes is negative? I suggest making this an unsigned type.

I have checked num_bytes for the <= 0 case. Thanks for catching this.

We would like to use int32_t in the API so that it is consistent with other
existing APIs (e.g., PPB_FileIO). Do you think that is okay?

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:219:
num_bytes = ppapi::proxy::UDPSocketResourceBase::kMaxReadSize;
On 2013/06/17 21:45:29, Cris Neckar wrote:
> don't try to recover from bad IPC parameters. We should be returning false and
> killing the child process when these values are bad.

I have changed to return directly. Thanks!

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:394:
int net_result) {
On 2013/06/17 21:45:29, Cris Neckar wrote:
> Cannot pass ints across IPC messages. Please change this to a size specific
type

This is not passed across IPC directly, it is converted to PP_Error by
NetErrorToPepperError which is int32_t.

Is there any existing utility for safely converting from int to int32_t? I used
static_cast<int32_t>() in NetErrorToPepperError. :)

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:421:
int net_result) {
On 2013/06/17 21:45:29, Cris Neckar wrote:
> ditto on int parameter

Please see my comment above.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:119:
scoped_ptr<int> send_buffer_size;
I have changed the behavior according to our discussion. Thanks!
On 2013/06/17 21:48:07, bbudge1 wrote:
> I think it's better to return PP_ERROR_BADARGUMENT immediately whenever you
can,
> rather than PP_ERROR_FAILED when Bind() is called, since it may not be obvious
> to the developer what caused the error at that point. For positive values, you
> have to wait until Bind, though since the API doesn't guarantee that the
browser
> will conform to it, you could even ignore these errors. In fact that might be
> better since we might break apps by changing these limits later on.

[bbudge, 2013-06-18 01:15:08]

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:398:
int32_t pp_result = NetErrorToPepperError(net_result);
Here's what I was thinking, starting from line 398:

  int32_t pp_result = NetErrorToPepperError(net_result);
  if (pp_result < 0)
    SendRecvFromError(context, pp_result);

  // Convert IPEndPoint we get back from RecvFrom to a PP_NetAddress_Private
  // to send back.
  PP_NetAddress_Private addr = NetAddressPrivateImpl::kInvalidNetAddress;
  if (!NetAddressPrivateImpl::IPEndPointToNetAddress(
          recvfrom_address_.address(), recvfrom_address_.port(), &addr)) {
    SendRecvFromError(context, PP_ERROR_FAILED);
  }

  SendRecvFromReply(context, PP_OK,
                    std::string(recvfrom_buffer_->data(), pp_result), addr);

  recvfrom_buffer_ = NULL;

I think it's equivalent.

https://codereview.chromium.org/16959005/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:162:
bool result = true;
Should set this to false so unrecognized enum value fails.

https://codereview.chromium.org/16959005/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/16959005/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:120:
OptionData option_data_;
It seems like this struct isn't needed anymore. Do you foresee more
initialization (pre-Bind) options being added?

[bbudge, 2013-06-18 01:19:30]

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:398:
int32_t pp_result = NetErrorToPepperError(net_result);
Except that I forgot to not send multiple replies. I should have returned after
sending the replies.

[bbudge, 2013-06-18 01:46:37]

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:398:
int32_t pp_result = NetErrorToPepperError(net_result);
So maybe it isn't any simpler. Your preference.

[yzshen1, 2013-06-18 05:21:17]

Thanks, BIll! Please take another look.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:398:
int32_t pp_result = NetErrorToPepperError(net_result);
On 2013/06/18 01:19:30, bbudge1 wrote:
> Except that I forgot to not send multiple replies. I should have returned
after
> sending the replies.

Right. We shouldn't send multiple replies. Besides, we also need to clear
|recvfrom_buffer_| before exiting the method.

So if you are okay, I will leave the code as it is.
Thanks!

https://codereview.chromium.org/16959005/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:162:
bool result = true;
I have changed to false, although here it is guaranteed to be either
PP_UDPSOCKET_OPTION_SEND_BUFFER_SIZE or PP_UDPSOCKET_OPTION_RECV_BUFFER_SIZE
(because line 152 and 153).

On 2013/06/18 01:15:09, bbudge1 wrote:
> Should set this to false so unrecognized enum value fails.

https://codereview.chromium.org/16959005/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/16959005/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:120:
OptionData option_data_;
On 2013/06/18 01:15:09, bbudge1 wrote:
> It seems like this struct isn't needed anymore. Do you foresee more
> initialization (pre-Bind) options being added?

Done.

[yzshen1, 2013-06-18 07:33:29]

Hi, Cris.

Would you please take another look? Thanks!

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:159: 
> > How does this handle the case where one side is a 32bit process and the
Other
> > side is 64bit? Please read an explicitly sized type.

Hi, Cris. I changed from base::ListValue to a new structure
ppapi::SocketOptionData which explicitly use int32_t.
Please let me know whether this looks better.

https://codereview.chromium.org/16959005/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:394:
int net_result) {
On 2013/06/18 00:28:58, yzshen1 wrote:
> On 2013/06/17 21:45:29, Cris Neckar wrote:
> > Cannot pass ints across IPC messages. Please change this to a size specific
> type
> 
> This is not passed across IPC directly, it is converted to PP_Error by
> NetErrorToPepperError which is int32_t.
> 
> Is there any existing utility for safely converting from int to int32_t? I
used
> static_cast<int32_t>() in NetErrorToPepperError. :)

In NetErrorToPepperError, I added a CHECK if we cannot convert an int value into
int32_t. (I considered to return a bool to indicate a conversion failure, but
decided that the resulting method signature is too hard to use. I would rather
CHECK which really shouldn't happen anyway.)
Please take a look.

[bbudge, 2013-06-18 14:54:37]

A few comment nits and one comment about numeric cast.
Regarding SocketOptionData, I would just embed an int32_t in the message and use
the enum value to guide conversion, but I'm not sure what you and Chris
discussed so I leave it to you.

LGTM.

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

https://codereview.chromium.org/16959005/diff/58002/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:21: // It should be set before calling
Bind().
s/It/This option/
Here and below...

https://codereview.chromium.org/16959005/diff/58002/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:100: * Sets a socket option to
|udp_socket|.
s/to/on

https://codereview.chromium.org/16959005/diff/58002/ppapi/host/error_conversi...
File ppapi/host/error_conversion.cc (right):

https://codereview.chromium.org/16959005/diff/58002/ppapi/host/error_conversi...
ppapi/host/error_conversion.cc:18: CHECK_LE(net_error,
std::numeric_limits<int32_t>::max());
Should we crash the browser process?
Why not use base/safe_numerics.h "checked_numeric_cast<int32_t>()"?

[yzshen1, 2013-06-18 16:39:16]

Thanks, Bill!

About SocketOptionData v.s. int32_t, I will talk with Cris and see what he
thinks.

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

https://codereview.chromium.org/16959005/diff/58002/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:21: // It should be set before calling
Bind().
On 2013/06/18 14:54:37, bbudge1 wrote:
> s/It/This option/
> Here and below...

Done.

https://codereview.chromium.org/16959005/diff/58002/ppapi/api/dev/ppb_udp_soc...
ppapi/api/dev/ppb_udp_socket_dev.idl:100: * Sets a socket option to
|udp_socket|.
On 2013/06/18 14:54:37, bbudge1 wrote:
> s/to/on

Done.

https://codereview.chromium.org/16959005/diff/58002/ppapi/host/error_conversi...
File ppapi/host/error_conversion.cc (right):

https://codereview.chromium.org/16959005/diff/58002/ppapi/host/error_conversi...
ppapi/host/error_conversion.cc:18: CHECK_LE(net_error,
std::numeric_limits<int32_t>::max());
On 2013/06/18 14:54:37, bbudge1 wrote:
> Should we crash the browser process?
I think it should be fine, because having a positive |net_error| value (usually
that means bytes transferred) that can overflow int32_t is not supposed to
happen.

> Why not use base/safe_numerics.h "checked_numeric_cast<int32_t>()"?

Done. Thanks!

[Cris Neckar, 2013-06-18 18:21:06]

lgtm for ipc changes

[yzshen1, 2013-06-18 18:31:03]

On 2013/06/18 18:21:06, Cris Neckar wrote:
> lgtm for ipc changes

Thanks, Cris!

BTW, one thing that I would like to double check with you:
As an alternative to ppapi::SocketOptionData, Bill mentioned above that we could
directly pass an int32_t in the IPC messages, and based on the option type we
may convert this int32_t to a boolean value at the browser process.
I personally think that using ppapi::SocketOptionData is more clear/robust, and
it is easier to extend in the future. What do you think?

[commit-bot: I haz the power, 2013-06-19 06:51:15]

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

[commit-bot: I haz the power, 2013-06-19 13:31:01]

Change committed as 207228

[Tom Sepez, 2013-06-24 17:45:20]

after-the-fact LGTM2 for IPC changes (sorry, was out last week).

[yzshen1, 2013-06-24 17:53:55]

On 2013/06/24 17:45:20, Tom Sepez wrote:
> after-the-fact LGTM2 for IPC changes (sorry, was out last week).

Thanks, Tom! :)