Switched proxy for TCPServerSocketPrivate.

Switched proxy for TCPServerSocketPrivate.

BUG=230784
TEST=browser_tests:*TCPServerSocketPrivate

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

[ygorshenin1, 2013-07-11 13:43:58]

+ Yuzhu

[ygorshenin1, 2013-07-15 07:49:05]

Friendly ping.

[ygorshenin1, 2013-07-17 18:12:23]

+ Justin for ppapi/proxy/ppapi_messages.h

[jschuh, 2013-07-18 01:04:11]

It doesn't look like there's any attack surface changes here from an IPC
security perspective. Is that correct?

[ygorshenin1, 2013-07-18 07:44:32]

On 2013/07/18 01:04:11, Justin Schuh wrote:
> It doesn't look like there's any attack surface changes here from an IPC
> security perspective. Is that correct?

Yes, new messages for TCPServerSockets are just simplified version of the old.

[ygorshenin1, 2013-07-19 09:24:20]

Friendly ping.

[jschuh, 2013-07-19 16:53:26]

Okay, looks like an ipc security no-op then, so lgtm.

[yzshen1, 2013-07-19 21:56:17]

On 2013/07/19 09:24:20, ygorshenin1 wrote:
> Friendly ping.

Hi, Yuri.

Sorry, I was on vacation till today. (I think I mentioned that the other day on
GTalk.)
I will review it.

[yzshen1, 2013-07-19 23:51:57]

Thanks, Yuri!

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:41: const
scoped_refptr<PepperMessageFilter>& pepper_message_filter);
Please add a TODO that we should remove this once TCP socket is converted to the
new design.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h
(right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:26:
const scoped_refptr<PepperMessageFilter>& pepper_message_filter);
Please add a TODO that we should remove this parameter once TCP socket is
converted to the new design.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:394: if
(tcp_sockets_.size() >= kMaxSocketsAllowed)
After the change, we don't limit the amount of living tcp sever sockets anymore.
(A previous change has done the same thing to UDP socket.)

