Provide a way to disable Nagle's algorithm on Pepper TCP sockets. (issue 12220050)

[w...@chromium.org]

Reviewers: yzshen1, dmichael,

Message:
yzshen: PTAL :)

dmichael: We don't seem to have any tests for PPB_TCPSocket_Private... am I
just
missing something?

Description:
Provide a way to disable Nagle's algorithm on Pepper TCP sockets.

This CL adds a SetSocketFeature() API to PPB_TCPSocket_Private, supporting
one
option, to control Nagle's algorithim on the underlying socket.

BUG=170248


Please review this at https://chromiumcodereview.appspot.com/12220050/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M content/browser/renderer_host/pepper/pepper_message_filter.h
M content/browser/renderer_host/pepper/pepper_message_filter.cc
M content/browser/renderer_host/pepper/pepper_tcp_socket.h
M content/browser/renderer_host/pepper/pepper_tcp_socket.cc
M content/renderer/pepper/pepper_plugin_delegate_impl.h
M content/renderer/pepper/pepper_plugin_delegate_impl.cc
M ppapi/api/private/ppb_tcp_socket_private.idl
M ppapi/c/pp_macros.h
M ppapi/c/private/ppb_tcp_socket_private.h
M ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c
M ppapi/proxy/ppapi_messages.h
M ppapi/proxy/ppb_tcp_socket_private_proxy.h
M ppapi/proxy/ppb_tcp_socket_private_proxy.cc
M ppapi/shared_impl/private/tcp_socket_private_impl.h
M ppapi/shared_impl/private/tcp_socket_private_impl.cc
M ppapi/thunk/ppb_tcp_socket_private_api.h
M ppapi/thunk/ppb_tcp_socket_private_thunk.cc
M webkit/plugins/ppapi/plugin_delegate.h
M webkit/plugins/ppapi/ppb_tcp_socket_private_impl.h
M webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc

[dmic...@chromium.org]

It looks like there are tests to me: ppapi/tests/test_tcp_socket_private*
...though I haven't looked carefully at their contents.

https://chromiumcodereview.appspot.com/12220050/

[w...@chromium.org]

On 2013/02/07 18:23:10, dmichael wrote:
> It looks like there are tests to me: ppapi/tests/test_tcp_socket_private*
> ...though I haven't looked carefully at their contents.

Ummm... yeah, not entirely sure how I missed those!

I've updated the C++ wrappers and added a simple test.

Updating the wrappers was messy, now that there are three versions of the
interface. It's a shame we can't manipulate resources of allocated via one
version of an interface via APIs from an earlier version.

https://chromiumcodereview.appspot.com/12220050/

[David Michael]

We can. See, e.g., var.cc. We only check the more recent version of an API when the particular function has changed. When the function has not changed, we only use the oldest version of the API that has the function. There's no need to do the complicated fallback from 0.5 to 0.4 to 0.3 in most cases.

Our general strategy is that we'll only have 1 backing implementation for a given resource on a particular version of Chrome. The same object handles all the valid versions for that API. If we ever encounter a place where this won't work, we're likely to define a new API instead of breaking this rule of 1 object per API (regardless of version).

https://chromiumcodereview.appspot.com/12220050/

[w...@chromium.org]

On 2013/02/07 20:00:12, dmichael wrote:

> On Thu, Feb 7, 2013 at 12:48 PM, <mailto:w...@chromium.org> wrote:

> > On 2013/02/07 18:23:10, dmichael wrote:
> >

> >> It looks like there are tests to me: ppapi/tests/test_tcp_socket_**
> >> private*

> >> ...though I haven't looked carefully at their contents.
> >>
> >
> > Ummm... yeah, not entirely sure how I missed those!
> >
> > I've updated the C++ wrappers and added a simple test.
> >
> > Updating the wrappers was messy, now that there are three versions of
> the
> > interface. It's a shame we can't manipulate resources of allocated via
> one
> > version of an interface via APIs from an earlier version.
> >
> We can. See, e.g., var.cc. We only check the more recent version of an API

> when the particular function has changed. When the function has *not
> *changed,

> we only use the oldest version of the API that has the function. There's
> no
> need to do the complicated fallback from 0.5 to 0.4 to 0.3 in most cases.

