Exposed Listen and Accept methods to plugin.

Exposed Listen and Accept methods to in-process plugins.


BUG=108277
TEST=UI test TestTCPServerSocketPrivate


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123033

[brettw, 2012-01-24 17:19:46]

Can you give some background about why this is needed? I only see a bug you
filed with a one sentence request that this be added.

[dpolukhin, 2012-01-25 07:34:38]

On 2012/01/24 17:19:46, brettw wrote:
> Can you give some background about why this is needed? I only see a bug you
> filed with a one sentence request that this be added.

We need TCP port listening to implement port forwarding in NaCl openssh port.
Also port listening is required to implement things like video streaming, when
source has to implement HTTP server. Let's create mail thread if you would like
to discuss exact use cases that we would like to support.

[brettw, 2012-02-07 00:15:22]

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

http://codereview.chromium.org/9283022/diff/17001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1809: render_view_->Send(new
PpapiHostMsg_PPBTCPServerSocket_Create(
Supporting these in the renderer is very problematic. Justin will follow up, we
also need to figure out what to do about the existing socket sending stuff.

http://codereview.chromium.org/9283022/diff/17001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/17001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:32: int32_t Listen([in]
PP_Resource tcp_server_socket,
These functions need documentation.

http://codereview.chromium.org/9283022/diff/17001/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_server_socket_private_impl.h (right):

http://codereview.chromium.org/9283022/diff/17001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_server_socket_private_impl.h:20: class
PPAPI_SHARED_EXPORT TCPServerSocketPrivateImpl
This class/file should be PPB_TCPServerSocket_Shared (probably OK to omit
"private" but it should have "Shared"

[Dmitry Polukhin, 2012-02-07 06:45:49]

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

http://codereview.chromium.org/9283022/diff/17001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1809: render_view_->Send(new
PpapiHostMsg_PPBTCPServerSocket_Create(
On 2012/02/07 00:15:22, brettw wrote:
> Supporting these in the renderer is very problematic. Justin will follow up,
we
> also need to figure out what to do about the existing socket sending stuff.

I think we need to transform "create" methods to async methods with callback. It
will also simplify check what pages/renders can use socket APIs. Now this check
happens in render but it needs to be moved to browser process. Inside browser
process it is easy to do on UI thread but create messages are processed to IO
thread. With async methods it will be easy to send message from IO to UI thread.

[ygorshenin1, 2012-02-07 13:42:59]

PTAL

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

http://codereview.chromium.org/9283022/diff/17001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1809: render_view_->Send(new
PpapiHostMsg_PPBTCPServerSocket_Create(
Fixed: PPB_TCPServerSocke_Private::Create method is asynchronous now.

On 2012/02/07 06:45:50, Dmitry Polukhin wrote:
> On 2012/02/07 00:15:22, brettw wrote:
> > Supporting these in the renderer is very problematic. Justin will follow up,
> we
> > also need to figure out what to do about the existing socket sending stuff.
> 
> I think we need to transform "create" methods to async methods with callback.
It
> will also simplify check what pages/renders can use socket APIs. Now this
check
> happens in render but it needs to be moved to browser process. Inside browser
> process it is easy to do on UI thread but create messages are processed to IO
> thread. With async methods it will be easy to send message from IO to UI
thread.

http://codereview.chromium.org/9283022/diff/17001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/17001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:32: int32_t Listen([in]
PP_Resource tcp_server_socket,
On 2012/02/07 00:15:22, brettw wrote:
> These functions need documentation.

Done.

http://codereview.chromium.org/9283022/diff/17001/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_server_socket_private_impl.h (right):

http://codereview.chromium.org/9283022/diff/17001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_server_socket_private_impl.h:20: class
PPAPI_SHARED_EXPORT TCPServerSocketPrivateImpl
On 2012/02/07 00:15:22, brettw wrote:
> This class/file should be PPB_TCPServerSocket_Shared (probably OK to omit
> "private" but it should have "Shared"

Done.

[brettw, 2012-02-09 00:13:48]

Thanks for the nice patch. Sorry I took so long to review, it was quite large. I
mostly have minor comments. I'm assuming we'll resolve the renderer message
security issue at the same time as we do for the regular sockets so don't need
to block this patch on that work.

http://codereview.chromium.org/9283022/diff/32007/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9283022/diff/32007/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:355: 
No blank lines in a block of functions implementing an interface (so everything
is grouped).

http://codereview.chromium.org/9283022/diff/32007/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:439: 
I wouldn't do blank lines here either.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:112: if (result !=
net::OK)
Style nit: this needs {} since the other arm has them.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:145: delete socket;
The ownership model of this is pretty complex. I think passing a scoped ptr for
the socket in AcceptCallback::Run (using the new "Pass" semantics as per a
chromium-dev thread a few weeks ago) would make it clear that ownership is
transferred. Then you can just put the socket in a scoped_ptr as soon as you
make it above, and delete this line.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_socket.h (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_socket.h:36: // Used for creation
already connected sockets.
Ditto with passing ownership of the socket here.

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:27: [in]
PP_CompletionCallback callback);
We don't use callbacks in "create" functions because then the caller has no way
to cancel the callback by destroying the object. If you need async creation,
either put it in Listen() or add another function you have to call.

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:16: :
Resource(instance), instance_(instance), state_(NOT_INITIALIZED) {
Can you initialize tcp_socket_buffer_ to null in the two constructors?

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h:23: public:
Style: public/protected should be indented one space in this class.

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h:74: CLOSED,
The nacl toolchain (which we'll eventually compile this with) will complain
about this comma, can you remove it?

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h:80: PP_Instance
instance_;
The resource base class will keep track of the instance (pp_instance()), so you
should remove this.

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
File ppapi/tests/test_tcp_server_socket_private.cc (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.cc:39: if
(!tcp_server_socket_private_interface_)
Nit: need {}

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource_creation_impl.cc:260: int32_t
ResourceCreationImpl::CreateTCPServerSocketPrivate(
When you make creation synchronous, you should return PP_Resource from this.

[yzshen1, 2012-02-09 02:31:50]

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:537: return;
|socket| is leaked here.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:12: #include
"content/public/browser/browser_thread.h"
duplicate.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:73: int result =
socket_->Accept(
I think Accept() will DCHECK if the previous Accept() hasn't completed. You may
want to check that currently state_ != ACCEPT_IN_PROGRESS before calling it.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:134:
BrowserThread::PostTask(BrowserThread::IO,
The logic related to accept_callback_ is a little bit complex. How about:
- Change PepperMessageFilter::OnTCPServerSocketAcceptComplete to, say,
AddAcceptedTCPSocket(), and call it directly instead of using Callback&PostTask.
- And then send AcceptACK message here.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.h (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.h:21: using
base::Callback;
Don't use 'using' in header files.

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

http://codereview.chromium.org/9283022/diff/35001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1676:
tcp_sockets_.AddWithID(socket, socket_id);
TCPSocketRegister?

http://codereview.chromium.org/9283022/diff/35001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1711: void
PepperPluginDelegateImpl::TCPSocketRegister(
Please use RegisterTCPSocket (Verb+Noun).

http://codereview.chromium.org/9283022/diff/35001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9283022/diff/35001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:499:
uninitialized_tcp_server_sockets_;
wrong indent.

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:58: * Cancels all pending
callbacks reporting PP_Error_Aborted and
PP_ERROR_ABORTED, please.

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:93:
PostAbortAndClearIfNecessary(&accept_callback_);
Do you need to post abort for initialize_callback_?

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
File ppapi/tests/test_tcp_server_socket_private.cc (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.cc:179: error_message =
SyncConnect(socket, host, port);
Is it possible that this will go on indefinitely when something goes wrong?

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.cc:252: PP_Resource server_socket;
Please note that |server_socket| is leaked.
PP_Resource is ref-counted. When you create a resource, the system will add 1
ref for you. You have to release it after you finish using it.

(You also need to take care of the ref count of PP_Resource returned by
Accept().)

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
File ppapi/tests/test_tcp_server_socket_private.h (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.h:20: enum { kLocalPortListenSync =
20000,
Is it necessary to be in the public section, or in the .h file?

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:346: if (strcmp(name,
PPB_TCPSERVERSOCKET_PRIVATE_INTERFACE) == 0)
I think you could add entry in interfaces_ppb_private.h instead of adding it
here.

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/ppb_tc...
File webkit/plugins/ppapi/ppb_tcp_server_socket_private_impl.cc (right):

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_server_socket_private_impl.cc:48: PP_Resource
tcp_socket =
nit: Why not directly *tcp_socket_buffer_ = ...?

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/ppb_tc...
File webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc (right):

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc:27: return NULL;
To be consistent, please use 0 instead of NULL. (and line 43)

[ygorshenin1, 2012-02-09 15:00:10]

PTAL

http://codereview.chromium.org/9283022/diff/32007/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9283022/diff/32007/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:355: 
On 2012/02/09 00:13:48, brettw wrote:
> No blank lines in a block of functions implementing an interface (so
everything
> is grouped).

Done.

http://codereview.chromium.org/9283022/diff/32007/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:439: 
On 2012/02/09 00:13:48, brettw wrote:
> I wouldn't do blank lines here either.

Done.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:537: return;
On 2012/02/09 02:31:50, yzshen1 wrote:
> |socket| is leaked here.

Done.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:12: #include
"content/public/browser/browser_thread.h"
On 2012/02/09 02:31:50, yzshen1 wrote:
> duplicate.

Done.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:73: int result =
socket_->Accept(
I added a state_ check before DCHECK, but didn't understand why "Accept() will
DCHECK if the previous Accept() hasn't completed". Can You explain it?

On 2012/02/09 02:31:50, yzshen1 wrote:
> I think Accept() will DCHECK if the previous Accept() hasn't completed. You
may
> want to check that currently state_ != ACCEPT_IN_PROGRESS before calling it.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:112: if (result !=
net::OK)
On 2012/02/09 00:13:48, brettw wrote:
> Style nit: this needs {} since the other arm has them.

Done.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:134:
BrowserThread::PostTask(BrowserThread::IO,
On 2012/02/09 02:31:50, yzshen1 wrote:
> The logic related to accept_callback_ is a little bit complex. How about:
> - Change PepperMessageFilter::OnTCPServerSocketAcceptComplete to, say,
> AddAcceptedTCPSocket(), and call it directly instead of using
Callback&PostTask.
> - And then send AcceptACK message here.

Done.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:145: delete socket;
On 2012/02/09 00:13:48, brettw wrote:
> The ownership model of this is pretty complex. I think passing a scoped ptr
for
> the socket in AcceptCallback::Run (using the new "Pass" semantics as per a
> chromium-dev thread a few weeks ago) would make it clear that ownership is
> transferred. Then you can just put the socket in a scoped_ptr as soon as you
> make it above, and delete this line.

OK, now socket ownership is passed to PepperMessageFilter::AddAcceptedTCPSocket.
If this is not what you wanted, please, point me out this thread.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.h (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.h:21: using
base::Callback;
On 2012/02/09 02:31:50, yzshen1 wrote:
> Don't use 'using' in header files.

Done.

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_socket.h (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_socket.h:36: // Used for creation
already connected sockets.
On 2012/02/09 00:13:48, brettw wrote:
> Ditto with passing ownership of the socket here.

Done.

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

http://codereview.chromium.org/9283022/diff/35001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1676:
tcp_sockets_.AddWithID(socket, socket_id);
On 2012/02/09 02:31:50, yzshen1 wrote:
> TCPSocketRegister?

Done.

http://codereview.chromium.org/9283022/diff/35001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1711: void
PepperPluginDelegateImpl::TCPSocketRegister(
On 2012/02/09 02:31:50, yzshen1 wrote:
> Please use RegisterTCPSocket (Verb+Noun).

Done.

http://codereview.chromium.org/9283022/diff/35001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9283022/diff/35001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:499:
uninitialized_tcp_server_sockets_;
On 2012/02/09 02:31:50, yzshen1 wrote:
> wrong indent.

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:27: [in]
PP_CompletionCallback callback);
On 2012/02/09 00:13:48, brettw wrote:
> We don't use callbacks in "create" functions because then the caller has no
way
> to cancel the callback by destroying the object. If you need async creation,
> either put it in Listen() or add another function you have to call.

We decided that separate "Init" function will look unclear for users, the same
is for implementing initialization logic in Listen call. So, Create function
will return PP_Resource immediately, but proper initialization of corresponding
PPB_TCPServerSocket_Private will be completed only when callback is invoked.

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:58: * Cancels all pending
callbacks reporting PP_Error_Aborted and
On 2012/02/09 02:31:50, yzshen1 wrote:
> PP_ERROR_ABORTED, please.

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:16: :
Resource(instance), instance_(instance), state_(NOT_INITIALIZED) {
On 2012/02/09 00:13:48, brettw wrote:
> Can you initialize tcp_socket_buffer_ to null in the two constructors?

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:93:
PostAbortAndClearIfNecessary(&accept_callback_);
On 2012/02/09 02:31:50, yzshen1 wrote:
> Do you need to post abort for initialize_callback_?

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h:23: public:
On 2012/02/09 00:13:48, brettw wrote:
> Style: public/protected should be indented one space in this class.

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h:74: CLOSED,
On 2012/02/09 00:13:48, brettw wrote:
> The nacl toolchain (which we'll eventually compile this with) will complain
> about this comma, can you remove it?

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h:80: PP_Instance
instance_;
On 2012/02/09 00:13:48, brettw wrote:
> The resource base class will keep track of the instance (pp_instance()), so
you
> should remove this.

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
File ppapi/tests/test_tcp_server_socket_private.cc (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.cc:39: if
(!tcp_server_socket_private_interface_)
On 2012/02/09 00:13:48, brettw wrote:
> Nit: need {}

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.cc:179: error_message =
SyncConnect(socket, host, port);
Alas, yes. If |server_socket| is in listening mode, but Accept isn't called,
then Connect will go in infinitely.

On 2012/02/09 02:31:50, yzshen1 wrote:
> Is it possible that this will go on indefinitely when something goes wrong?

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.cc:252: PP_Resource server_socket;
On 2012/02/09 02:31:50, yzshen1 wrote:
> Please note that |server_socket| is leaked.
> PP_Resource is ref-counted. When you create a resource, the system will add 1
> ref for you. You have to release it after you finish using it.
> 
> (You also need to take care of the ref count of PP_Resource returned by
> Accept().)

Done.

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
File ppapi/tests/test_tcp_server_socket_private.h (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.h:20: enum { kLocalPortListenSync =
20000,
No, it's not necessary. These constants are moved to .cc file.

On 2012/02/09 02:31:50, yzshen1 wrote:
> Is it necessary to be in the public section, or in the .h file?

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:346: if (strcmp(name,
PPB_TCPSERVERSOCKET_PRIVATE_INTERFACE) == 0)
This is possible only after addition of PPB_TCPServerSocket_Private_Proxy class.
Maybe it's better to do that in another CL?

On 2012/02/09 02:31:50, yzshen1 wrote:
> I think you could add entry in interfaces_ppb_private.h instead of adding it
> here.

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/ppb_tc...
File webkit/plugins/ppapi/ppb_tcp_server_socket_private_impl.cc (right):

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_server_socket_private_impl.cc:48: PP_Resource
tcp_socket =
On 2012/02/09 02:31:50, yzshen1 wrote:
> nit: Why not directly *tcp_socket_buffer_ = ...?

Done.

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/ppb_tc...
File webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc (right):

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc:27: return NULL;
On 2012/02/09 02:31:50, yzshen1 wrote:
> To be consistent, please use 0 instead of NULL. (and line 43)

Done.

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/9283022/diff/35001/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource_creation_impl.cc:260: int32_t
ResourceCreationImpl::CreateTCPServerSocketPrivate(
On 2012/02/09 00:13:48, brettw wrote:
> When you make creation synchronous, you should return PP_Resource from this.

Done.

http://codereview.chromium.org/9283022/diff/48001/ppapi/tests/test_tcp_server...
File ppapi/tests/test_tcp_server_socket_private.cc (right):

http://codereview.chromium.org/9283022/diff/48001/ppapi/tests/test_tcp_server...
ppapi/tests/test_tcp_server_socket_private.cc:197: // ports.
By unknown reasons even after TCPServerSocketLibevent::Close call port isn't
available for a while, so it's impossible now to use single port for all tests.
And we can't use the same hack as used for PPB_UDPSocket_Private tests. We can
use predefined ports here, but after addition of NaCl tests and out-of-process
tests (if needed) number of predefined ports will be large, so this approach
doesn't scale. Temporary solution is to select first available port.

[yzshen1, 2012-02-09 17:36:38]

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:73: int result =
socket_->Accept(
I mean:
http://code.google.com/codesearch#OAMlx_jo-ck/src/net/socket/tcp_server_socke...

On 2012/02/09 15:00:10, ygorshenin1 wrote:
> I added a state_ check before DCHECK, but didn't understand why "Accept() will
> DCHECK if the previous Accept() hasn't completed". Can You explain it?
> 
> On 2012/02/09 02:31:50, yzshen1 wrote:
> > I think Accept() will DCHECK if the previous Accept() hasn't completed. You
> may
> > want to check that currently state_ != ACCEPT_IN_PROGRESS before calling it.
>

[brettw, 2012-02-09 18:19:11]

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:27: [in]
PP_CompletionCallback callback);
I kind of know what you mean, but nothing else works like this, and I think it's
important we're consistent.

Are you sure we can't fold this initialization into Listen()? It seems like
every user of this API would do Create() and then Listen(), and Listen() is
already asynchronous. Folding this into Listen() would make it much easier to
program for since you now get a single event when it's ready rather than having
to write a state machine to start listening.

[ygorshenin1, 2012-02-09 18:44:27]

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:27: [in]
PP_CompletionCallback callback);
Well, but how to distinguish errors in socket initialization and errors in
listen?

On 2012/02/09 18:19:11, brettw wrote:
> I kind of know what you mean, but nothing else works like this, and I think
it's
> important we're consistent.
> 
> Are you sure we can't fold this initialization into Listen()? It seems like
> every user of this API would do Create() and then Listen(), and Listen() is
> already asynchronous. Folding this into Listen() would make it much easier to
> program for since you now get a single event when it's ready rather than
having
> to write a state machine to start listening.

[brettw, 2012-02-09 18:52:33]

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:27: [in]
PP_CompletionCallback callback);
That's true. What errors are you concerned about differentiating?

[ygorshenin1, 2012-02-09 19:13:07]

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:27: [in]
PP_CompletionCallback callback);
New socket can't be initialized if socket table is full. Already initialized
socket can't bind to local address if this address is busy. In both cases
user'll see PP_ERROR_FAILED as result.

On 2012/02/09 18:52:33, brettw wrote:
> That's true. What errors are you concerned about differentiating?

[brettw, 2012-02-09 19:18:54]

On Thu, Feb 9, 2012 at 11:13 AM,  <ygorshenin@chromium.org> wrote:
>
>
http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
> File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):
>
>
http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
> ppapi/api/private/ppb_tcp_server_socket_private.idl:27: [in]
> PP_CompletionCallback callback);
> New socket can't be initialized if socket table is full. Already
> initialized socket can't bind to local address if this address is busy.
> In both cases user'll see PP_ERROR_FAILED as result.

Could we return PP_ERROR_NOSPACE if the socket table is full to
differentiate these cases?

Also, we should have a description of these error modes in the
documentation for the functions, I think it would be useful for
developers.

Brett

[ygorshenin1, 2012-02-10 13:27:40]

PTAL

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:73: int result =
socket_->Accept(
OK, but in this case check that state_ == LISTENING is enough, right?

On 2012/02/09 17:36:39, yzshen1 wrote:
> I mean:
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/net/socket/tcp_server_socke...
> 
> On 2012/02/09 15:00:10, ygorshenin1 wrote:
> > I added a state_ check before DCHECK, but didn't understand why "Accept()
will
> > DCHECK if the previous Accept() hasn't completed". Can You explain it?
> > 
> > On 2012/02/09 02:31:50, yzshen1 wrote:
> > > I think Accept() will DCHECK if the previous Accept() hasn't completed.
You
> > may
> > > want to check that currently state_ != ACCEPT_IN_PROGRESS before calling
it.
> > 
>

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/35001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:27: [in]
PP_CompletionCallback callback);
On 2012/02/09 19:13:07, ygorshenin1 wrote:
> New socket can't be initialized if socket table is full. Already initialized
> socket can't bind to local address if this address is busy. In both cases
> user'll see PP_ERROR_FAILED as result.
> 
> On 2012/02/09 18:52:33, brettw wrote:
> > That's true. What errors are you concerned about differentiating?
> 

Done.

[brettw, 2012-02-10 21:43:32]

Just some comments that I think are a result of the last change we made.

We also need to figure out how to get this secured like the client sockets
dpolukhin is working on a patch for. I'll let you guys figure out the best order
to land these two patches.

http://codereview.chromium.org/9283022/diff/50005/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/50005/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:36: * accept incoming
connections or in the case of failure.
Can you add the description of the failures modes that you sent me in the
comments? That was very useful. We should describe what each possible return
value means.

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:48: if (state_ ==
NOT_INITIALIZED || state_ == INITIALIZING) {
I don't see you using the INITIALIZING state anywhere?

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:55: if (state_ ==
NOT_INITIALIZED) {
This should also be able to be simplified if we can remove the INITIALIZING
state.

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:61: } else {  //
state_ == BEFORE_LISTENING
I also don't understand this case. Can't you only call Listen in one state? It
seems like you should be calling it once after resource creation and any other
usage should fail.

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:96:
PostAbortAndClearIfNecessary(&listen_callback_);
Not necessary, but did you consider just inlining the two lines of
"PostAbortAndClearIfNecessary" here? It would actually be fewer lines of code,
and I'm not sure that having the separate function makes it clearer. I realize
you copied this from the client socket, and it was there partially for
historical reasons, and there may also be more callers.

http://codereview.chromium.org/9283022/diff/50005/ppapi/thunk/ppb_tcp_server_...
File ppapi/thunk/ppb_tcp_server_socket_private_thunk.cc (right):

http://codereview.chromium.org/9283022/diff/50005/ppapi/thunk/ppb_tcp_server_...
ppapi/thunk/ppb_tcp_server_socket_private_thunk.cc:21: if (enter.failed()) {
Style nit: no {} here for consistency.

[yzshen1, 2012-02-10 22:40:14]

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/35001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:73: int result =
socket_->Accept(
Ah, you are right. Thanks for pointing it out.
On 2012/02/10 13:27:40, ygorshenin1 wrote:
> OK, but in this case check that state_ == LISTENING is enough, right?
> 
> On 2012/02/09 17:36:39, yzshen1 wrote:
> > I mean:
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/net/socket/tcp_server_socke...
> > 
> > On 2012/02/09 15:00:10, ygorshenin1 wrote:
> > > I added a state_ check before DCHECK, but didn't understand why "Accept()
> will
> > > DCHECK if the previous Accept() hasn't completed". Can You explain it?
> > > 
> > > On 2012/02/09 02:31:50, yzshen1 wrote:
> > > > I think Accept() will DCHECK if the previous Accept() hasn't completed.
> You
> > > may
> > > > want to check that currently state_ != ACCEPT_IN_PROGRESS before calling
> it.
> > > 
> > 
>

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:109:
IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBTCPServerSocket_StopListening,
If both Destroy and StopListening are handled in the same way, can we use one
message instead of two?

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.h:62: net::StreamSocket*
socket);
Please comment about the ownership of |socket|.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:109:
net::StreamSocket* socket = socket_buffer_.release();
I would suggest to transfer socket_buffer_ to a temp scoped_ptr<>. And then
release it when calling AddAcceptedTCPSocket(). It is harder to leak object that
way.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:124:
SendAcceptACKError();
If you SendAcceptACKError(), you don't need to proceed to AddAcceptedTCPSocket()
and Send(), right?
(Please note that you still need to set state_ = LISTENING.)

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:130: if
(accepted_socket_id != 0) {
Please note that you still need to send an error response if accepted_socket_id
== 0.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.h (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.h:20: using
net::StreamSocket;
You missed this one.

http://codereview.chromium.org/9283022/diff/50005/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/50005/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:20: * Allocates a TCP server
socket resource. Also, starts lazy
I think you could remove the 2nd and 3rd sentence. It seems to be an
implementation detail that the user don't need to care.

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:40: if (state_ !=
NOT_INITIALIZED &&
Why we have to init and listen as two separate steps?
If I understand the API spec correctly, we don't allow a second call to Listen
(if it succeeds), not even after StopListening(). 

Then we can simply do Init&Listen in one round trip to the brower. The state
machine will be much simpler. Right?

One thing to consider: if we do init&listen together, we should not treat
uninitialized tcp sockets as a FIFO queue any more. The order matters now.
One possible solution is to generate a temp_socket_id at the renderer side, send
it in the Listen message to the browser, the browser then does init&listen and
sends (temp_socket_id, real_socket_id) back. Then we match this response to an
uninitialized socket object.

[ygorshenin1, 2012-02-13 14:59:45]

Many thanks, PTAL.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:109:
IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBTCPServerSocket_StopListening,
On 2012/02/10 22:40:15, yzshen1 wrote:
> If both Destroy and StopListening are handled in the same way, can we use one
> message instead of two?

Done. I think, Destroy is a more appropriate name as this message can be sent
even if Listen isn't called.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.h:62: net::StreamSocket*
socket);
On 2012/02/10 22:40:15, yzshen1 wrote:
> Please comment about the ownership of |socket|.

Done.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:109:
net::StreamSocket* socket = socket_buffer_.release();
On 2012/02/10 22:40:15, yzshen1 wrote:
> I would suggest to transfer socket_buffer_ to a temp scoped_ptr<>. And then
> release it when calling AddAcceptedTCPSocket(). It is harder to leak object
that
> way.

Done.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:124:
SendAcceptACKError();
On 2012/02/10 22:40:15, yzshen1 wrote:
> If you SendAcceptACKError(), you don't need to proceed to
AddAcceptedTCPSocket()
> and Send(), right?
> (Please note that you still need to set state_ = LISTENING.)

Done.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:130: if
(accepted_socket_id != 0) {
On 2012/02/10 22:40:15, yzshen1 wrote:
> Please note that you still need to send an error response if
accepted_socket_id
> == 0.

Done.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.h (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.h:20: using
net::StreamSocket;
On 2012/02/10 22:40:15, yzshen1 wrote:
> You missed this one.

Done.

http://codereview.chromium.org/9283022/diff/50005/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/50005/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:20: * Allocates a TCP server
socket resource. Also, starts lazy
On 2012/02/10 22:40:15, yzshen1 wrote:
> I think you could remove the 2nd and 3rd sentence. It seems to be an
> implementation detail that the user don't need to care.

Done.

http://codereview.chromium.org/9283022/diff/50005/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:36: * accept incoming
connections or in the case of failure.
On 2012/02/10 21:43:32, brettw wrote:
> Can you add the description of the failures modes that you sent me in the
> comments? That was very useful. We should describe what each possible return
> value means.

Done.

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:40: if (state_ !=
NOT_INITIALIZED &&
Because of efficiency. When user creates socket, initialization process is
started, so, when user decided to call Listen, socket is initialized. May be
this is useless because Listen called only once in most cases. Current state
machine is non-obvious and bug-prone so we decided to combine init and listen
processes.

On 2012/02/10 22:40:15, yzshen1 wrote:
> Why we have to init and listen as two separate steps?
> If I understand the API spec correctly, we don't allow a second call to Listen
> (if it succeeds), not even after StopListening(). 
> 
> Then we can simply do Init&Listen in one round trip to the brower. The state
> machine will be much simpler. Right?
> 
> One thing to consider: if we do init&listen together, we should not treat
> uninitialized tcp sockets as a FIFO queue any more. The order matters now.
> One possible solution is to generate a temp_socket_id at the renderer side,
send
> it in the Listen message to the browser, the browser then does init&listen and
> sends (temp_socket_id, real_socket_id) back. Then we match this response to an
> uninitialized socket object.
>

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:48: if (state_ ==
NOT_INITIALIZED || state_ == INITIALIZING) {
It was used in webkit/plugin/ppapi/ppb_tcp_server_socket_private.cc. But now
it's not used.

On 2012/02/10 21:43:32, brettw wrote:
> I don't see you using the INITIALIZING state anywhere?

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:55: if (state_ ==
NOT_INITIALIZED) {
On 2012/02/10 21:43:32, brettw wrote:
> This should also be able to be simplified if we can remove the INITIALIZING
> state.

Done.

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:61: } else {  //
state_ == BEFORE_LISTENING
OK, code is simplified.

On 2012/02/10 21:43:32, brettw wrote:
> I also don't understand this case. Can't you only call Listen in one state? It
> seems like you should be calling it once after resource creation and any other
> usage should fail.

http://codereview.chromium.org/9283022/diff/50005/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:96:
PostAbortAndClearIfNecessary(&listen_callback_);
On 2012/02/10 21:43:32, brettw wrote:
> Not necessary, but did you consider just inlining the two lines of
> "PostAbortAndClearIfNecessary" here? It would actually be fewer lines of code,
> and I'm not sure that having the separate function makes it clearer. I realize
> you copied this from the client socket, and it was there partially for
> historical reasons, and there may also be more callers.

Done.

http://codereview.chromium.org/9283022/diff/50005/ppapi/thunk/ppb_tcp_server_...
File ppapi/thunk/ppb_tcp_server_socket_private_thunk.cc (right):

http://codereview.chromium.org/9283022/diff/50005/ppapi/thunk/ppb_tcp_server_...
ppapi/thunk/ppb_tcp_server_socket_private_thunk.cc:21: if (enter.failed()) {
On 2012/02/10 21:43:32, brettw wrote:
> Style nit: no {} here for consistency.

Done.

[brettw, 2012-02-13 22:19:49]

lgtm

[yzshen1, 2012-02-14 00:42:04]

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:109:
IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBTCPServerSocket_StopListening,
[nit, optional]
Agree. I meant to keep Destroy instead of StopListening.

On 2012/02/13 14:59:46, ygorshenin1 wrote:
> On 2012/02/10 22:40:15, yzshen1 wrote:
> > If both Destroy and StopListening are handled in the same way, can we use
one
> > message instead of two?
> 
> Done. I think, Destroy is a more appropriate name as this message can be sent
> even if Listen isn't called.

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

http://codereview.chromium.org/9283022/diff/50005/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:849: // PPB_TCPServerSocket_Private.
Please keep this comment.

http://codereview.chromium.org/9283022/diff/23035/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9283022/diff/23035/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.h:59: // Adds already
accepted socket to the internal TCP sockets
nit: Please make every line as close to 80 chars as possible.

http://codereview.chromium.org/9283022/diff/23035/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/23035/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:82: real_socket_id_,
Can we send 0 instead of |real_socket_id_| to the render/plugin?

http://codereview.chromium.org/9283022/diff/23035/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/23035/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:35: * PP_ERROR_NOSPACE
socket if the socket table is full, or
Please remove the first 'socket'.

nit: 'socket table is full' is not clear enough, because we never mention what
is a socket table.

http://codereview.chromium.org/9283022/diff/23035/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/23035/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:8: #include <cstring>
why do we need cstring?

http://codereview.chromium.org/9283022/diff/23035/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:79: if (state_ !=
LISTENING)
(1) This is incorrect according to the API spec. Consider the following example:
- Listen() is called;
- Before it completes, StopListening() is called.

With your current implementation, StopListening() doesn't take effect, so the
pending listen will complete in the future. (Very likely it won't be
PP_ERROR_ABORTED.)

(2) One more thing you need to consider: You need to handle the case that the
tcp server socket goes away when it is still waiting for ListenACK.

One way to do it: in TCPServerSocketStopListening, if you already have a
real_socket_id, then send a StopListening msg to the browser; otherwise, clean
up uninitialized_tcp_server_socket_.

In OnTCPServerSocketListenACK, if temp_socket_id_ cannot be found in the
uninitialized map, send a StopListening msg to the browser.

[ygorshenin1, 2012-02-14 09:34:44]

Thanks, PTAL.

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9283022/diff/50005/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:109:
IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBTCPServerSocket_StopListening,
On 2012/02/14 00:42:04, yzshen1 wrote:
> [nit, optional]
> Agree. I meant to keep Destroy instead of StopListening.
> 
> On 2012/02/13 14:59:46, ygorshenin1 wrote:
> > On 2012/02/10 22:40:15, yzshen1 wrote:
> > > If both Destroy and StopListening are handled in the same way, can we use
> one
> > > message instead of two?
> > 
> > Done. I think, Destroy is a more appropriate name as this message can be
sent
> > even if Listen isn't called.
> 

Done.

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

http://codereview.chromium.org/9283022/diff/50005/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:849: // PPB_TCPServerSocket_Private.
On 2012/02/14 00:42:04, yzshen1 wrote:
> Please keep this comment.

Done.

http://codereview.chromium.org/9283022/diff/23035/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9283022/diff/23035/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.h:59: // Adds already
accepted socket to the internal TCP sockets
On 2012/02/14 00:42:04, yzshen1 wrote:
> nit: Please make every line as close to 80 chars as possible.

Done.

http://codereview.chromium.org/9283022/diff/23035/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_server_socket.cc (right):

http://codereview.chromium.org/9283022/diff/23035/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_server_socket.cc:82: real_socket_id_,
Yes, You're right, it's better to send zero in the case of failure.

On 2012/02/14 00:42:04, yzshen1 wrote:
> Can we send 0 instead of |real_socket_id_| to the render/plugin?

http://codereview.chromium.org/9283022/diff/23035/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/23035/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:35: * PP_ERROR_NOSPACE
socket if the socket table is full, or
On 2012/02/14 00:42:04, yzshen1 wrote:
> Please remove the first 'socket'.
> 
> nit: 'socket table is full' is not clear enough, because we never mention what
> is a socket table.

Done.

http://codereview.chromium.org/9283022/diff/23035/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/23035/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:8: #include <cstring>
Thanks, You're right. <cstring> isn't needed here. Inclusion of
"base/message_loop.h" also deleted.

On 2012/02/14 00:42:04, yzshen1 wrote:
> why do we need cstring?

[yzshen1, 2012-02-14 18:26:30]

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

http://codereview.chromium.org/9283022/diff/35054/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2119: render_view_->Send(
nit: it is good to have a comment why socket can be NULL.

http://codereview.chromium.org/9283022/diff/35054/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/35054/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:77: if (state_ !=
LISTENING)
You haven't fixed this issue that I mentioned before.

"This is incorrect according to the API spec. Consider the following example:
- Listen() is called;
- Before it completes, StopListening() is called.

With your current implementation, StopListening() doesn't take effect, so the
pending listen will complete in the future. (Very likely it won't be
PP_ERROR_ABORTED.)
"

[ygorshenin1, 2012-02-14 20:27:43]

PTAL

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

http://codereview.chromium.org/9283022/diff/35054/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2119: render_view_->Send(
On 2012/02/14 18:26:31, yzshen1 wrote:
> nit: it is good to have a comment why socket can be NULL.

Done.

http://codereview.chromium.org/9283022/diff/35054/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/35054/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:77: if (state_ !=
LISTENING)
On 2012/02/14 18:26:31, yzshen1 wrote:
> You haven't fixed this issue that I mentioned before.
> 
> "This is incorrect according to the API spec. Consider the following example:
> - Listen() is called;
> - Before it completes, StopListening() is called.
> 
> With your current implementation, StopListening() doesn't take effect, so the
> pending listen will complete in the future. (Very likely it won't be
> PP_ERROR_ABORTED.)
> "

Done.

[yzshen1, 2012-02-14 20:32:18]

LGTM with nits:

http://codereview.chromium.org/9283022/diff/39061/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/39061/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:76: void
PPB_TCPServerSocket_Shared::StopListening() {
You could return early:

if (state_ == CLOSED)
  return;

[ygorshenin1, 2012-02-14 20:45:18]

Many thanks!

http://codereview.chromium.org/9283022/diff/39061/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc (right):

http://codereview.chromium.org/9283022/diff/39061/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.cc:76: void
PPB_TCPServerSocket_Shared::StopListening() {
On 2012/02/14 20:32:18, yzshen1 wrote:
> You could return early:
> 
> if (state_ == CLOSED)
>   return;

Done.

[ygorshenin1, 2012-02-15 10:49:15]

+ jam

[jam, 2012-02-15 17:03:55]

On 2012/02/15 10:49:15, ygorshenin1 wrote:
> + jam

why? brettw is an owner in all the content files too

[ygorshenin1, 2012-02-16 13:29:47]

+ Darin

Need an OWNER review for webkit/glue/webkit_glue.gypi.

[darin (slow to review), 2012-02-16 19:02:38]

http://codereview.chromium.org/9283022/diff/25166/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/25166/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:60: void StopListening([in]
PP_Resource tcp_server_socket);
Is there a reason not to call this Close?

[viettrungluu, 2012-02-16 19:10:55]

http://codereview.chromium.org/9283022/diff/25166/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_server_socket_private.idl (right):

http://codereview.chromium.org/9283022/diff/25166/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_server_socket_private.idl:60: void StopListening([in]
PP_Resource tcp_server_socket);
On 2012/02/16 19:02:39, darin wrote:
> Is there a reason not to call this Close?

Symmetry with Listen()? (Close() usually goes with Open(). I guess we haven't
been super-consistent with this.)

[darin (slow to review), 2012-02-16 19:30:04]

On Thu, Feb 16, 2012 at 11:10 AM, <viettrungluu@chromium.org> wrote:

>
> http://codereview.chromium.**org/9283022/diff/25166/ppapi/**
>
api/private/ppb_tcp_server_**socket_private.idl<http://codereview.chromium.org/9283022/diff/25166/ppapi/api/private/ppb_tcp_server_socket_private.idl>
> File ppapi/api/private/ppb_tcp_**server_socket_private.idl (right):
>
> http://codereview.chromium.**org/9283022/diff/25166/ppapi/**
>
api/private/ppb_tcp_server_**socket_private.idl#newcode60<http://codereview.chromium.org/9283022/diff/25166/ppapi/api/private/ppb_tcp_server_socket_private.idl#newcode60>
> ppapi/api/private/ppb_tcp_**server_socket_private.idl:60: void
> StopListening([in] PP_Resource tcp_server_socket);
> On 2012/02/16 19:02:39, darin wrote:
>
>> Is there a reason not to call this Close?
>>
>
> Symmetry with Listen()? (Close() usually goes with Open(). I guess we
> haven't been super-consistent with this.)
>

Agreed.  In the Pepper API, we usually do tie Close to Open.  I was just
thinking that "close()" is the function you call when using berkeley
sockets :-)


>
>
http://codereview.chromium.**org/9283022/<http://codereview.chromium.org/9283...
>

[Dmitry Polukhin, 2012-02-16 19:49:54]

On 2012/02/16 19:30:04, darin wrote:
> Agreed.  In the Pepper API, we usually do tie Close to Open.  I was just
> thinking that "close()" is the function you call when using berkeley
> sockets :-)

For client TCP socket we have Connect/Disconnect and UDP sockets Bind/Close.
Should we rename StopListening to Close?

[darin (slow to review), 2012-02-16 22:06:09]

Is there something analogous to the shutdown BSD socket call?

-Darin


On Thu, Feb 16, 2012 at 11:49 AM, <dpolukhin@chromium.org> wrote:

> On 2012/02/16 19:30:04, darin wrote:
>
>> Agreed.  In the Pepper API, we usually do tie Close to Open.  I was just
>> thinking that "close()" is the function you call when using berkeley
>> sockets :-)
>>
>
> For client TCP socket we have Connect/Disconnect and UDP sockets
> Bind/Close.
> Should we rename StopListening to Close?
>
>
http://codereview.chromium.**org/9283022/<http://codereview.chromium.org/9283...
>

[ygorshenin1, 2012-02-17 10:15:43]

Semantic of "shutdown" system call is to close a socket with guaranteed delivery
of data, that has been sent via this socket. So, "Shutdown" is more appropriate
for PPB_TCPSocket_Private (instead of "Close" method), not for
PPB_TCPServerSocket_Private.

So, finally, should we rename "StopListening" to "Close"?

On 2012/02/16 22:06:09, darin wrote:
> Is there something analogous to the shutdown BSD socket call?
> 
> -Darin
> 
> 
> On Thu, Feb 16, 2012 at 11:49 AM, <mailto:dpolukhin@chromium.org> wrote:
> 
> > On 2012/02/16 19:30:04, darin wrote:
> >
> >> Agreed.  In the Pepper API, we usually do tie Close to Open.  I was just
> >> thinking that "close()" is the function you call when using berkeley
> >> sockets :-)
> >>
> >
> > For client TCP socket we have Connect/Disconnect and UDP sockets
> > Bind/Close.
> > Should we rename StopListening to Close?
> >
> >
>
http://codereview.chromium.**org/9283022/%3Chttp://codereview.chromium.org/92...>
> >

[ygorshenin1, 2012-02-17 11:37:00]

Added protection for TCP Server Sockets. PTAL.

[Dmitry Polukhin, 2012-02-17 11:56:22]

LGTM

http://codereview.chromium.org/9283022/diff/73003/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9283022/diff/73003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.h:22: #include
"ppapi/c/private/ppb_net_address_private.h"
nit: it looks like forward declaration for PP_NetAddress_Private should be
enough

[ygorshenin1, 2012-02-17 12:08:00]

Thanks!

http://codereview.chromium.org/9283022/diff/73003/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9283022/diff/73003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.h:22: #include
"ppapi/c/private/ppb_net_address_private.h"
On 2012/02/17 11:56:22, Dmitry Polukhin wrote:
> nit: it looks like forward declaration for PP_NetAddress_Private should be
> enough

Done.

[darin (slow to review), 2012-02-17 18:54:09]

On Fri, Feb 17, 2012 at 2:15 AM, <ygorshenin@chromium.org> wrote:

> Semantic of "shutdown" system call is to close a socket with guaranteed
> delivery
> of data, that has been sent via this socket. So, "Shutdown" is more
> appropriate
> for PPB_TCPSocket_Private (instead of "Close" method), not for
> PPB_TCPServerSocket_Private.
>

OK, yes... sorry to confuse the issue, I was actually referring to that
case.  It is fair to punt that to a later discussion.


>
> So, finally, should we rename "StopListening" to "Close"?


I think StopListening is fine.  I was just curious about the thinking that
motivated the name choice.

-Darin



>
>
> On 2012/02/16 22:06:09, darin wrote:
>
>> Is there something analogous to the shutdown BSD socket call?
>>
>
>  -Darin
>>
>
>
>  On Thu, Feb 16, 2012 at 11:49 AM, <mailto:dpolukhin@chromium.org**>
>> wrote:
>>
>
>  > On 2012/02/16 19:30:04, darin wrote:
>> >
>> >> Agreed.  In the Pepper API, we usually do tie Close to Open.  I was
>> just
>> >> thinking that "close()" is the function you call when using berkeley
>> >> sockets :-)
>> >>
>> >
>> > For client TCP socket we have Connect/Disconnect and UDP sockets
>> > Bind/Close.
>> > Should we rename StopListening to Close?
>> >
>> >
>>
>
> http://codereview.chromium.****org/9283022/%3Chttp://coderevi**
> ew.chromium.org/9283022/ <http://codereview.chromium.org/9283022/>>
>
>> >
>>
>
>
>
>
>
>
http://codereview.chromium.**org/9283022/<http://codereview.chromium.org/9283...
>

[ygorshenin1, 2012-02-20 08:34:56]

+ tony

Need an OWNER LGTM for webkit/glue/webkit_glue.gypi.

[tony, 2012-02-21 19:51:07]

webkit/glue LGTM

[commit-bot: I haz the power, 2012-02-22 13:29:00]

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

[commit-bot: I haz the power, 2012-02-22 15:56:06]

Change committed as 123033