We probably should do something, for example, limit the total number of living
objects of TCP server socket message filter.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_socket_utils.h (right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_socket_utils.h:28: bool
CanUseSocketAPIs(bool external_plugin,
I took a quick look at all callers, and it seems we could remove this method and
only use the one above.
(But it could be a separate CL.)

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc
(right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:102:
base::Unretained(this),
You need to use |this| instead of |base::Unretained(this)| here.

This is different from PepperTCPServerSocketMessageFilter::OnMsgAccept(), in
which we should use |base::Unretained(this)|.
I have a comment in, for example, PepperUDPSocketMessageFilter::OnMsgRecvFrom()
about why in some cases base::Unretained() is needed.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

But here we should use |this|. Otherwise, if this object dies before the task is
run, bad things will happen.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:113:
ppapi::host::ReplyMessageContext reply_context(
nit: You could move this downwards.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:120:
int result = socket_->Accept(
nit: maybe it is better to use |net_result| so that we know it is not PP_Error.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:157:
int result = socket_->Listen(net::IPEndPoint(address, port), backlog);
nit: maybe it is better to use |net_result| so that we know it is not PP_Error.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:170:
SendListenError(context, NetErrorToPepperError(result));
This changes the behavior of the API, it returns more detailed error info than
before.

When stablizing TCP/UDP socket, I did something at the plugin side to convert
result code to maintain the private API behavior.
(https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/udp_so...)

But I am not sure how this will affect the users, maybe it is fine. :)

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:193:
state_ = STATE_LISTENING;
What if we fail above? Then we will be in STATE_LISTEN_IN_PROGRESS. I think we
should set to CLOSED to match the previous behavior.

(The previous code directly destroyed the object when it failed, so it didn't
have such a problem.)

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:201:
SendAcceptError(context, PP_ERROR_FAILED);
we should also reset state_ in failure cases.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h
(right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:67:
int result);
This is net::Error right?
Maybe it is better to use "int net_result". Because we also have "int32_t
result" which is PP_Error. This way, it is less likely that we pass "net_result"
mistakenly as a PP_Error.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:70:
int result);
ditto

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:91:
scoped_refptr<PepperMessageFilter> pepper_message_filter_;
nit: it is better to reorder the members according to whether they are used on
the IO or UI thread, and add comments

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(left):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:63: 
It seems irrelevant to change this file.
(And it seems the empty line makes sense, which separates the overrides with the
other  private methods.)

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/ppb_tcp_server...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (left):

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/ppb_tcp_server...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:86:
API_ID_PPB_TCPSERVERSOCKET_PRIVATE,
The definition of this ID in api_id.h is not needed anymore.
Besides, you could also change ppapi_thread.cc to remove the corresponding
AddRoute().

(We should do the same thing for UDPSocket as well, now that we have converted
it to the new design.)

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
File ppapi/proxy/tcp_server_socket_private_resource.cc (right):

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:85: if (state_ !=
STATE_LISTENING)
The previous behavior is that if there is a pending accept, it should still
succeed.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:95: Post(BROWSER,
PpapiHostMsg_TCPServerSocket_StopListening());
It is nice to abort listen/accept callbacks here, if any.
That matches the previous behavior, as well as behavior of TCP/UDP socket.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:108: state_ = STATE_LISTENING;
We should properly set |state_| on failure.

The previous code stays in the before_listening state, but now that you have
added listen_in_progress, you need to make change accordingly.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:123: return;
Please also reset |state_| here.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
File ppapi/proxy/tcp_server_socket_private_resource.h (right):

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.h:18: 
I noticed that you didn't delete ppb_tcp_server_socket_shared.

https://codereview.chromium.org/19005006/diff/5001/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (left):

https://codereview.chromium.org/19005006/diff/5001/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:580: // For PPB_TCPServerSocket_Private.
I think you should also change the mock plugin delegate.

[ygorshenin1, 2013-07-29 14:03:56]

PTAL

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:41: const
scoped_refptr<PepperMessageFilter>& pepper_message_filter);
On 2013/07/19 23:51:58, yzshen1 wrote:
> Please add a TODO that we should remove this once TCP socket is converted to
the
> new design.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h
(right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:26:
const scoped_refptr<PepperMessageFilter>& pepper_message_filter);
On 2013/07/19 23:51:58, yzshen1 wrote:
> Please add a TODO that we should remove this parameter once TCP socket is
> converted to the new design.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:394: if
(tcp_sockets_.size() >= kMaxSocketsAllowed)
On 2013/07/19 23:51:58, yzshen1 wrote:
> After the change, we don't limit the amount of living tcp sever sockets
anymore.
> (A previous change has done the same thing to UDP socket.)
> 
> We probably should do something, for example, limit the total number of living
> objects of TCP server socket message filter.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_socket_utils.h (right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_socket_utils.h:28: bool
CanUseSocketAPIs(bool external_plugin,
I've added a TODO comment.

On 2013/07/19 23:51:58, yzshen1 wrote:
> I took a quick look at all callers, and it seems we could remove this method
and
> only use the one above.
> (But it could be a separate CL.)

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc
(right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:102:
base::Unretained(this),
On 2013/07/19 23:51:58, yzshen1 wrote:
> You need to use |this| instead of |base::Unretained(this)| here.
> 
> This is different from PepperTCPServerSocketMessageFilter::OnMsgAccept(), in
> which we should use |base::Unretained(this)|.
> I have a comment in, for example,
PepperUDPSocketMessageFilter::OnMsgRecvFrom()
> about why in some cases base::Unretained() is needed.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> But here we should use |this|. Otherwise, if this object dies before the task
is
> run, bad things will happen.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:113:
ppapi::host::ReplyMessageContext reply_context(
On 2013/07/19 23:51:58, yzshen1 wrote:
> nit: You could move this downwards.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:120:
int result = socket_->Accept(
On 2013/07/19 23:51:58, yzshen1 wrote:
> nit: maybe it is better to use |net_result| so that we know it is not
PP_Error.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:157:
int result = socket_->Listen(net::IPEndPoint(address, port), backlog);
On 2013/07/19 23:51:58, yzshen1 wrote:
> nit: maybe it is better to use |net_result| so that we know it is not
PP_Error.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:170:
SendListenError(context, NetErrorToPepperError(result));
Hope, that it's fine as tcp server sockets are still private and there're not so
many apps which use this API.

On 2013/07/19 23:51:58, yzshen1 wrote:
> This changes the behavior of the API, it returns more detailed error info than
> before.
> 
> When stablizing TCP/UDP socket, I did something at the plugin side to convert
> result code to maintain the private API behavior.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/udp_so...)
> 
> But I am not sure how this will affect the users, maybe it is fine. :)

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:193:
state_ = STATE_LISTENING;
On 2013/07/19 23:51:58, yzshen1 wrote:
> What if we fail above? Then we will be in STATE_LISTEN_IN_PROGRESS. I think we
> should set to CLOSED to match the previous behavior.
> 
> (The previous code directly destroyed the object when it failed, so it didn't
> have such a problem.)

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:201:
SendAcceptError(context, PP_ERROR_FAILED);
On 2013/07/19 23:51:58, yzshen1 wrote:
> we should also reset state_ in failure cases.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h
(right):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:67:
int result);
On 2013/07/19 23:51:58, yzshen1 wrote:
> This is net::Error right?
> Maybe it is better to use "int net_result". Because we also have "int32_t
> result" which is PP_Error. This way, it is less likely that we pass
"net_result"
> mistakenly as a PP_Error.

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:70:
int result);
On 2013/07/19 23:51:58, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:91:
scoped_refptr<PepperMessageFilter> pepper_message_filter_;
On 2013/07/19 23:51:58, yzshen1 wrote:
> nit: it is better to reorder the members according to whether they are used on
> the IO or UI thread, and add comments

Done.

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(left):

https://codereview.chromium.org/19005006/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:63: 
On 2013/07/19 23:51:58, yzshen1 wrote:
> It seems irrelevant to change this file.
> (And it seems the empty line makes sense, which separates the overrides with
the
> other  private methods.)

Done.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/ppb_tcp_server...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (left):

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/ppb_tcp_server...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:86:
API_ID_PPB_TCPSERVERSOCKET_PRIVATE,
On 2013/07/19 23:51:58, yzshen1 wrote:
> The definition of this ID in api_id.h is not needed anymore.
> Besides, you could also change ppapi_thread.cc to remove the corresponding
> AddRoute().
> 
> (We should do the same thing for UDPSocket as well, now that we have converted
> it to the new design.)

Done.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
File ppapi/proxy/tcp_server_socket_private_resource.cc (right):

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:85: if (state_ !=
STATE_LISTENING)
On 2013/07/19 23:51:58, yzshen1 wrote:
> The previous behavior is that if there is a pending accept, it should still
> succeed.

Done.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:95: Post(BROWSER,
PpapiHostMsg_TCPServerSocket_StopListening());
On 2013/07/19 23:51:58, yzshen1 wrote:
> It is nice to abort listen/accept callbacks here, if any.
> That matches the previous behavior, as well as behavior of TCP/UDP socket.

Done.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:108: state_ = STATE_LISTENING;
On 2013/07/19 23:51:58, yzshen1 wrote:
> We should properly set |state_| on failure.
> 
> The previous code stays in the before_listening state, but now that you have
> added listen_in_progress, you need to make change accordingly.

Done.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:123: return;
On 2013/07/19 23:51:58, yzshen1 wrote:
> Please also reset |state_| here.

Done.

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
File ppapi/proxy/tcp_server_socket_private_resource.h (right):

https://codereview.chromium.org/19005006/diff/5001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.h:18: 
On 2013/07/19 23:51:58, yzshen1 wrote:
> I noticed that you didn't delete ppb_tcp_server_socket_shared.

Done.

https://codereview.chromium.org/19005006/diff/5001/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (left):

https://codereview.chromium.org/19005006/diff/5001/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:580: // For PPB_TCPServerSocket_Private.
On 2013/07/19 23:51:58, yzshen1 wrote:
> I think you should also change the mock plugin delegate.

Done.

[yzshen1, 2013-07-29 18:18:10]

Thanks! Only a few nits left.

https://codereview.chromium.org/19005006/diff/35001/ppapi/proxy/tcp_server_so...
File ppapi/proxy/tcp_server_socket_private_resource.cc (right):

https://codereview.chromium.org/19005006/diff/35001/ppapi/proxy/tcp_server_so...
ppapi/proxy/tcp_server_socket_private_resource.cc:104: if
(!TrackedCallback::IsPending(listen_callback_)) {
There is a race. Please consider the following sequence:
- a listen reply arrives at the message queue, but hasn't run.
- StopListening() is called, an abort task is posted for listen_callback_;
- the listen reply is processed. Because at this moment listen_callback_ is
still pending, this method will set state_ back to STATE_LISTENING.

Therefore, we should test whether state_ = STATE_CLOSED and return directly if
it is.

https://codereview.chromium.org/19005006/diff/35001/ppapi/proxy/tcp_server_so...
ppapi/proxy/tcp_server_socket_private_resource.cc:124: if
(!TrackedCallback::IsPending(accept_callback_)) {
ditto, please see my previous comment.

[ygorshenin1, 2013-07-30 11:03:18]

PTAL

https://codereview.chromium.org/19005006/diff/35001/ppapi/proxy/tcp_server_so...
File ppapi/proxy/tcp_server_socket_private_resource.cc (right):

https://codereview.chromium.org/19005006/diff/35001/ppapi/proxy/tcp_server_so...
ppapi/proxy/tcp_server_socket_private_resource.cc:104: if
(!TrackedCallback::IsPending(listen_callback_)) {
On 2013/07/29 18:18:10, yzshen1 wrote:
> There is a race. Please consider the following sequence:
> - a listen reply arrives at the message queue, but hasn't run.
> - StopListening() is called, an abort task is posted for listen_callback_;
> - the listen reply is processed. Because at this moment listen_callback_ is
> still pending, this method will set state_ back to STATE_LISTENING.
> 
> Therefore, we should test whether state_ = STATE_CLOSED and return directly if
> it is.

Done.

https://codereview.chromium.org/19005006/diff/35001/ppapi/proxy/tcp_server_so...
ppapi/proxy/tcp_server_socket_private_resource.cc:124: if
(!TrackedCallback::IsPending(accept_callback_)) {
On 2013/07/29 18:18:10, yzshen1 wrote:
> ditto, please see my previous comment.

Done.

[yzshen1, 2013-07-30 17:56:49]

LGTM

Thanks!

https://codereview.chromium.org/19005006/diff/90001/ppapi/proxy/tcp_server_so...
File ppapi/proxy/tcp_server_socket_private_resource.cc (right):

https://codereview.chromium.org/19005006/diff/90001/ppapi/proxy/tcp_server_so...
ppapi/proxy/tcp_server_socket_private_resource.cc:104: if (state_ ==
STATE_CLOSED)
Nit: it seems to following is more robust:
if (state != STATE_BEFORE_LISTENING || !TrackedCallback::IsPending(...)) {
  return;
}

And you don't need the else at line 111 - 113.

https://codereview.chromium.org/19005006/diff/90001/ppapi/proxy/tcp_server_so...
ppapi/proxy/tcp_server_socket_private_resource.cc:124: if (state_ ==
STATE_CLOSED)
Nit: it seems to following is more robust:
if (state != STATE_LISTENING || !TrackedCallback::IsPending(...)) {
  return;
}

[ygorshenin1, 2013-07-31 11:09:30]

Many thanks!

Brett, I need your OWNER's lgtm for content/*.

https://codereview.chromium.org/19005006/diff/90001/ppapi/proxy/tcp_server_so...
File ppapi/proxy/tcp_server_socket_private_resource.cc (right):

https://codereview.chromium.org/19005006/diff/90001/ppapi/proxy/tcp_server_so...
ppapi/proxy/tcp_server_socket_private_resource.cc:104: if (state_ ==
STATE_CLOSED)
On 2013/07/30 17:56:49, yzshen1 wrote:
> Nit: it seems to following is more robust:
> if (state != STATE_BEFORE_LISTENING || !TrackedCallback::IsPending(...)) {
>   return;
> }
> 
> And you don't need the else at line 111 - 113.

Done.

https://codereview.chromium.org/19005006/diff/90001/ppapi/proxy/tcp_server_so...
ppapi/proxy/tcp_server_socket_private_resource.cc:124: if (state_ ==
STATE_CLOSED)
On 2013/07/30 17:56:49, yzshen1 wrote:
> Nit: it seems to following is more robust:
> if (state != STATE_LISTENING || !TrackedCallback::IsPending(...)) {
>   return;
> }

Done.

[jam, 2013-08-02 15:16:53]

just saw this while looking to see if anyone else had outstanding changes to
these files as I'm changing them in https://codereview.chromium.org/20777009/.
lgtm on behalf of brett for content. I'll wait for you to land and then merge
afterwards.

https://codereview.chromium.org/19005006/diff/97001/content/browser/renderer_...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/19005006/diff/97001/content/browser/renderer_...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:31: // The
plugin name and path shouldn't be needed for NaCl apps.
nit: don't mention nacl in content

[commit-bot: I haz the power, 2013-08-05 08:29:35]

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

[commit-bot: I haz the power, 2013-08-05 11:59:47]

Change committed as 215597

[ygorshenin1, 2013-08-05 12:00:16]

Many thanks!

https://chromiumcodereview.appspot.com/19005006/diff/97001/content/browser/re...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://chromiumcodereview.appspot.com/19005006/diff/97001/content/browser/re...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:31: // The
plugin name and path shouldn't be needed for NaCl apps.
On 2013/08/02 15:16:53, jam wrote:
> nit: don't mention nacl in content

Done.

[jam, 2013-08-05 15:05:26]

btw did you mean to delete ppb_tcp_server_socket_private_impl.*?