> Our general strategy is that we'll only have 1 backing implementation for
> a
> given resource on a particular version of Chrome. The same object handles
> all the valid versions for that API. If we ever encounter a place where
> this won't work, we're likely to define a new API instead of breaking this
> rule of 1 object per API (regardless of version).

Right, I remember looking at var.cc previously, so I was a little surprised
by
the complexity in TCPSocket_Private. I've trimmed it, and the UDP socket
wrapper, down.

https://chromiumcodereview.appspot.com/12220050/

[yzs...@chromium.org]

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_message_filter.cc
File content/browser/renderer_host/pepper/pepper_message_filter.cc
(right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_message_filter.cc#newcode364
content/browser/renderer_host/pepper/pepper_message_filter.cc:364:
Do you plan to add implementation later?
If yes, please add a TODO.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.cc
File content/browser/renderer_host/pepper/pepper_tcp_socket.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.cc#newcode204
content/browser/renderer_host/pepper/pepper_tcp_socket.cc:204: case
PP_TCPSOCKETFEATURE_NO_DELAY:
wrong indent.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.cc#newcode431
content/browser/renderer_host/pepper/pepper_tcp_socket.cc:431:
connection_state_ == SSL_CONNECTED ||
wrong indent.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h
File content/browser/renderer_host/pepper/pepper_tcp_socket.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h#newcode63
content/browser/renderer_host/pepper/pepper_tcp_socket.h:63: void
SetBoolFeature(uint32_t name, bool value);
I noticed that in pepper_message_filter, |name| is int32_t.
Is it intentional to be different?

And I think it is better to pass PP_TCPSocketFeature_Private directly.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.cc
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.cc#newcode1187
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1187: uint32
socket_id, int32_t name, bool value) {
- wrong indent.
- From chromium style guide:
"For function declarations and definitions, put each argument on a
separate line unless the whole declaration fits on one line."

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.cc#newcode1508
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1508: uint32
plugin_dispatcher_id, uint32 socket_id, bool succeeded) {
From chromium style guide:
"For function declarations and definitions, put each argument on a
separate line unless the whole declaration fits on one line."

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.h
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.h#newcode271
content/renderer/pepper/pepper_plugin_delegate_impl.h:271: virtual void
TCPSocketSetBoolFeature(uint32 socket_id, int32_t name,
From chromium style guide:
"For function declarations and definitions, put each argument on a
separate line unless the whole declaration fits on one line."

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl
File ppapi/api/private/ppb_tcp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl#newcode13
ppapi/api/private/ppb_tcp_socket_private.idl:13: M26 = 0.5
nit: you probably cannot get it into 26, so this should be changed to
27.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl#newcode20
ppapi/api/private/ppb_tcp_socket_private.idl:20:
PP_TCPSOCKETFEATURE_NO_DELAY = 0,
it seems good to comment that setting this feature after a SSL handshake
is started will fail.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl#newcode23
ppapi/api/private/ppb_tcp_socket_private.idl:23:
PP_TCPSOCKETFEATURE_INVALID = -1
Please order them by their values.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl#newcode156
ppapi/api/private/ppb_tcp_socket_private.idl:156: int32_t
SetSocketFeature([in] PP_Resource udp_socket,
- nit: I think SetFeature() is clear enough.
- udp_socket -> tcp_socket.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/tcp_socket_private.cc
File ppapi/cpp/private/tcp_socket_private.cc (left):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/tcp_socket_private.cc#oldcode51
ppapi/cpp/private/tcp_socket_private.cc:51: if
(has_interface<PPB_TCPSocket_Private_0_4>()) {
I am not sure whether we want to make this change.
This implies that v0.5 method X is able to work with v0.3 method Y. It
may be the case now. v0.5 SetSocketFeature() is able to work with v0.3
Connect/Write()/Read()/etc. However, what if later we introduce a v0.6
Disconnect() which cannot work with v0.3 Connect()?

Unless we want to make this a promise, we may not want to encourage this
usage.

Maybe we could send a mail to the pepper-team@ for discussion.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/udp_socket_private.cc
File ppapi/cpp/private/udp_socket_private.cc (left):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/udp_socket_private.cc#oldcode56
ppapi/cpp/private/udp_socket_private.cc:56: if
(has_interface<PPB_UDPSocket_Private_0_4>()) {
Please see my comment in tcp_socket_private.cc

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/proxy/ppb_instance_proxy.cc
File ppapi/proxy/ppb_instance_proxy.cc (left):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/proxy/ppb_instance_proxy.cc#oldcode1148
ppapi/proxy/ppb_instance_proxy.cc:1148: if
(!dispatcher()->permissions().HasPermission(PERMISSION_DEV))
Is this caused by a sync? It seems to have nothing to do with the other
parts of your CL.
(And if it is, I wonder why this file is included.)

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/proxy/ppb_tcp_socket_private_proxy.cc
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/proxy/ppb_tcp_socket_private_proxy.cc#newcode49
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:49: virtual void
SendBoolSocketFeature(int32_t name, bool value) OVERRIDE;
Please use the enum type instead of int32_t.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.cc
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.cc#newcode230
ppapi/shared_impl/private/tcp_socket_private_impl.cc:230: case
PP_TCPSOCKETFEATURE_NO_DELAY:
wrong indent.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.h
File ppapi/shared_impl/private/tcp_socket_private_impl.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.h#newcode69
ppapi/shared_impl/private/tcp_socket_private_impl.h:69: virtual int32_t
SetSocketFeature(PP_TCPSocketFeature_Private name,
For function declarations and definitions, put each argument on a
separate line unless the whole declaration fits on one line.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.h#newcode70
ppapi/shared_impl/private/tcp_socket_private_impl.h:70: struct PP_Var
value, scoped_refptr<TrackedCallback> callback) OVERRIDE;
you don't need to have 'struct' here.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/thunk/ppb_tcp_socket_private_api.h
File ppapi/thunk/ppb_tcp_socket_private_api.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/thunk/ppb_tcp_socket_private_api.h#newcode44
ppapi/thunk/ppb_tcp_socket_private_api.h:44: PP_Var value,
wrong indent.

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/mock_plugin_delegate.cc
File webkit/plugins/ppapi/mock_plugin_delegate.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/mock_plugin_delegate.cc#newcode267
webkit/plugins/ppapi/mock_plugin_delegate.cc:267: void
MockPluginDelegate::TCPSocketSetBoolFeature(uint32 socket_id, int32_t
name,
For function declarations and definitions, put each argument on a
separate line unless the whole declaration fits on one line.

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/mock_plugin_delegate.h
File webkit/plugins/ppapi/mock_plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/mock_plugin_delegate.h#newcode127
webkit/plugins/ppapi/mock_plugin_delegate.h:127: virtual void
TCPSocketSetBoolFeature(uint32 socket_id, int32_t name,
For function declarations and definitions, put each argument on a
separate line unless the whole declaration fits on one line.

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/plugin_delegate.h
File webkit/plugins/ppapi/plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/plugin_delegate.h#newcode547
webkit/plugins/ppapi/plugin_delegate.h:547: virtual void
TCPSocketSetBoolFeature(uint32 socket_id, int32_t name,
For function declarations and definitions, put each argument on a
separate line unless the whole declaration fits on one line.

https://chromiumcodereview.appspot.com/12220050/

[w...@chromium.org]

PTAL; let's discuss the C++ wrapper versioning issue off-CL.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_message_filter.cc
File content/browser/renderer_host/pepper/pepper_message_filter.cc
(right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_message_filter.cc#newcode364
content/browser/renderer_host/pepper/pepper_message_filter.cc:364:

On 2013/02/08 21:51:11, yzshen1 wrote:
> Do you plan to add implementation later?
> If yes, please add a TODO.

Nope, it's supposed to be added in this CL! Looks like I got my branches
mixed up.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.cc
File content/browser/renderer_host/pepper/pepper_tcp_socket.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.cc#newcode204
content/browser/renderer_host/pepper/pepper_tcp_socket.cc:204: case
PP_TCPSOCKETFEATURE_NO_DELAY:

On 2013/02/08 21:51:11, yzshen1 wrote:
> wrong indent.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.cc#newcode431
content/browser/renderer_host/pepper/pepper_tcp_socket.cc:431:
connection_state_ == SSL_CONNECTED ||

On 2013/02/08 21:51:11, yzshen1 wrote:
> wrong indent.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h
File content/browser/renderer_host/pepper/pepper_tcp_socket.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h#newcode63
content/browser/renderer_host/pepper/pepper_tcp_socket.h:63: void
SetBoolFeature(uint32_t name, bool value);

On 2013/02/08 21:51:11, yzshen1 wrote:
> I noticed that in pepper_message_filter, |name| is int32_t.
> Is it intentional to be different?

No; I just followed the style of the UDP SetBoolSocketFeature
implementation. Fixed.

> And I think it is better to pass PP_TCPSocketFeature_Private directly.

I based this on the UDP SetSocketFeature implementation, which uses
int32_t here, but I've switched these to use the enum.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.cc
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.cc#newcode1187
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1187: uint32
socket_id, int32_t name, bool value) {

On 2013/02/08 21:51:11, yzshen1 wrote:
> - wrong indent.
> - From chromium style guide:
> "For function declarations and definitions, put each argument on a
separate line
> unless the whole declaration fits on one line."

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.cc#newcode1508
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1508: uint32
plugin_dispatcher_id, uint32 socket_id, bool succeeded) {

On 2013/02/08 21:51:11, yzshen1 wrote:
> From chromium style guide:
> "For function declarations and definitions, put each argument on a
separate line
> unless the whole declaration fits on one line."

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.h
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/renderer/pepper/pepper_plugin_delegate_impl.h#newcode271
content/renderer/pepper/pepper_plugin_delegate_impl.h:271: virtual void
TCPSocketSetBoolFeature(uint32 socket_id, int32_t name,

On 2013/02/08 21:51:11, yzshen1 wrote:
> From chromium style guide:
> "For function declarations and definitions, put each argument on a
separate line
> unless the whole declaration fits on one line."

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl
File ppapi/api/private/ppb_tcp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl#newcode13
ppapi/api/private/ppb_tcp_socket_private.idl:13: M26 = 0.5

On 2013/02/08 21:51:11, yzshen1 wrote:
> nit: you probably cannot get it into 26, so this should be changed to
27.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl#newcode20
ppapi/api/private/ppb_tcp_socket_private.idl:20:
PP_TCPSOCKETFEATURE_NO_DELAY = 0,

On 2013/02/08 21:51:11, yzshen1 wrote:
> it seems good to comment that setting this feature after a SSL
handshake is
> started will fail.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl#newcode23
ppapi/api/private/ppb_tcp_socket_private.idl:23:
PP_TCPSOCKETFEATURE_INVALID = -1

On 2013/02/08 21:51:11, yzshen1 wrote:
> Please order them by their values.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/api/private/ppb_tcp_socket_private.idl#newcode156
ppapi/api/private/ppb_tcp_socket_private.idl:156: int32_t
SetSocketFeature([in] PP_Resource udp_socket,

On 2013/02/08 21:51:11, yzshen1 wrote:
> - nit: I think SetFeature() is clear enough.
> - udp_socket -> tcp_socket.

SGTM; I'd chosen the function name to match the one in
UDPSocket_Private. Since we don't care about consistency between the
APIs, I've renamed this to SetOption() for consistency with BSD socket
terminology.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/tcp_socket_private.cc
File ppapi/cpp/private/tcp_socket_private.cc (left):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/tcp_socket_private.cc#oldcode51
ppapi/cpp/private/tcp_socket_private.cc:51: if
(has_interface<PPB_TCPSocket_Private_0_4>()) {

On 2013/02/08 21:51:11, yzshen1 wrote:
> I am not sure whether we want to make this change.
> This implies that v0.5 method X is able to work with v0.3 method Y. It
may be
> the case now. v0.5 SetSocketFeature() is able to work with v0.3
> Connect/Write()/Read()/etc. However, what if later we introduce a v0.6
> Disconnect() which cannot work with v0.3 Connect()?

> Unless we want to make this a promise, we may not want to encourage
this usage.

> Maybe we could send a mail to the pepper-team@ for discussion.

This change was suggested by dmichael@ - let's discuss offline. We
should make sure we have clear guidelines on stuff like this.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/proxy/ppb_instance_proxy.cc
File ppapi/proxy/ppb_instance_proxy.cc (left):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/proxy/ppb_instance_proxy.cc#oldcode1148
ppapi/proxy/ppb_instance_proxy.cc:1148: if
(!dispatcher()->permissions().HasPermission(PERMISSION_DEV))

On 2013/02/08 21:51:11, yzshen1 wrote:
> Is this caused by a sync? It seems to have nothing to do with the
other parts of
> your CL.
> (And if it is, I wonder why this file is included.)

Mix up w/ a branch for an un-related CL. :(

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/proxy/ppb_tcp_socket_private_proxy.cc
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/proxy/ppb_tcp_socket_private_proxy.cc#newcode49
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:49: virtual void
SendBoolSocketFeature(int32_t name, bool value) OVERRIDE;

On 2013/02/08 21:51:11, yzshen1 wrote:
> Please use the enum type instead of int32_t.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.cc
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.cc#newcode230
ppapi/shared_impl/private/tcp_socket_private_impl.cc:230: case
PP_TCPSOCKETFEATURE_NO_DELAY:

On 2013/02/08 21:51:11, yzshen1 wrote:
> wrong indent.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.h
File ppapi/shared_impl/private/tcp_socket_private_impl.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.h#newcode69
ppapi/shared_impl/private/tcp_socket_private_impl.h:69: virtual int32_t
SetSocketFeature(PP_TCPSocketFeature_Private name,

On 2013/02/08 21:51:11, yzshen1 wrote:
> For function declarations and definitions, put each argument on a
separate line
> unless the whole declaration fits on one line.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/shared_impl/private/tcp_socket_private_impl.h#newcode70
ppapi/shared_impl/private/tcp_socket_private_impl.h:70: struct PP_Var
value, scoped_refptr<TrackedCallback> callback) OVERRIDE;

On 2013/02/08 21:51:11, yzshen1 wrote:
> you don't need to have 'struct' here.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/thunk/ppb_tcp_socket_private_api.h
File ppapi/thunk/ppb_tcp_socket_private_api.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/thunk/ppb_tcp_socket_private_api.h#newcode44
ppapi/thunk/ppb_tcp_socket_private_api.h:44: PP_Var value,

On 2013/02/08 21:51:11, yzshen1 wrote:
> wrong indent.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/mock_plugin_delegate.cc
File webkit/plugins/ppapi/mock_plugin_delegate.cc (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/mock_plugin_delegate.cc#newcode267
webkit/plugins/ppapi/mock_plugin_delegate.cc:267: void
MockPluginDelegate::TCPSocketSetBoolFeature(uint32 socket_id, int32_t
name,

On 2013/02/08 21:51:11, yzshen1 wrote:
> For function declarations and definitions, put each argument on a
separate line
> unless the whole declaration fits on one line.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/mock_plugin_delegate.h
File webkit/plugins/ppapi/mock_plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/mock_plugin_delegate.h#newcode127
webkit/plugins/ppapi/mock_plugin_delegate.h:127: virtual void
TCPSocketSetBoolFeature(uint32 socket_id, int32_t name,

On 2013/02/08 21:51:11, yzshen1 wrote:
> For function declarations and definitions, put each argument on a
separate line
> unless the whole declaration fits on one line.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/plugin_delegate.h
File webkit/plugins/ppapi/plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/webkit/plugins/ppapi/plugin_delegate.h#newcode547
webkit/plugins/ppapi/plugin_delegate.h:547: virtual void
TCPSocketSetBoolFeature(uint32 socket_id, int32_t name,

On 2013/02/08 21:51:11, yzshen1 wrote:
> For function declarations and definitions, put each argument on a
separate line
> unless the whole declaration fits on one line.

Done.

https://chromiumcodereview.appspot.com/12220050/

[w...@chromium.org]

FYI, I've added thunking for the new version of the interface - I notice the
previous version wasn't thunked but I think that was just because the
methods it
added were private-only?

https://chromiumcodereview.appspot.com/12220050/

[yzs...@chromium.org]

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h
File content/browser/renderer_host/pepper/pepper_tcp_socket.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h#newcode63
content/browser/renderer_host/pepper/pepper_tcp_socket.h:63: void
SetBoolFeature(uint32_t name, bool value);

On 2013/02/10 04:47:02, Wez wrote:
> On 2013/02/08 21:51:11, yzshen1 wrote:
> > I noticed that in pepper_message_filter, |name| is int32_t.
> > Is it intentional to be different?

> No; I just followed the style of the UDP SetBoolSocketFeature
implementation.
> Fixed.

> > And I think it is better to pass PP_TCPSocketFeature_Private
directly.

> I based this on the UDP SetSocketFeature implementation, which uses
int32_t
> here, but I've switched these to use the enum.

I didn't see that you have switched to enum. Have you uploaded the
latest patch?

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/tcp_socket_private.cc
File ppapi/cpp/private/tcp_socket_private.cc (left):

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/tcp_socket_private.cc#oldcode51
ppapi/cpp/private/tcp_socket_private.cc:51: if
(has_interface<PPB_TCPSocket_Private_0_4>()) {

Sounds good.

On 2013/02/10 04:47:02, Wez wrote:
> On 2013/02/08 21:51:11, yzshen1 wrote:
> > I am not sure whether we want to make this change.
> > This implies that v0.5 method X is able to work with v0.3 method Y.
It may be
> > the case now. v0.5 SetSocketFeature() is able to work with v0.3
> > Connect/Write()/Read()/etc. However, what if later we introduce a
v0.6
> > Disconnect() which cannot work with v0.3 Connect()?
> >
> > Unless we want to make this a promise, we may not want to encourage
this
> usage.
> >
> > Maybe we could send a mail to the pepper-team@ for discussion.

> This change was suggested by dmichael@ - let's discuss offline. We
should make
> sure we have clear guidelines on stuff like this.

https://chromiumcodereview.appspot.com/12220050/diff/6005/content/browser/renderer_host/pepper/pepper_message_filter.h
File content/browser/renderer_host/pepper/pepper_message_filter.h
(right):

https://chromiumcodereview.appspot.com/12220050/diff/6005/content/browser/renderer_host/pepper/pepper_message_filter.h#newcode155
content/browser/renderer_host/pepper/pepper_message_filter.h:155: void
OnTCPSetBoolOption(uint32 socket_id, uint32_t name, bool value);
I still think that using the enum type is better than uint32_t.

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1221
ppapi/proxy/ppapi_messages.h:1221: uint32_t /* socket_id */,
Please be consistent with other messages about the type of |socket_id|.

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1222
ppapi/proxy/ppapi_messages.h:1222: uint32_t /* name */,
Please use the enum type.

https://chromiumcodereview.appspot.com/12220050/

[Yuzhu Shen]

> The previous version wasn't thunked

What do you mean by that? I thought we had const GetPPB_TCPSocket_Private_0_4_Thunk() and GetPPB_TCPSocket_Private_0_3_Thunk()?

--
Best regards,
Yuzhu Shen.

[w...@chromium.org]

On 2013/02/12 18:02:46, yzshen wrote:
> > The previous version wasn't thunked
> What do you mean by that? I thought we had const
> GetPPB_TCPSocket_Private_0_4_Thunk() and
> GetPPB_TCPSocket_Private_0_3_Thunk()?

See the old-style thunking definitions in ppapi/thunk/thunk.h.

> On Sun, Feb 10, 2013 at 1:20 AM, <mailto:w...@chromium.org> wrote:

> >
> > FYI, I've added thunking for the new version of the interface - I notice
> > the
> > previous version wasn't thunked but I think that was just because the
> > methods it
> > added were private-only?
> >
> >

https://chromiumcodereview.**appspot.com/12220050/%3Chttps://chromiumcodereview.appspot.com/12220050/>

> >



> --
> Best regards,
> Yuzhu Shen.

https://chromiumcodereview.appspot.com/12220050/

[w...@chromium.org]

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h
File content/browser/renderer_host/pepper/pepper_tcp_socket.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h#newcode63
content/browser/renderer_host/pepper/pepper_tcp_socket.h:63: void
SetBoolFeature(uint32_t name, bool value);

On 2013/02/12 17:39:58, yzshen1 wrote:
> On 2013/02/10 04:47:02, Wez wrote:
> > On 2013/02/08 21:51:11, yzshen1 wrote:
> > > I noticed that in pepper_message_filter, |name| is int32_t.
> > > Is it intentional to be different?
> >
> > No; I just followed the style of the UDP SetBoolSocketFeature
implementation.
> > Fixed.
> >
> > > And I think it is better to pass PP_TCPSocketFeature_Private
directly.
> >
> > I based this on the UDP SetSocketFeature implementation, which uses
int32_t
> > here, but I've switched these to use the enum.

> I didn't see that you have switched to enum. Have you uploaded the
latest patch?

Yes; see PepperPluginDelegateImpl. This SetBoolFeature() is called by
PepperMessageFilter w/ a uint32_t "name" received via IPC, so to switch
this to an enum would require duplicating knowledge of the valid values.

https://chromiumcodereview.appspot.com/12220050/diff/6005/content/browser/renderer_host/pepper/pepper_message_filter.h
File content/browser/renderer_host/pepper/pepper_message_filter.h
(right):

https://chromiumcodereview.appspot.com/12220050/diff/6005/content/browser/renderer_host/pepper/pepper_message_filter.h#newcode155
content/browser/renderer_host/pepper/pepper_message_filter.h:155: void
OnTCPSetBoolOption(uint32 socket_id, uint32_t name, bool value);

On 2013/02/12 17:39:58, yzshen1 wrote:
> I still think that using the enum type is better than uint32_t.

This is an IPC handler.

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1221
ppapi/proxy/ppapi_messages.h:1221: uint32_t /* socket_id */,

On 2013/02/12 17:39:58, yzshen1 wrote:
> Please be consistent with other messages about the type of
|socket_id|.

Done.

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1222
ppapi/proxy/ppapi_messages.h:1222: uint32_t /* name */,

On 2013/02/12 17:39:58, yzshen1 wrote:
> Please use the enum type.

This is an IPC message definition. Using the enum type here would imply
that recipients can rely on the field having one of the defined enum
values, which is not the case.

https://chromiumcodereview.appspot.com/12220050/

[Yuzhu Shen]

I think this is not used but somebody forgot to remove it.

Please see interfaces_ppb_private_no_permissions.h

[yzs...@chromium.org]

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h
File content/browser/renderer_host/pepper/pepper_tcp_socket.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/12001/content/browser/renderer_host/pepper/pepper_tcp_socket.h#newcode63
content/browser/renderer_host/pepper/pepper_tcp_socket.h:63: void
SetBoolFeature(uint32_t name, bool value);

You could directly use the enum over IPC, and use it at the host side.
For example, see:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_messages.h&q=ppapi_messages.h&sq=package:chromium&type=cs&l=69

Please see my comment in pepper_tcp_socket.h
You can use enum over IPC.

On 2013/02/12 18:45:21, Wez wrote:
> On 2013/02/12 17:39:58, yzshen1 wrote:
> > I still think that using the enum type is better than uint32_t.

> This is an IPC handler.

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1222
ppapi/proxy/ppapi_messages.h:1222: uint32_t /* name */,

On 2013/02/12 18:45:21, Wez wrote:
> On 2013/02/12 17:39:58, yzshen1 wrote:
> > Please use the enum type.

> This is an IPC message definition. Using the enum type here would
imply that
> recipients can rely on the field having one of the defined enum
values, which is
> not the case.

I don't understand, why the recipients shouldn't rely on the field
having one of the defined enum values?

https://chromiumcodereview.appspot.com/12220050/

[w...@chromium.org]

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1222
ppapi/proxy/ppapi_messages.h:1222: uint32_t /* name */,

On 2013/02/13 08:07:14, yzshen1 wrote:
> On 2013/02/12 18:45:21, Wez wrote:
> > On 2013/02/12 17:39:58, yzshen1 wrote:
> > > Please use the enum type.
> >
> > This is an IPC message definition. Using the enum type here would
imply that
> > recipients can rely on the field having one of the defined enum
values, which
> is
> > not the case.
> I don't understand, why the recipients shouldn't rely on the field
having one of
> the defined enum values?

Because over IPC an enum is transferred as an int. If the peer is buggy
or compromised then the int's value may fall outside the enum's range.
Since the IPC layer simply static_cast<>s the int back to the enum, the
resulting value is undefined for values outside the enum's range.

https://chromiumcodereview.appspot.com/12220050/

[yzs...@chromium.org]

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1222
ppapi/proxy/ppapi_messages.h:1222: uint32_t /* name */,

Your concern makes sense. I am okay to do it your way (for now).

However, we need to find a better way to fix the general problem. Using
integer as a replacement of enum doesn't seem to be a real fix:
- It hurts readability.
- It is still cast back to the enum type (in pepper_tcp_socket.cc). And
I think you rely on the 'default' branch to catch out-of-range values.
There are two problems:
(1) the validation is not in the IPC layer, instead it happens long
after we receive the IPC message. Imagine that we have a lot of messages
passing enum types as integers. The validation will be scattered in
different parts of our codebase. And it relies on every user of those
messages to do the validation, which is error-prone.
(2) Since the result of the cast is undefined, theoretically it can
be any value. Therefore, it may run any branch of the switch statement.
That uncertainty bothers me as well. :)

[w...@chromium.org]

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1222
ppapi/proxy/ppapi_messages.h:1222: uint32_t /* name */,

Agreed; I've filed crbug.com/176110 for fixing the IPC issue.

https://chromiumcodereview.appspot.com/12220050/diff/6005/ppapi/proxy/ppapi_messages.h#newcode1222
ppapi/proxy/ppapi_messages.h:1222: uint32_t /* name */,

On 2013/02/14 08:35:38, yzshen1 wrote:
> Your concern makes sense. I am okay to do it your way (for now).

> However, we need to find a better way to fix the general problem.
Using integer
> as a replacement of enum doesn't seem to be a real fix:
> - It hurts readability.
> - It is still cast back to the enum type (in pepper_tcp_socket.cc).
And I think
> you rely on the 'default' branch to catch out-of-range values.

D'oh! Yes, I should remove the cast.

There are two
> problems:
> (1) the validation is not in the IPC layer, instead it happens
long after we
> receive the IPC message. Imagine that we have a lot of messages
passing enum
> types as integers. The validation will be scattered in different parts
of our
> codebase. And it relies on every user of those messages to do the
validation,
> which is error-prone.
> (2) Since the result of the cast is undefined, theoretically it
can be any
> value. Therefore, it may run any branch of the switch statement. That
> uncertainty bothers me as well. :)

Yup. See above.

I'll update the CL to remove the cast - if you're otherwise happy with
it then please wield your awesome OWNER powers. ;)

[yzs...@chromium.org]

LGTM

Thanks!

https://chromiumcodereview.appspot.com/12220050/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/w...@chromium.org/12220050/15034


https://chromiumcodereview.appspot.com/12220050/

[commi...@chromium.org]

Presubmit check for 12220050-15034 failed and returned exit status 1.

INFO:root:Found 28 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ppapi/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
ppapi/proxy/ppapi_messages.h

Missing PPAPI IDL for stable interface:

***************
ppapi/c/pp_macros.h
***************

Presubmit checks took 4.2s to calculate.



https://chromiumcodereview.appspot.com/12220050/

[w...@chromium.org]

Cris, can you review the IPC message & handling, plz?

https://chromiumcodereview.appspot.com/12220050/

[tse...@chromium.org]

Messages LGTM.

https://chromiumcodereview.appspot.com/12220050/

[w...@chromium.org]

On 2013/02/15 22:48:28, Tom Sepez wrote:
> Messages LGTM.

Thanks!

https://chromiumcodereview.appspot.com/12220050/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/w...@chromium.org/12220050/18002


https://chromiumcodereview.appspot.com/12220050/

[commi...@chromium.org]

Presubmit check for 12220050-18002 failed and returned exit status 1.

INFO:root:Found 28 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ppapi/PRESUBMIT.py

** Presubmit ERRORS **

Missing PPAPI IDL for stable interface:

***************
ppapi/c/pp_macros.h
***************

Presubmit checks took 4.4s to calculate.



https://chromiumcodereview.appspot.com/12220050/