TCPSockets are switched to the new Pepper proxy.

TCPSockets are switched to the new Pepper proxy.

BUG=230784
TEST=browser_tests:*TCPSocket*, *TCPServerSocket*

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

[yzshen1, 2013-08-16 20:40:42]

Thanks Yuri! It is great to have this cleaned up. :)

A few more things to clean up:
- api_id.h
- ppapi_thread.cc

https://codereview.chromium.org/22923014/diff/2001/content/browser/plugin_ser...
File content/browser/plugin_service_impl.cc (right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/plugin_ser...
content/browser/plugin_service_impl.cc:332:
PpapiPluginProcessHost::PluginClient* client) {
This parameter is not needed any more.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:31:
channel->AddFilter(PepperMessageFilter::CreateExternal(permissions));
You should still put it in a scoped_refptr<> before passing it into AddFilter().
Without doing that, if AddFilter() doesn't take one ref (and then release it),
the object is leaked.

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

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:28:
ContentBrowserPepperHostFactory(BrowserPpapiHostImpl* host);
explicit?

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:41:
// pending resource host, zero otherwise.
- Please take about the ownership of |socket|.
- It is better to let this factory return scoped_ptr<ppapi::host::ResourceHost>,
the work of adding to pending resource host list should be handled by
TCPServerSocket itself.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:52:
scoped_refptr<SSLContextHelper> ssl_context_helper_;
Maybe we could make it part of BrowserPpapiHostImpl?
And then you don't need to pass it into TCPSocket, because it has access to
BrowserPpapiHostImpl.

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

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:78: browser_path_
= browser_context->GetPath();
It seems this is not used? Maybe we could remove BrowserContext parameter
entirely?

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.h (right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.h:37: static
PepperMessageFilter* CreateInProcess(BrowserContext* browser_context);
How about the following names?
CreateForRendererProcess
CreateForPpapiPluginProcess
CreateForExternalPluginProcess

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

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_socket_utils.h:14: class 
X509Certificate;
extra space.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_socket_utils.h:44: //
|PPB_X509Certificate_Fields|.
nit: we usually don't use | around type names.

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

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:87:
ContentBrowserPepperHostFactory* factory_;
Please mentioned that it is not owned by this class.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_tcp_socket.h (left):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_PEPPER_PEPPER_TCP_SOCKET_H_
I didn't see you delete pepper_tcp_socket.cc?

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:147:
return PP_ERROR_FAILED;
PP_ERROR_NOACCESS.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:202: if
(connection_state_ != CONNECTED || read_buffer_.get() ||
Please keep the comments from PepperTCPSocket.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:207:
const ppapi::host::ReplyMessageContext reply_context(
Please move this to be above line 230.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:289: //
Make sure no further callbacks from socket_.
In the previous code, when we get a disconnect request, we destroy the socket
entirely, therefore, we don't have to have a CLOSED state.
Now we have to, and we should make sure once the socket is in CLOSED state, any
call will fail.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
(right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:53: //
Used for creation already connected sockets.  Takes ownership of
nit: creation -> creating

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:170: //
rather have our |Write()| do so whenever possible. To do this, we may have
Please update the comment, we don't have a method called "Write" anymore.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/ssl_context_helper.h (right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/ssl_context_helper.h:23: virtual
~SSLContextHelper();
//    private:
//     friend class base::RefCounted<MyFoo>;
//     ~MyFoo();

And please also note that the destructor not need to be virtual.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/pp_utils.h
File ppapi/proxy/pp_utils.h (right):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/pp_utils.h#new...
ppapi/proxy/pp_utils.h:12: namespace pp_utils {
No need to add another layer of namespace.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/pp_utils.h#new...
ppapi/proxy/pp_utils.h:14: int32_t ConvertPPError(int32_t pp_error, bool
private_api);
Both the file name and the method name are too broad.

And please add comments about what exactly this method does.

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

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:121: Connection
connection(PluginGlobals::Get()->GetBrowserSender(),
TCPServerSocketPrivateResource itself has a connection() accessor, please use it
directly.
(and then you don't need to add #include, either.)

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_pri...
File ppapi/proxy/tcp_socket_private_resource.h (right):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_pri...
ppapi/proxy/tcp_socket_private_resource.h:4: 
Please add #ifndef / #define

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
File ppapi/proxy/tcp_socket_resource.cc (right):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource.cc:24: SendCreate(BROWSER,
PpapiHostMsg_TCPSocket_CreatePrivate());
wrong message!

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
File ppapi/proxy/tcp_socket_resource_base.cc (left):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:12: #include "base/bind.h"
Things like bind, logging are still needed, right? Why removing the includes?

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
File ppapi/proxy/tcp_socket_resource_base.cc (right):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:55: local_addr_ = local_addr_;
You used the wrong variable name. Besides, you could put them in the
initialization list as well.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:56: remote_addr_ = remote_addr_;
You used the wrong variable name.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:202:
PpapiHostMsg_TCPSocket_Read(bytes_to_read),
you used the wrong variable name.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:324: // It is possible that
|ssl_handshake_callback_| is pending while
wrong indent.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:372: static_cast<int32_t>(data.size()) :
four more spaces of indent, please.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
File ppapi/proxy/tcp_socket_resource_base.h (left):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.h:8: #include <queue>
it is still needed, right?

[ygorshenin1, 2013-08-19 14:33:35]

PTAL

https://codereview.chromium.org/22923014/diff/2001/content/browser/plugin_ser...
File content/browser/plugin_service_impl.cc (right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/plugin_ser...
content/browser/plugin_service_impl.cc:332:
PpapiPluginProcessHost::PluginClient* client) {
On 2013/08/16 20:40:42, yzshen1 wrote:
> This parameter is not needed any more.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:31:
channel->AddFilter(PepperMessageFilter::CreateExternal(permissions));
On 2013/08/16 20:40:42, yzshen1 wrote:
> You should still put it in a scoped_refptr<> before passing it into
AddFilter().
> Without doing that, if AddFilter() doesn't take one ref (and then release it),
> the object is leaked.

Done.

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

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:28:
ContentBrowserPepperHostFactory(BrowserPpapiHostImpl* host);
On 2013/08/16 20:40:42, yzshen1 wrote:
> explicit?

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:41:
// pending resource host, zero otherwise.
On 2013/08/16 20:40:42, yzshen1 wrote:
> - Please take about the ownership of |socket|.
> - It is better to let this factory return
scoped_ptr<ppapi::host::ResourceHost>,
> the work of adding to pending resource host list should be handled by
> TCPServerSocket itself.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:52:
scoped_refptr<SSLContextHelper> ssl_context_helper_;
On 2013/08/16 20:40:42, yzshen1 wrote:
> Maybe we could make it part of BrowserPpapiHostImpl?
> And then you don't need to pass it into TCPSocket, because it has access to
> BrowserPpapiHostImpl.

Done.

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

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:78: browser_path_
= browser_context->GetPath();
On 2013/08/16 20:40:42, yzshen1 wrote:
> It seems this is not used? Maybe we could remove BrowserContext parameter
> entirely?

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.h (right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.h:37: static
PepperMessageFilter* CreateInProcess(BrowserContext* browser_context);
On 2013/08/16 20:40:42, yzshen1 wrote:
> How about the following names?
> CreateForRendererProcess
> CreateForPpapiPluginProcess
> CreateForExternalPluginProcess

Done.

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

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_socket_utils.h:14: class 
X509Certificate;
On 2013/08/16 20:40:42, yzshen1 wrote:
> extra space.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_socket_utils.h:44: //
|PPB_X509Certificate_Fields|.
On 2013/08/16 20:40:42, yzshen1 wrote:
> nit: we usually don't use | around type names.

Done.

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

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:87:
ContentBrowserPepperHostFactory* factory_;
On 2013/08/16 20:40:42, yzshen1 wrote:
> Please mentioned that it is not owned by this class.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_tcp_socket.h (left):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_PEPPER_PEPPER_TCP_SOCKET_H_
On 2013/08/16 20:40:42, yzshen1 wrote:
> I didn't see you delete pepper_tcp_socket.cc?

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:147:
return PP_ERROR_FAILED;
On 2013/08/16 20:40:42, yzshen1 wrote:
> PP_ERROR_NOACCESS.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:202: if
(connection_state_ != CONNECTED || read_buffer_.get() ||
On 2013/08/16 20:40:42, yzshen1 wrote:
> Please keep the comments from PepperTCPSocket.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:207:
const ppapi::host::ReplyMessageContext reply_context(
On 2013/08/16 20:40:42, yzshen1 wrote:
> Please move this to be above line 230.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:289: //
Make sure no further callbacks from socket_.
On 2013/08/16 20:40:42, yzshen1 wrote:
> In the previous code, when we get a disconnect request, we destroy the socket
> entirely, therefore, we don't have to have a CLOSED state.
> Now we have to, and we should make sure once the socket is in CLOSED state,
any
> call will fail.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
(right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:53: //
Used for creation already connected sockets.  Takes ownership of
On 2013/08/16 20:40:42, yzshen1 wrote:
> nit: creation -> creating

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:170: //
rather have our |Write()| do so whenever possible. To do this, we may have
On 2013/08/16 20:40:42, yzshen1 wrote:
> Please update the comment, we don't have a method called "Write" anymore.

Done.

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/ssl_context_helper.h (right):

https://codereview.chromium.org/22923014/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/pepper/ssl_context_helper.h:23: virtual
~SSLContextHelper();
On 2013/08/16 20:40:42, yzshen1 wrote:
> //    private:
> //     friend class base::RefCounted<MyFoo>;
> //     ~MyFoo();
> 
> And please also note that the destructor not need to be virtual.

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/pp_utils.h
File ppapi/proxy/pp_utils.h (right):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/pp_utils.h#new...
ppapi/proxy/pp_utils.h:12: namespace pp_utils {
On 2013/08/16 20:40:42, yzshen1 wrote:
> No need to add another layer of namespace. 

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/pp_utils.h#new...
ppapi/proxy/pp_utils.h:14: int32_t ConvertPPError(int32_t pp_error, bool
private_api);
On 2013/08/16 20:40:42, yzshen1 wrote:
> Both the file name and the method name are too broad.
> 
> And please add comments about what exactly this method does.

Done.

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

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_server_soc...
ppapi/proxy/tcp_server_socket_private_resource.cc:121: Connection
connection(PluginGlobals::Get()->GetBrowserSender(),
On 2013/08/16 20:40:42, yzshen1 wrote:
> TCPServerSocketPrivateResource itself has a connection() accessor, please use
it
> directly.
> (and then you don't need to add #include, either.)

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_pri...
File ppapi/proxy/tcp_socket_private_resource.h (right):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_pri...
ppapi/proxy/tcp_socket_private_resource.h:4: 
On 2013/08/16 20:40:42, yzshen1 wrote:
> Please add #ifndef / #define

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
File ppapi/proxy/tcp_socket_resource.cc (right):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource.cc:24: SendCreate(BROWSER,
PpapiHostMsg_TCPSocket_CreatePrivate());
On 2013/08/16 20:40:42, yzshen1 wrote:
> wrong message!

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
File ppapi/proxy/tcp_socket_resource_base.cc (left):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:12: #include "base/bind.h"
On 2013/08/16 20:40:42, yzshen1 wrote:
> Things like bind, logging are still needed, right? Why removing the includes?

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
File ppapi/proxy/tcp_socket_resource_base.cc (right):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:55: local_addr_ = local_addr_;
On 2013/08/16 20:40:42, yzshen1 wrote:
> You used the wrong variable name. Besides, you could put them in the
> initialization list as well.

Done. Tests for TCPServerSocket handle this now.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:56: remote_addr_ = remote_addr_;
On 2013/08/16 20:40:42, yzshen1 wrote:
> You used the wrong variable name.

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:202:
PpapiHostMsg_TCPSocket_Read(bytes_to_read),
On 2013/08/16 20:40:42, yzshen1 wrote:
> you used the wrong variable name.

Done, now test for TCPSocketPrivate handles this.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:324: // It is possible that
|ssl_handshake_callback_| is pending while
On 2013/08/16 20:40:42, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.cc:372: static_cast<int32_t>(data.size()) :
On 2013/08/16 20:40:42, yzshen1 wrote:
> four more spaces of indent, please.

Done.

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
File ppapi/proxy/tcp_socket_resource_base.h (left):

https://codereview.chromium.org/22923014/diff/2001/ppapi/proxy/tcp_socket_res...
ppapi/proxy/tcp_socket_resource_base.h:8: #include <queue>
On 2013/08/16 20:40:42, yzshen1 wrote:
> it is still needed, right?

Done.

[yzshen1, 2013-08-19 19:07:07]

Only a few nits left.

Thanks! :)

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h
(right):

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:94:
// Non-owning ptr.
nit: also comment for line 93.

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:353:
SetState(STATE_CLOSED);
I think we shouldn't close in this case:

- the plugin requests to connect.
- connect succeeds.
- the plugin requests to connect again.
- we shouldn't close the socket.

I think the previous behavior makes sense.

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:378:
SetState(STATE_CLOSED);
ditto.

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/pp_utils.h
File ppapi/proxy/pp_utils.h (right):

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/pp_utils.h#ne...
ppapi/proxy/pp_utils.h:16: int32_t ConvertPPError(int32_t pp_error, bool
private_api);
Maybe we should merge this into ppapi/host/error_conversion.h?

And we could call it ConvertNetworkAPIErrorForCompatibility() or something like
that.

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/tcp_socket_re...
File ppapi/proxy/tcp_socket_resource_base.cc (right):

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/tcp_socket_re...
ppapi/proxy/tcp_socket_resource_base.cc:376: ? static_cast<int32_t>(data.size())
? and : should be on the previous line.

[ygorshenin1, 2013-08-20 13:00:49]

PTAL

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h
(right):

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:94:
// Non-owning ptr.
On 2013/08/19 19:07:07, yzshen1 wrote:
> nit: also comment for line 93.

Done.

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:353:
SetState(STATE_CLOSED);
On 2013/08/19 19:07:07, yzshen1 wrote:
> I think we shouldn't close in this case:
> 
> - the plugin requests to connect.
> - connect succeeds.
> - the plugin requests to connect again.
> - we shouldn't close the socket.
> 
> I think the previous behavior makes sense.

Done.

https://codereview.chromium.org/22923014/diff/64001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:378:
SetState(STATE_CLOSED);
On 2013/08/19 19:07:07, yzshen1 wrote:
> ditto.

Done.

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/pp_utils.h
File ppapi/proxy/pp_utils.h (right):

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/pp_utils.h#ne...
ppapi/proxy/pp_utils.h:16: int32_t ConvertPPError(int32_t pp_error, bool
private_api);
On 2013/08/19 19:07:07, yzshen1 wrote:
> Maybe we should merge this into ppapi/host/error_conversion.h?
> 
> And we could call it ConvertNetworkAPIErrorForCompatibility() or something
like
> that.

I've renamed this method, but not merged into ppapi/host/error_conversion.h,
because
it requires in addition to ppapi_host an introduction of ppapi_host_untrusted
target, as ConvertNetworkAPIErrorForCompatibility() is compiled for nacl. Yuzhu,
what do you think?

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/tcp_socket_re...
File ppapi/proxy/tcp_socket_resource_base.cc (right):

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/tcp_socket_re...
ppapi/proxy/tcp_socket_resource_base.cc:376: ? static_cast<int32_t>(data.size())
I've used
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/ast.cc&q=fi...
as an example.
Done.

On 2013/08/19 19:07:07, yzshen1 wrote:
> ? and : should be on the previous line.

[ygorshenin1, 2013-08-20 13:02:48]

+ Justin for ppapi/proxy/ppapi_messages.h

[yzshen1, 2013-08-20 22:39:58]

LGTM

Thanks!

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/pp_utils.h
File ppapi/proxy/pp_utils.h (right):

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/pp_utils.h#ne...
ppapi/proxy/pp_utils.h:16: int32_t ConvertPPError(int32_t pp_error, bool
private_api);
On 2013/08/20 13:00:49, ygorshenin1 wrote:
> On 2013/08/19 19:07:07, yzshen1 wrote:
> > Maybe we should merge this into ppapi/host/error_conversion.h?
> > 
> > And we could call it ConvertNetworkAPIErrorForCompatibility() or something
> like
> > that.
> 
> I've renamed this method, but not merged into ppapi/host/error_conversion.h,
> because
> it requires in addition to ppapi_host an introduction of ppapi_host_untrusted
> target, as ConvertNetworkAPIErrorForCompatibility() is compiled for nacl.
Yuzhu,
> what do you think?

I think you could move error_conversion.h/cc into ppapi/proxy and then move this
function into those files.

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/tcp_socket_re...
File ppapi/proxy/tcp_socket_resource_base.cc (right):

https://codereview.chromium.org/22923014/diff/64001/ppapi/proxy/tcp_socket_re...
ppapi/proxy/tcp_socket_resource_base.cc:376: ? static_cast<int32_t>(data.size())
Okay.

I think that is not mandatory. :)
On 2013/08/20 13:00:49, ygorshenin1 wrote:
> I've used
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/ast.cc&q=fi...
> as an example.
> Done.
> 
> On 2013/08/19 19:07:07, yzshen1 wrote:
> > ? and : should be on the previous line.
>

[ygorshenin1, 2013-08-21 12:00:40]

Many thanks, Yuzhu!

+jam for content/*
+mmenke for ppapi/proxy/DEPS

https://chromiumcodereview.appspot.com/22923014/diff/64001/ppapi/proxy/pp_uti...
File ppapi/proxy/pp_utils.h (right):

https://chromiumcodereview.appspot.com/22923014/diff/64001/ppapi/proxy/pp_uti...
ppapi/proxy/pp_utils.h:16: int32_t ConvertPPError(int32_t pp_error, bool
private_api);
On 2013/08/20 22:39:58, yzshen1 wrote:
> On 2013/08/20 13:00:49, ygorshenin1 wrote:
> > On 2013/08/19 19:07:07, yzshen1 wrote:
> > > Maybe we should merge this into ppapi/host/error_conversion.h?
> > > 
> > > And we could call it ConvertNetworkAPIErrorForCompatibility() or something
> > like
> > > that.
> > 
> > I've renamed this method, but not merged into ppapi/host/error_conversion.h,
> > because
> > it requires in addition to ppapi_host an introduction of
ppapi_host_untrusted
> > target, as ConvertNetworkAPIErrorForCompatibility() is compiled for nacl.
> Yuzhu,
> > what do you think?
> 
> I think you could move error_conversion.h/cc into ppapi/proxy and then move
this
> function into those files.
> 

Done.

[mmenke1, 2013-08-21 14:32:07]

I think it's fine to have ppapi/proxy depend on net, though I can't see why it's
needed in this CL.

On 2013/08/21 12:00:40, ygorshenin1 wrote:
> Many thanks, Yuzhu!
> 
> +jam for content/*
> +mmenke for ppapi/proxy/DEPS
> 
>
https://chromiumcodereview.appspot.com/22923014/diff/64001/ppapi/proxy/pp_uti...
> File ppapi/proxy/pp_utils.h (right):
> 
>
https://chromiumcodereview.appspot.com/22923014/diff/64001/ppapi/proxy/pp_uti...
> ppapi/proxy/pp_utils.h:16: int32_t ConvertPPError(int32_t pp_error, bool
> private_api);
> On 2013/08/20 22:39:58, yzshen1 wrote:
> > On 2013/08/20 13:00:49, ygorshenin1 wrote:
> > > On 2013/08/19 19:07:07, yzshen1 wrote:
> > > > Maybe we should merge this into ppapi/host/error_conversion.h?
> > > > 
> > > > And we could call it ConvertNetworkAPIErrorForCompatibility() or
something
> > > like
> > > > that.
> > > 
> > > I've renamed this method, but not merged into
ppapi/host/error_conversion.h,
> > > because
> > > it requires in addition to ppapi_host an introduction of
> ppapi_host_untrusted
> > > target, as ConvertNetworkAPIErrorForCompatibility() is compiled for nacl.
> > Yuzhu,
> > > what do you think?
> > 
> > I think you could move error_conversion.h/cc into ppapi/proxy and then move
> this
> > function into those files.
> > 
> 
> Done.

[ygorshenin1, 2013-08-21 14:34:53]

On 2013/08/21 14:32:07, mmenke (incorrect) wrote:
> I think it's fine to have ppapi/proxy depend on net, though I can't see why
it's
> needed in this CL.

Because ppapi/proxy/error_conversion.cc includes net/base/net_errors.h.

> 
> On 2013/08/21 12:00:40, ygorshenin1 wrote:
> > Many thanks, Yuzhu!
> > 
> > +jam for content/*
> > +mmenke for ppapi/proxy/DEPS
> > 
> >
>
https://chromiumcodereview.appspot.com/22923014/diff/64001/ppapi/proxy/pp_uti...
> > File ppapi/proxy/pp_utils.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/22923014/diff/64001/ppapi/proxy/pp_uti...
> > ppapi/proxy/pp_utils.h:16: int32_t ConvertPPError(int32_t pp_error, bool
> > private_api);
> > On 2013/08/20 22:39:58, yzshen1 wrote:
> > > On 2013/08/20 13:00:49, ygorshenin1 wrote:
> > > > On 2013/08/19 19:07:07, yzshen1 wrote:
> > > > > Maybe we should merge this into ppapi/host/error_conversion.h?
> > > > > 
> > > > > And we could call it ConvertNetworkAPIErrorForCompatibility() or
> something
> > > > like
> > > > > that.
> > > > 
> > > > I've renamed this method, but not merged into
> ppapi/host/error_conversion.h,
> > > > because
> > > > it requires in addition to ppapi_host an introduction of
> > ppapi_host_untrusted
> > > > target, as ConvertNetworkAPIErrorForCompatibility() is compiled for
nacl.
> > > Yuzhu,
> > > > what do you think?
> > > 
> > > I think you could move error_conversion.h/cc into ppapi/proxy and then
move
> > this
> > > function into those files.
> > > 
> > 
> > Done.

[mmenke1, 2013-08-21 14:36:35]

Ah, right, missed that the file was moved.  LGTM.

On 2013/08/21 14:34:53, ygorshenin1 wrote:
> On 2013/08/21 14:32:07, mmenke (incorrect) wrote:
> > I think it's fine to have ppapi/proxy depend on net, though I can't see why
> it's
> > needed in this CL.
> 
> Because ppapi/proxy/error_conversion.cc includes net/base/net_errors.h.
> 
> > 
> > On 2013/08/21 12:00:40, ygorshenin1 wrote:
> > > Many thanks, Yuzhu!
> > > 
> > > +jam for content/*
> > > +mmenke for ppapi/proxy/DEPS
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/22923014/diff/64001/ppapi/proxy/pp_uti...
> > > File ppapi/proxy/pp_utils.h (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/22923014/diff/64001/ppapi/proxy/pp_uti...
> > > ppapi/proxy/pp_utils.h:16: int32_t ConvertPPError(int32_t pp_error, bool
> > > private_api);
> > > On 2013/08/20 22:39:58, yzshen1 wrote:
> > > > On 2013/08/20 13:00:49, ygorshenin1 wrote:
> > > > > On 2013/08/19 19:07:07, yzshen1 wrote:
> > > > > > Maybe we should merge this into ppapi/host/error_conversion.h?
> > > > > > 
> > > > > > And we could call it ConvertNetworkAPIErrorForCompatibility() or
> > something
> > > > > like
> > > > > > that.
> > > > > 
> > > > > I've renamed this method, but not merged into
> > ppapi/host/error_conversion.h,
> > > > > because
> > > > > it requires in addition to ppapi_host an introduction of
> > > ppapi_host_untrusted
> > > > > target, as ConvertNetworkAPIErrorForCompatibility() is compiled for
> nacl.
> > > > Yuzhu,
> > > > > what do you think?
> > > > 
> > > > I think you could move error_conversion.h/cc into ppapi/proxy and then
> move
> > > this
> > > > function into those files.
> > > > 
> > > 
> > > Done.

[yzshen1, 2013-08-21 17:22:58]

https://chromiumcodereview.appspot.com/22923014/diff/78002/ppapi/proxy/DEPS
File ppapi/proxy/DEPS (right):

https://chromiumcodereview.appspot.com/22923014/diff/78002/ppapi/proxy/DEPS#n...
ppapi/proxy/DEPS:7: "+net",
Hmm... I am sorry I didn't realize this.
We probably shouldn't let proxy depends on net, because proxy is also compiled
at the untrusted side of NaCl.
So maybe the original file layout is the right solution (having
ppapi/host/error_conversion and ppapi/proxy/pp_utils).

[ygorshenin1, 2013-08-22 07:49:46]

On 2013/08/21 17:22:58, yzshen1 wrote:
> https://chromiumcodereview.appspot.com/22923014/diff/78002/ppapi/proxy/DEPS
> File ppapi/proxy/DEPS (right):
> 
>
https://chromiumcodereview.appspot.com/22923014/diff/78002/ppapi/proxy/DEPS#n...
> ppapi/proxy/DEPS:7: "+net",
> Hmm... I am sorry I didn't realize this.
> We probably shouldn't let proxy depends on net, because proxy is also compiled
> at the untrusted side of NaCl.
> So maybe the original file layout is the right solution (having
> ppapi/host/error_conversion and ppapi/proxy/pp_utils).

I've restored ppapi/host/error_conversion.{cc|h}. But instead of
ppapi/proxy/pp_utils.{cc|h}
I'm using ppapi/proxy/error_conversion.{cc|h} now. This name is more descriptive
than pp_utils.

[ygorshenin1, 2013-08-22 07:52:29]

John, friendly ping.

[yzshen1, 2013-08-23 04:45:56]

On 2013/08/22 07:49:46, ygorshenin1 wrote:
> On 2013/08/21 17:22:58, yzshen1 wrote:
> > https://chromiumcodereview.appspot.com/22923014/diff/78002/ppapi/proxy/DEPS
> > File ppapi/proxy/DEPS (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/22923014/diff/78002/ppapi/proxy/DEPS#n...
> > ppapi/proxy/DEPS:7: "+net",
> > Hmm... I am sorry I didn't realize this.
> > We probably shouldn't let proxy depends on net, because proxy is also
compiled
> > at the untrusted side of NaCl.
> > So maybe the original file layout is the right solution (having
> > ppapi/host/error_conversion and ppapi/proxy/pp_utils).
> 
> I've restored ppapi/host/error_conversion.{cc|h}. But instead of
> ppapi/proxy/pp_utils.{cc|h}
> I'm using ppapi/proxy/error_conversion.{cc|h} now. This name is more
descriptive
> than pp_utils.

LGTM

Thanks!

[jam, 2013-08-25 16:46:37]

On 2013/08/22 07:52:29, ygorshenin1 wrote:
> John, friendly ping.

(was ooo)
chrome and content lgtm (didn't look at files inside pepper directories)

[ygorshenin1, 2013-08-26 12:10:37]

Justin, friendly ping.

[jschuh, 2013-08-27 22:58:05]

ipc security lgtm

[commit-bot: I haz the power, 2013-08-28 08:11:04]

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

[commit-bot: I haz the power, 2013-08-28 19:43:03]

Change committed as 220073

[ygorshenin1, 2013-08-30 09:11:38]

Many thanks to everybody!