AllowBroadcast() is exposed to NaCl.

AllowBroadcast() is exposed to NaCl.


BUG=136797
TEST=browser_tests:*.UDPSocketPrivate

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

[ygorshenin1, 2012-08-07 14:36:28]

+ bbudge for ppapi/native_client/*

[bbudge, 2012-08-07 18:16:21]

LGTM

[bbudge, 2012-08-07 19:27:22]

On 2012/08/07 18:16:21, bbudge1 wrote:
> LGTM

Yuzhu is on paternity leave. You might want to find another PPAPI owner for
review.

[ygorshenin1, 2012-08-08 07:42:48]

- yzshen
+ brettw

[ygorshenin1, 2012-08-13 19:03:29]

Brett, friendly ping.

[ygorshenin1, 2012-08-17 08:59:21]

+ jam for content/*
+ viettrungluu for ppapi part

[jam, 2012-08-17 16:45:22]

On 2012/08/17 08:59:21, ygorshenin1 wrote:
> + jam for content/*

I'm not familiar with that code, so I'm not a good reviewer.

[brettw, 2012-08-20 04:34:45]

Sorry for the delay, been slammed with flapper.

http://codereview.chromium.org/10735056/diff/21001/ppapi/api/private/ppb_udp_...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/api/private/ppb_udp_...
ppapi/api/private/ppb_udp_socket_private.idl:43: [version=0.4]
Can you provide documentation for this? In particular, what the callback means.
Does it mean the setting has taken effect? Do we really need the callback?
Normally these "set the setting" things don't need one. And it's also weird that
you can only set one setting at once (you could easily imagine wanting to set
several options on the socket).

http://codereview.chromium.org/10735056/diff/21001/ppapi/native_client/src/sh...
File ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/native_client/src/sh...
ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc:2989: NaClSrpcError
PpbUDPSocketPrivateRpcClient::PPB_UDPSocket_Private_SetSocketFeature(
If this file isn't autogenerated, check length

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1387: PP_UDPSocketFeature_Private /* name */,
For the enums we should cast them to int32s. This will allow you to remove some
dependencies, but more importantly it needs to be explicitly sized we may be
sending between a 64- and 32-bit process in the future.

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppb_udp_socket...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppb_udp_socket...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:67: name, (value.value.as_bool ==
PP_TRUE)));
You can use PP_ToBool. Actually, I'd probably just send a SerializedVar in the
IPC message. You can grep for examples. Then you don't need to do the checking
and extraction here (since you need to do it in the _impl side anyway).

http://codereview.chromium.org/10735056/diff/21001/ppapi/tests/test_udp_socke...
File ppapi/tests/test_udp_socket_private.cc (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/tests/test_udp_socke...
ppapi/tests/test_udp_socket_private.cc:80:
TestUDPSocketPrivate::SetBroadcastOptions(pp::UDPSocketPrivate* socket) {
I'd wrap this before the arg like the previous test.

[ygorshenin1, 2012-08-20 12:08:01]

PTAL

http://codereview.chromium.org/10735056/diff/21001/ppapi/api/private/ppb_udp_...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/api/private/ppb_udp_...
ppapi/api/private/ppb_udp_socket_private.idl:43: [version=0.4]
Done.

Yes, callback means that the setting has effect. SetSocketFeature takes PP_Var
as value and callback so it's very general and can be easily extended. If some
complex check will be performed on the browser side, callback will be the only
way to pass back results.

I agree, that set once setting at once is weird, but in this case we can easily
verify compatibility of feature name and value type (and value). Also POSIX and
WS2 setsockopt takes option name, not or-ed combination of names.

On 2012/08/20 04:34:45, brettw wrote:
> Can you provide documentation for this? In particular, what the callback
means.
> Does it mean the setting has taken effect? Do we really need the callback?
> Normally these "set the setting" things don't need one. And it's also weird
that
> you can only set one setting at once (you could easily imagine wanting to set
> several options on the socket).

http://codereview.chromium.org/10735056/diff/21001/ppapi/native_client/src/sh...
File ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/native_client/src/sh...
ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc:2989: NaClSrpcError
PpbUDPSocketPrivateRpcClient::PPB_UDPSocket_Private_SetSocketFeature(
This file is autogenerated by
src/ppapi/native_client/src/shared/ppapi_proxy/run_srpcgen.py.

On 2012/08/20 04:34:45, brettw wrote:
> If this file isn't autogenerated, check length

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1387: PP_UDPSocketFeature_Private /* name */,
On 2012/08/20 04:34:45, brettw wrote:
> For the enums we should cast them to int32s. This will allow you to remove
some
> dependencies, but more importantly it needs to be explicitly sized we may be
> sending between a 64- and 32-bit process in the future.

Done.

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppb_udp_socket...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppb_udp_socket...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:67: name, (value.value.as_bool ==
PP_TRUE)));
Done.

Yes, I considered this way. It's possible to send SerializedVarSendInput for
out-of-process plugins, but I don't know how to send it from in-process plugins.
And in any case I need checking and extraction code.

On 2012/08/20 04:34:45, brettw wrote:
> You can use PP_ToBool. Actually, I'd probably just send a SerializedVar in the
> IPC message. You can grep for examples. Then you don't need to do the checking
> and extraction here (since you need to do it in the _impl side anyway).

http://codereview.chromium.org/10735056/diff/21001/ppapi/tests/test_udp_socke...
File ppapi/tests/test_udp_socket_private.cc (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/tests/test_udp_socke...
ppapi/tests/test_udp_socket_private.cc:80:
TestUDPSocketPrivate::SetBroadcastOptions(pp::UDPSocketPrivate* socket) {
On 2012/08/20 04:34:45, brettw wrote:
> I'd wrap this before the arg like the previous test.

Done.

[brettw, 2012-08-20 17:55:15]

http://codereview.chromium.org/10735056/diff/21001/ppapi/api/private/ppb_udp_...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/api/private/ppb_udp_...
ppapi/api/private/ppb_udp_socket_private.idl:43: [version=0.4]
My weird that you can't set more than one at a time, I wasn't referring to
bitwise or-ing things, but rather that if you want to set two options, you have
to write a complex state machine since you can't set the second one until the
callback for the first one completes.

I don't think we should design this based on potential future extensibility. We
can always add a separate function or a new option setting function if we start
adding fancy checks that require callbacks.

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppb_udp_socket...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppb_udp_socket...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:67: name, (value.value.as_bool ==
PP_TRUE)));
The implementation of in process plugins and the proxy are independent. You
should be able to switch the IPC message to use a serialized var and just change
the functions in this proxy file to use the serialized var stuff, and not change
anything else (unless I'm misunderstanding something).

[ygorshenin1, 2012-08-21 12:00:27]

PTAL

http://codereview.chromium.org/10735056/diff/21001/ppapi/api/private/ppb_udp_...
File ppapi/api/private/ppb_udp_socket_private.idl (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/api/private/ppb_udp_...
ppapi/api/private/ppb_udp_socket_private.idl:43: [version=0.4]
OK, I'll delete callbacks.

On 2012/08/20 17:55:15, brettw wrote:
> My weird that you can't set more than one at a time, I wasn't referring to
> bitwise or-ing things, but rather that if you want to set two options, you
have
> to write a complex state machine since you can't set the second one until the
> callback for the first one completes.
> 
> I don't think we should design this based on potential future extensibility.
We
> can always add a separate function or a new option setting function if we
start
> adding fancy checks that require callbacks.

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppb_udp_socket...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/10735056/diff/21001/ppapi/proxy/ppb_udp_socket...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:67: name, (value.value.as_bool ==
PP_TRUE)));
As was decided in the discussion, PpapiHostMsg_PPBUDPSocket_Set{T}SocketFeature
messages should be used.

On 2012/08/20 17:55:15, brettw wrote:
> The implementation of in process plugins and the proxy are independent. You
> should be able to switch the IPC message to use a serialized var and just
change
> the functions in this proxy file to use the serialized var stuff, and not
change
> anything else (unless I'm misunderstanding something).

[brettw, 2012-08-22 22:45:21]

lgtm

http://codereview.chromium.org/10735056/diff/19012/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/udp_socket_private_impl.cc (right):

http://codereview.chromium.org/10735056/diff/19012/ppapi/shared_impl/private/...
ppapi/shared_impl/private/udp_socket_private_impl.cc:49:
SendSetSocketFeature(name, value);
If you want to keep this function returning int32_t, I think this Send function
should also return the int32_t, and it should return PP_ERROR_BADARGUMENT if you
supply an invalid enum, rather than asserting.

http://codereview.chromium.org/10735056/diff/19012/ppapi/thunk/ppb_udp_socket...
File ppapi/thunk/ppb_udp_socket_private_thunk.cc (right):

http://codereview.chromium.org/10735056/diff/19012/ppapi/thunk/ppb_udp_socket...
ppapi/thunk/ppb_udp_socket_private_thunk.cc:38: return PP_ERROR_FAILED;
If we're going to return int32_t, this should probably be PP_ERROR_BADRESOURCE.
I would actually change the return value of this function to be PP_Bool and just
return  PP_FALSE here, but I'll leave this up to you.

[ygorshenin1, 2012-08-23 09:48:40]

Thanks! As test was added that checks SetSocketFeature return values, please
PTAL.

http://codereview.chromium.org/10735056/diff/19012/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/udp_socket_private_impl.cc (right):

http://codereview.chromium.org/10735056/diff/19012/ppapi/shared_impl/private/...
ppapi/shared_impl/private/udp_socket_private_impl.cc:49:
SendSetSocketFeature(name, value);
On 2012/08/22 22:45:21, brettw wrote:
> If you want to keep this function returning int32_t, I think this Send
function
> should also return the int32_t, and it should return PP_ERROR_BADARGUMENT if
you
> supply an invalid enum, rather than asserting.

Done.

http://codereview.chromium.org/10735056/diff/19012/ppapi/thunk/ppb_udp_socket...
File ppapi/thunk/ppb_udp_socket_private_thunk.cc (right):

http://codereview.chromium.org/10735056/diff/19012/ppapi/thunk/ppb_udp_socket...
ppapi/thunk/ppb_udp_socket_private_thunk.cc:38: return PP_ERROR_FAILED;
On 2012/08/22 22:45:21, brettw wrote:
> If we're going to return int32_t, this should probably be
PP_ERROR_BADRESOURCE.
> I would actually change the return value of this function to be PP_Bool and
just
> return  PP_FALSE here, but I'll leave this up to you.

Just to distinguish between cases when method was called with incorrect
name/value/value type arguments and when some internal error occurs I prefer to
return int32_t.

[viettrungluu, 2012-08-27 18:33:54]

http://codereview.chromium.org/10735056/diff/29006/ppapi/cpp/private/udp_sock...
File ppapi/cpp/private/udp_socket_private.cc (right):

http://codereview.chromium.org/10735056/diff/29006/ppapi/cpp/private/udp_sock...
ppapi/cpp/private/udp_socket_private.cc:21: 
Could you please add backwards compatibility with 0.3? (I.e., make the wrapper
work with 0.3 as well, except for SetSocketFeature(), obviously. See
ppapi/cpp/private/tcp_socket_private.cpp to compare.)

http://codereview.chromium.org/10735056/diff/33042/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

http://codereview.chromium.org/10735056/diff/33042/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:386: void
PepperMessageFilter::OnUDPSetBoolSocketFeature(
What thread is this called on? (I think everything you do should be okay
regardless, but it wouldn't hurt to check.)

http://codereview.chromium.org/10735056/diff/33042/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:410: NOTREACHED();
Probably should have a break or return after this, for good form.

http://codereview.chromium.org/10735056/diff/33042/ppapi/cpp/private/udp_sock...
File ppapi/cpp/private/udp_socket_private.h (right):

http://codereview.chromium.org/10735056/diff/33042/ppapi/cpp/private/udp_sock...
ppapi/cpp/private/udp_socket_private.h:11: #include "ppapi/cpp/var.h"
No need to actually include this here. A forward declaration is enough.

http://codereview.chromium.org/10735056/diff/33042/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/udp_socket_private_impl.cc (right):

http://codereview.chromium.org/10735056/diff/33042/ppapi/shared_impl/private/...
ppapi/shared_impl/private/udp_socket_private_impl.cc:52: if (PP_VARTYPE_BOOL !=
value.type)
I think we'd generally prefer |value.type != PP_VARTYPE_BOOL)|.

http://codereview.chromium.org/10735056/diff/33042/ppapi/tests/test_udp_socke...
File ppapi/tests/test_udp_socket_private.h (right):

http://codereview.chromium.org/10735056/diff/33042/ppapi/tests/test_udp_socke...
ppapi/tests/test_udp_socket_private.h:45: const PPB_UDPSocket_Private_0_4*
interface_;
These tests use the C++ wrappers, so you should just continue to use them.

[ygorshenin1, 2012-08-27 19:12:47]

PTAL

http://codereview.chromium.org/10735056/diff/29006/ppapi/cpp/private/udp_sock...
File ppapi/cpp/private/udp_socket_private.cc (right):

http://codereview.chromium.org/10735056/diff/29006/ppapi/cpp/private/udp_sock...
ppapi/cpp/private/udp_socket_private.cc:21: 
On 2012/08/27 18:33:54, viettrungluu wrote:
> Could you please add backwards compatibility with 0.3? (I.e., make the wrapper
> work with 0.3 as well, except for SetSocketFeature(), obviously. See
> ppapi/cpp/private/tcp_socket_private.cpp to compare.)

Done.

http://codereview.chromium.org/10735056/diff/33042/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

http://codereview.chromium.org/10735056/diff/33042/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:386: void
PepperMessageFilter::OnUDPSetBoolSocketFeature(
On 2012/08/27 18:33:54, viettrungluu wrote:
> What thread is this called on? (I think everything you do should be okay
> regardless, but it wouldn't hurt to check.)

Done.

http://codereview.chromium.org/10735056/diff/33042/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:410: NOTREACHED();
On 2012/08/27 18:33:54, viettrungluu wrote:
> Probably should have a break or return after this, for good form.

Done.

http://codereview.chromium.org/10735056/diff/33042/ppapi/cpp/private/udp_sock...
File ppapi/cpp/private/udp_socket_private.h (right):

http://codereview.chromium.org/10735056/diff/33042/ppapi/cpp/private/udp_sock...
ppapi/cpp/private/udp_socket_private.h:11: #include "ppapi/cpp/var.h"
On 2012/08/27 18:33:54, viettrungluu wrote:
> No need to actually include this here. A forward declaration is enough.

Done.

http://codereview.chromium.org/10735056/diff/33042/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/udp_socket_private_impl.cc (right):

http://codereview.chromium.org/10735056/diff/33042/ppapi/shared_impl/private/...
ppapi/shared_impl/private/udp_socket_private_impl.cc:52: if (PP_VARTYPE_BOOL !=
value.type)
On 2012/08/27 18:33:54, viettrungluu wrote:
> I think we'd generally prefer |value.type != PP_VARTYPE_BOOL)|.

Done.

http://codereview.chromium.org/10735056/diff/33042/ppapi/tests/test_udp_socke...
File ppapi/tests/test_udp_socket_private.h (right):

http://codereview.chromium.org/10735056/diff/33042/ppapi/tests/test_udp_socke...
ppapi/tests/test_udp_socket_private.h:45: const PPB_UDPSocket_Private_0_4*
interface_;
Please, take a look at TestSetSocketFeatureErrors. It uses raw
PPB_UDPSocket_Private_0_4 interface.

On 2012/08/27 18:33:54, viettrungluu wrote:
> These tests use the C++ wrappers, so you should just continue to use them.

[viettrungluu, 2012-08-29 17:52:47]

http://codereview.chromium.org/10735056/diff/33042/ppapi/tests/test_udp_socke...
File ppapi/tests/test_udp_socket_private.h (right):

http://codereview.chromium.org/10735056/diff/33042/ppapi/tests/test_udp_socke...
ppapi/tests/test_udp_socket_private.h:45: const PPB_UDPSocket_Private_0_4*
interface_;
On 2012/08/27 19:12:47, ygorshenin1 wrote:
> Please, take a look at TestSetSocketFeatureErrors. It uses raw
> PPB_UDPSocket_Private_0_4 interface.

My point is that it shouldn't.

> 
> On 2012/08/27 18:33:54, viettrungluu wrote:
> > These tests use the C++ wrappers, so you should just continue to use them.
>

[ygorshenin1, 2012-08-30 05:41:59]

PTAL

http://codereview.chromium.org/10735056/diff/33042/ppapi/tests/test_udp_socke...
File ppapi/tests/test_udp_socket_private.h (right):

http://codereview.chromium.org/10735056/diff/33042/ppapi/tests/test_udp_socke...
ppapi/tests/test_udp_socket_private.h:45: const PPB_UDPSocket_Private_0_4*
interface_;
On 2012/08/29 17:52:47, viettrungluu wrote:
> On 2012/08/27 19:12:47, ygorshenin1 wrote:
> > Please, take a look at TestSetSocketFeatureErrors. It uses raw
> > PPB_UDPSocket_Private_0_4 interface.
> 
> My point is that it shouldn't.
> 
> > 
> > On 2012/08/27 18:33:54, viettrungluu wrote:
> > > These tests use the C++ wrappers, so you should just continue to use them.
> > 
> 

Done.

[viettrungluu, 2012-08-30 20:31:32]

LGTM, thanks.

[ygorshenin1, 2012-08-31 08:42:41]

Many thanks!

[commit-bot: I haz the power, 2012-08-31 08:42:59]

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

[commit-bot: I haz the power, 2012-08-31 08:43:10]

Presubmit check for 10735056-49003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc, line 2800, 83
chars \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 3004, 103
chars

Presubmit checks took 4.1s to calculate.