PPB_TCPSocket_Private/PPB_UDPSocket_Private are exposed to Browser

PPB_{TCP|UDP}Socket_Private interfaces are exposed to Browser process. Added shared (between NaCl and Pepper) tests for both interfaces.

BUG=105859
TEST=ui_tests

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

[Dmitry Polukhin, 2011-11-24 13:05:45]

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
File
ppapi/native_client/tests/ppapi_browser/ppb_tcp_socket_private/ppapi_ppb_tcp_socket_private.html
(right):

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
ppapi/native_client/tests/ppapi_browser/ppb_tcp_socket_private/ppapi_ppb_tcp_socket_private.html:9:
<script type="text/javascript" src="ppapi_ppb_tcp_socket_private.js"></script>
Please keep line 80 chars.

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
File
ppapi/native_client/tests/ppapi_browser/ppb_udp_socket_private/ppapi_ppb_udp_socket_private.cc
(right):

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
ppapi/native_client/tests/ppapi_browser/ppb_udp_socket_private/ppapi_ppb_udp_socket_private.cc:19:
void GenerateNetAddress(PP_NetAddress_Private *address) {
Please add comment why it works and don't close socket until you generate both
UDP sockets.

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
File
ppapi/native_client/tests/ppapi_browser/ppb_udp_socket_private/ppapi_ppb_udp_socket_private.html
(right):

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
ppapi/native_client/tests/ppapi_browser/ppb_udp_socket_private/ppapi_ppb_udp_socket_private.html:9:
<script type="text/javascript" src="ppapi_ppb_udp_socket_private.js"></script>
Please keep line 80 chars.

http://codereview.chromium.org/8688002/diff/1/ppapi/proxy/ppb_udp_socket_priv...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/1/ppapi/proxy/ppb_udp_socket_priv...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:125: return (new
UDPSocket(HostResource::MakeInstanceOnly(instance),
Please return empty line back to minimize changes.

http://codereview.chromium.org/8688002/diff/1/webkit/plugins/ppapi/plugin_del...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/8688002/diff/1/webkit/plugins/ppapi/plugin_del...
webkit/plugins/ppapi/plugin_delegate.h:86: class PluginDelegate {
Please also implement stubs in MockPluginDelegate.

[ygorshenin, 2011-11-24 13:40:34]

PTAL

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
File
ppapi/native_client/tests/ppapi_browser/ppb_tcp_socket_private/ppapi_ppb_tcp_socket_private.html
(right):

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
ppapi/native_client/tests/ppapi_browser/ppb_tcp_socket_private/ppapi_ppb_tcp_socket_private.html:9:
<script type="text/javascript" src="ppapi_ppb_tcp_socket_private.js"></script>
On 2011/11/24 13:05:45, Dmitry Polukhin wrote:
> Please keep line 80 chars.

Done.

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
File
ppapi/native_client/tests/ppapi_browser/ppb_udp_socket_private/ppapi_ppb_udp_socket_private.cc
(right):

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
ppapi/native_client/tests/ppapi_browser/ppb_udp_socket_private/ppapi_ppb_udp_socket_private.cc:19:
void GenerateNetAddress(PP_NetAddress_Private *address) {
On 2011/11/24 13:05:45, Dmitry Polukhin wrote:
> Please add comment why it works and don't close socket until you generate both
> UDP sockets.

Done.

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
File
ppapi/native_client/tests/ppapi_browser/ppb_udp_socket_private/ppapi_ppb_udp_socket_private.html
(right):

http://codereview.chromium.org/8688002/diff/1/ppapi/native_client/tests/ppapi...
ppapi/native_client/tests/ppapi_browser/ppb_udp_socket_private/ppapi_ppb_udp_socket_private.html:9:
<script type="text/javascript" src="ppapi_ppb_udp_socket_private.js"></script>
On 2011/11/24 13:05:45, Dmitry Polukhin wrote:
> Please keep line 80 chars.

Done.

http://codereview.chromium.org/8688002/diff/1/ppapi/proxy/ppb_udp_socket_priv...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/1/ppapi/proxy/ppb_udp_socket_priv...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:125: return (new
UDPSocket(HostResource::MakeInstanceOnly(instance),
On 2011/11/24 13:05:45, Dmitry Polukhin wrote:
> Please return empty line back to minimize changes.

Done.

http://codereview.chromium.org/8688002/diff/1/webkit/plugins/ppapi/plugin_del...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/8688002/diff/1/webkit/plugins/ppapi/plugin_del...
webkit/plugins/ppapi/plugin_delegate.h:86: class PluginDelegate {
On 2011/11/24 13:05:45, Dmitry Polukhin wrote:
> Please also implement stubs in MockPluginDelegate.

Done.

[Dmitry Polukhin, 2011-11-24 13:44:35]

Yuzhu and Trung,
Please take a look to TCP/UDP implementation in render process. NaCl part of the
changes is here http://codereview.chromium.org/8555002

[Dmitry Polukhin, 2011-11-24 15:07:42]

+ darin@chromium.org for OWNER review in content/browser and webkit/
+ dmichael@chromium.org for OWNER review in ppapi/

[dmichael (off chromium), 2011-11-28 16:42:03]

Why are we proxying a private interface to NaCl at all? Can you explain the
rationale or point me to somebody else who can? I just want to make sure we're
not 'accidentally' poking a hole and letting untrusted apps use TCP/UDP. It
appears it's behind a flag, so maybe it's fine; I just want to make sure it's
intentional and there's a good strategy here.

General comment: it looks like your new tests aren't running on the trybots. I
think you need to add them to ppapi/native_client/tests/nacl.scons.

[polina, 2011-11-28 20:01:56]

The NaCl tests have been dropped and replaced with ppapi/tests, which ended up
in the other CL: http://codereview.chromium.org/8555002/

I had the same question about the private nature and naming of the interfaces.
The idea was to treat them the same as the special-purpose pdf interfaces, i.e.
give access with a special switch.

[dmichael (off chromium), 2011-11-28 20:38:43]

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:76: 
nit: extra carriage return added

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:115:
MessageLoop::current()->PostTask(FROM_HERE, new AbortCallbackTask(callback));
It looks like you could pretty easily replace AbortCallbackTask and just use
base::Bind. This can wait for another CL.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:101:
MessageLoop::current()->PostTask(FROM_HERE, new AbortCallbackTask(callback));
ditto, this should be changed to base::Bind (now or in a future CL)

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:41: memset(local_addr_.data, 0,
sizeof(local_addr_.data));
please use arraysize instead of sizeof. This will protect you if data somehow
became a pointer instead of an array. Same on 43.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:134: if (read_callback_.func ||
ssl_handshake_callback_.func)
A plugin will eventually be able to provide a completion callbacks with a null
func to mean 'block until completion' off the main thread. As written, your code
would allow another read to happen in that case. You should probably use a
different indicator for whether a read or handshake is in progress (here and
elsewhere).

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:41: memset(recvfrom_addr_.data, 0,
sizeof(recvfrom_addr_.data));
sizeof->arraysize

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:55: if (bind_callback_.func)
Here, too, I think it would be better to use some other way to check whether a
bind is in progress. In the future, func can realistically be NULL with a Bind
in progress. I think this comment applies pretty much everywhere you're using a
completion callback to decide if you should return PP_ERROR_INPROGRESS.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.h:75: bool binded_;
nit: binded->bound

[yzshen1, 2011-11-28 20:59:15]

Thanks!

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

http://codereview.chromium.org/8688002/diff/22001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:45: #include
"ppapi/proxy/ppb_tcp_socket_private_proxy.h"
you can probably remove this include.

http://codereview.chromium.org/8688002/diff/22001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:48: #include
"ppapi/shared_impl/udp_socket_impl.h"
this is not needed, right?

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

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1699: tcp_sockets_.AddWithID(
Two issues here:
1) the following scenario is broken:
- create a socket object.
- without calling connect or connectwithnetaddress, destroy it.

In this case, a disconnect notification still needs to be sent to the browser to
release resources at the browser side. However, because the socket ID is not
added to |tcp_sockets_|, a DCHECK is hit.

2) it seems wrong to me to always hold a ref of PPB_TCP_Socket_Private_Impl. It
keeps the resource alive, unless Disconnect() is explicitly called. (Calling
Disconnect from destructor doesn't work anymore, because the object is alive as
long as Disconnect doesn't get called.)

UDP socket has the same problem, too.

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1722:
DCHECK(tcp_sockets_.Lookup(socket_id));
For ALL ACK methods:

Please don't assume that socket_id is in the set.
It is possible that we receive ACKs after the socket is closed /disconnected
(e.g., the plugin may have explicitly requested to close/disconnect the socket).

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1724:
*tcp_sockets_.Lookup(socket_id);
For ALL ACK methods:

Don't de-reference it directly. You should first check whether the return value
is NULL.

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1730: const std::string&
server_name,
wrong indent.

http://codereview.chromium.org/8688002/diff/22001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/content/renderer/render_vie...
content/renderer/render_view_impl.h:292: // It is better to move this check to
Browser process but Pepper message
'Browser' -> 'browser', please.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:7: #include <algorithm>
You could remove those includes that are not needed.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:76: 
1) Please remove the empty line.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:79: void
TCPSocket::SendToBrowser(IPC::Message* msg) {
Please keep the definitions in the same order as the declarations.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:93: uint16_t server_port) {
Wrong indent.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:7: #include <algorithm>
You could remove those includes that are not needed.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:72: SendClose();
Please use Close().

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:75: void
UDPSocket::SendToBrowser(IPC::Message* msg) {
Please keep the definitions in the same order as the declarations.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:26: Disconnect();
We cannot call it here -- this calls virtual method.
Besides, you have called it in subclasses.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:29: void TCPSocketImpl::Init(uint32
socket_id) {
Sort the definitions in the same order as the declarations, please.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:41: memset(local_addr_.data, 0,
sizeof(local_addr_.data));
Please add include.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:138: bytes_to_read_ =
std::min(bytes_to_read, kMaxReadSize);
Please add include.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:17: // The functions that actually "do
stuff" like Connect are implemented
I think the code that actually "do stuff" is in the browser process and exactly
the same.
Do you mean the way of sending messages is different?

Same comment for UDPSocket.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:68: // Send functions that need to be
implemented differently differently for
duplicate 'differently'

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:77: virtual void
PostAbort(PP_CompletionCallback callback) = 0;
Is there any reason we need to let subclasses implement this method? The
implementation (as well as the Task class) at both renderer / plugin side looks
the same to me.

Same comment for PostAbort() in UDPSocket.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:26: Close();
You shouldn't call it here. (See comment in ~TCPSocketImpl)

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:29: void UDPSocketImpl::Init(uint32
socket_id) {
Sort the methods, please.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:41: memset(recvfrom_addr_.data, 0,
sizeof(recvfrom_addr_.data));
Add include, please.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:76: bytes_to_read_ = std::min(num_bytes,
kMaxReadSize);
add include.

http://codereview.chromium.org/8688002/diff/22001/webkit/glue/webkit_glue.gypi
File webkit/glue/webkit_glue.gypi (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/glue/webkit_glue.gyp...
webkit/glue/webkit_glue.gypi:290:
'../plugins/ppapi/ppb_tcp_socket_private_impl.cc',
Sort it, please.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:411: // For TCPSocket_Private.
Please use PPB_TCPSocket_Private.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:428: // For UDPSocket_Private.
Please use PPB_UDPSocket_Private.

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

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc:10: #include
"webkit/plugins/ppapi/common.h"
Why this is needed?

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc:13: #include
"webkit/plugins/ppapi/plugin_module.h"
Why this is needed?

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc:43:
PPB_TCPSocket_Private_Impl::~PPB_TCPSocket_Private_Impl() {
Please call Disconnect() here, as the subclass at the proxy side does.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
File webkit/plugins/ppapi/ppb_tcp_socket_private_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.h:17: virtual void
SendConnect(const std::string& host, uint16_t port);
Please use OVERRIDE.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.h:28: virtual
~PPB_TCPSocket_Private_Impl();
Is there any reason that we want to keep the destructor as private?

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
File webkit/plugins/ppapi/ppb_udp_socket_private_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
webkit/plugins/ppapi/ppb_udp_socket_private_impl.cc:10: #include
"webkit/plugins/ppapi/common.h"
Not needed.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
webkit/plugins/ppapi/ppb_udp_socket_private_impl.cc:13: #include
"webkit/plugins/ppapi/plugin_module.h"
Not needed.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
webkit/plugins/ppapi/ppb_udp_socket_private_impl.cc:43:
PPB_UDPSocket_Private_Impl::~PPB_UDPSocket_Private_Impl() {
Call Close(), please.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
File webkit/plugins/ppapi/ppb_udp_socket_private_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
webkit/plugins/ppapi/ppb_udp_socket_private_impl.h:26: virtual
~PPB_UDPSocket_Private_Impl();
Why it needs to be private?

[yzshen1, 2011-11-28 21:38:59]

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

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1699: tcp_sockets_.AddWithID(
On 2011/11/28 20:59:16, yzshen1 wrote:
> Two issues here:
> 1) the following scenario is broken:
> - create a socket object.
> - without calling connect or connectwithnetaddress, destroy it.
> 
> In this case, a disconnect notification still needs to be sent to the browser
to
> release resources at the browser side. However, because the socket ID is not
> added to |tcp_sockets_|, a DCHECK is hit.
> 
> 2) it seems wrong to me to always hold a ref of PPB_TCP_Socket_Private_Impl.
It
> keeps the resource alive, unless Disconnect() is explicitly called. (Calling
> Disconnect from destructor doesn't work anymore, because the object is alive
as
> long as Disconnect doesn't get called.)

I think one way to do it:
- don't hold a reference, just hold a raw pointer.
- when PPB_TCP_Socket_Private_Impl is destroyed, notify this object to remove
itself from the map.
- when receiving ACKs with a nonexistent socket ID, ignore them silently.


> 
> UDP socket has the same problem, too.

[yzshen1, 2011-11-28 22:11:14]

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:134: if (read_callback_.func ||
ssl_handshake_callback_.func)
On 2011/11/28 20:38:43, dmichael wrote:
> A plugin will eventually be able to provide a completion callbacks with a null
> func to mean 'block until completion' off the main thread. As written, your
code
> would allow another read to happen in that case.
Dave: with line 129-130, it won't get into this state.

Since I am the author of this code, I will be happy to make the change to enable
'block until completion' off the main thread.

But I am not sure about what should be changed. For example, what should be done
in OnReadCompleted() to notify the plugin in an off-main-thread-blocking
scenario. Would you please explain that to me or point me to relevant code/doc?
Thanks!

> You should probably use a
> different indicator for whether a read or handshake is in progress (here and
> elsewhere).

[dmichael (off chromium), 2011-11-28 22:21:27]

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:134: if (read_callback_.func ||
ssl_handshake_callback_.func)
On 2011/11/28 22:11:14, yzshen1 wrote:
> On 2011/11/28 20:38:43, dmichael wrote:
> > A plugin will eventually be able to provide a completion callbacks with a
null
> > func to mean 'block until completion' off the main thread. As written, your
> code
> > would allow another read to happen in that case.
> Dave: with line 129-130, it won't get into this state.
> 
> Since I am the author of this code, I will be happy to make the change to
enable
> 'block until completion' off the main thread.
> 
> But I am not sure about what should be changed. For example, what should be
done
> in OnReadCompleted() to notify the plugin in an off-main-thread-blocking
> scenario. Would you please explain that to me or point me to relevant
code/doc?
> Thanks!
> 
> > You should probably use a
> > different indicator for whether a read or handshake is in progress (here and
> > elsewhere).
> 
There's a lot of work still to be done for blocking completion callbacks; it
doesn't make sense to add that support here yet. I'm intending to implement a
completion callback helper at some point that will use a condition variable to
wake up the caller. There's going to be 1 big lock for the proxy, which is also
not implemented yet.

I agree, as-is, the callback function will work to signal if an operation is
in-progress. But I'm afraid it's a subtlety that will be missed later when
implementing blocking completion callbacks. And as it's written right now, it
could be a little confusing. A null 'func' has two possible meanings here:
blocking completion callback, or the associated operation is not in-progress.

If you think I'm being overly pedantic, please stick a TODO (put my name on it
if you like) to use some other strategy for determining if an operation is in
progress. This will be important later, but it doesn't need to hold up the CL.

[yzshen1, 2011-11-28 22:37:53]

On 2011/11/28 22:21:27, dmichael wrote:
>
http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
> File ppapi/shared_impl/tcp_socket_impl.cc (right):
> 
>
http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
> ppapi/shared_impl/tcp_socket_impl.cc:134: if (read_callback_.func ||
> ssl_handshake_callback_.func)
> On 2011/11/28 22:11:14, yzshen1 wrote:
> > On 2011/11/28 20:38:43, dmichael wrote:
> > > A plugin will eventually be able to provide a completion callbacks with a
> null
> > > func to mean 'block until completion' off the main thread. As written,
your
> > code
> > > would allow another read to happen in that case.
> > Dave: with line 129-130, it won't get into this state.
> > 
> > Since I am the author of this code, I will be happy to make the change to
> enable
> > 'block until completion' off the main thread.
> > 
> > But I am not sure about what should be changed. For example, what should be
> done
> > in OnReadCompleted() to notify the plugin in an off-main-thread-blocking
> > scenario. Would you please explain that to me or point me to relevant
> code/doc?
> > Thanks!
> > 
> > > You should probably use a
> > > different indicator for whether a read or handshake is in progress (here
and
> > > elsewhere).
> > 
> There's a lot of work still to be done for blocking completion callbacks; it
> doesn't make sense to add that support here yet. I'm intending to implement a
> completion callback helper at some point that will use a condition variable to
> wake up the caller. There's going to be 1 big lock for the proxy, which is
also
> not implemented yet.
> 
> I agree, as-is, the callback function will work to signal if an operation is
> in-progress. But I'm afraid it's a subtlety that will be missed later when
> implementing blocking completion callbacks. And as it's written right now, it
> could be a little confusing. A null 'func' has two possible meanings here:
> blocking completion callback, or the associated operation is not in-progress.
> 
> If you think I'm being overly pedantic, please stick a TODO (put my name on it
> if you like) to use some other strategy for determining if an operation is in
> progress. This will be important later, but it doesn't need to hold up the CL.

Thanks for your explanation, Dave!

Yuri: if you would like to make the change, please go ahead. otherwise, please
add a TODO with my name on it. :) Thanks!

[ygorshenin, 2011-11-29 18:30:08]

PTAL

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

http://codereview.chromium.org/8688002/diff/22001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:45: #include
"ppapi/proxy/ppb_tcp_socket_private_proxy.h"
On 2011/11/28 20:59:16, yzshen1 wrote:
> you can probably remove this include.

Done.

http://codereview.chromium.org/8688002/diff/22001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:48: #include
"ppapi/shared_impl/udp_socket_impl.h"
On 2011/11/28 20:59:16, yzshen1 wrote:
> this is not needed, right?

"ppapi/shared_impl/udp_socket_impl.h" is not needed.

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

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1699: tcp_sockets_.AddWithID(
On 2011/11/28 20:59:16, yzshen1 wrote:
> Two issues here:
> 1) the following scenario is broken:
> - create a socket object.
> - without calling connect or connectwithnetaddress, destroy it.
> 
> In this case, a disconnect notification still needs to be sent to the browser
to
> release resources at the browser side. However, because the socket ID is not
> added to |tcp_sockets_|, a DCHECK is hit.
> 
> 2) it seems wrong to me to always hold a ref of PPB_TCP_Socket_Private_Impl.
It
> keeps the resource alive, unless Disconnect() is explicitly called. (Calling
> Disconnect from destructor doesn't work anymore, because the object is alive
as
> long as Disconnect doesn't get called.)
> 
> UDP socket has the same problem, too.

Done.

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1699: tcp_sockets_.AddWithID(
On 2011/11/28 21:38:59, yzshen1 wrote:
> On 2011/11/28 20:59:16, yzshen1 wrote:
> > Two issues here:
> > 1) the following scenario is broken:
> > - create a socket object.
> > - without calling connect or connectwithnetaddress, destroy it.
> > 
> > In this case, a disconnect notification still needs to be sent to the
browser
> to
> > release resources at the browser side. However, because the socket ID is not
> > added to |tcp_sockets_|, a DCHECK is hit.
> > 
> > 2) it seems wrong to me to always hold a ref of PPB_TCP_Socket_Private_Impl.
> It
> > keeps the resource alive, unless Disconnect() is explicitly called. (Calling
> > Disconnect from destructor doesn't work anymore, because the object is alive
> as
> > long as Disconnect doesn't get called.)
> 
> I think one way to do it:
> - don't hold a reference, just hold a raw pointer.
> - when PPB_TCP_Socket_Private_Impl is destroyed, notify this object to remove
> itself from the map.
> - when receiving ACKs with a nonexistent socket ID, ignore them silently.
> 
> 
> > 
> > UDP socket has the same problem, too.
> 

Done.

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1722:
DCHECK(tcp_sockets_.Lookup(socket_id));
On 2011/11/28 20:59:16, yzshen1 wrote:
> For ALL ACK methods:
> 
> Please don't assume that socket_id is in the set.
> It is possible that we receive ACKs after the socket is closed /disconnected
> (e.g., the plugin may have explicitly requested to close/disconnect the
socket).

Done.

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1724:
*tcp_sockets_.Lookup(socket_id);
On 2011/11/28 20:59:16, yzshen1 wrote:
> For ALL ACK methods:
> 
> Don't de-reference it directly. You should first check whether the return
value
> is NULL.

Done.

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1730: const std::string&
server_name,
On 2011/11/28 20:59:16, yzshen1 wrote:
> wrong indent.

Done.

http://codereview.chromium.org/8688002/diff/22001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/content/renderer/render_vie...
content/renderer/render_view_impl.h:292: // It is better to move this check to
Browser process but Pepper message
On 2011/11/28 20:59:16, yzshen1 wrote:
> 'Browser' -> 'browser', please.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:7: #include <algorithm>
On 2011/11/28 20:59:16, yzshen1 wrote:
> You could remove those includes that are not needed.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:76: 
On 2011/11/28 20:59:16, yzshen1 wrote:
> 1) Please remove the empty line.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:76: 
On 2011/11/28 20:38:43, dmichael wrote:
> nit: extra carriage return added

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:79: void
TCPSocket::SendToBrowser(IPC::Message* msg) {
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please keep the definitions in the same order as the declarations.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:93: uint16_t server_port) {
On 2011/11/28 20:59:16, yzshen1 wrote:
> Wrong indent.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:115:
MessageLoop::current()->PostTask(FROM_HERE, new AbortCallbackTask(callback));
On 2011/11/28 20:38:43, dmichael wrote:
> It looks like you could pretty easily replace AbortCallbackTask and just use
> base::Bind. This can wait for another CL.

Added TODO comment.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:7: #include <algorithm>
On 2011/11/28 20:59:16, yzshen1 wrote:
> You could remove those includes that are not needed.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:72: SendClose();
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please use Close().

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:75: void
UDPSocket::SendToBrowser(IPC::Message* msg) {
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please keep the definitions in the same order as the declarations.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:101:
MessageLoop::current()->PostTask(FROM_HERE, new AbortCallbackTask(callback));
On 2011/11/28 20:38:43, dmichael wrote:
> ditto, this should be changed to base::Bind (now or in a future CL)

Added TODO comment.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:26: Disconnect();
On 2011/11/28 20:59:16, yzshen1 wrote:
> We cannot call it here -- this calls virtual method.
> Besides, you have called it in subclasses.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:29: void TCPSocketImpl::Init(uint32
socket_id) {
On 2011/11/28 20:59:16, yzshen1 wrote:
> Sort the definitions in the same order as the declarations, please.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:41: memset(local_addr_.data, 0,
sizeof(local_addr_.data));
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please add include.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:41: memset(local_addr_.data, 0,
sizeof(local_addr_.data));
On 2011/11/28 20:38:43, dmichael wrote:
> please use arraysize instead of sizeof. This will protect you if data somehow
> became a pointer instead of an array. Same on 43.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:134: if (read_callback_.func ||
ssl_handshake_callback_.func)
On 2011/11/28 22:21:27, dmichael wrote:
> On 2011/11/28 22:11:14, yzshen1 wrote:
> > On 2011/11/28 20:38:43, dmichael wrote:
> > > A plugin will eventually be able to provide a completion callbacks with a
> null
> > > func to mean 'block until completion' off the main thread. As written,
your
> > code
> > > would allow another read to happen in that case.
> > Dave: with line 129-130, it won't get into this state.
> > 
> > Since I am the author of this code, I will be happy to make the change to
> enable
> > 'block until completion' off the main thread.
> > 
> > But I am not sure about what should be changed. For example, what should be
> done
> > in OnReadCompleted() to notify the plugin in an off-main-thread-blocking
> > scenario. Would you please explain that to me or point me to relevant
> code/doc?
> > Thanks!
> > 
> > > You should probably use a
> > > different indicator for whether a read or handshake is in progress (here
and
> > > elsewhere).
> > 
> There's a lot of work still to be done for blocking completion callbacks; it
> doesn't make sense to add that support here yet. I'm intending to implement a
> completion callback helper at some point that will use a condition variable to
> wake up the caller. There's going to be 1 big lock for the proxy, which is
also
> not implemented yet.
> 
> I agree, as-is, the callback function will work to signal if an operation is
> in-progress. But I'm afraid it's a subtlety that will be missed later when
> implementing blocking completion callbacks. And as it's written right now, it
> could be a little confusing. A null 'func' has two possible meanings here:
> blocking completion callback, or the associated operation is not in-progress.
> 
> If you think I'm being overly pedantic, please stick a TODO (put my name on it
> if you like) to use some other strategy for determining if an operation is in
> progress. This will be important later, but it doesn't need to hold up the CL.

Added TODO comment.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:138: bytes_to_read_ =
std::min(bytes_to_read, kMaxReadSize);
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please add include.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:17: // The functions that actually "do
stuff" like Connect are implemented
On 2011/11/28 20:59:16, yzshen1 wrote:
> I think the code that actually "do stuff" is in the browser process and
exactly
> the same.
> Do you mean the way of sending messages is different?
> 
> Same comment for UDPSocket.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:68: // Send functions that need to be
implemented differently differently for
On 2011/11/28 20:59:16, yzshen1 wrote:
> duplicate 'differently'

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:77: virtual void
PostAbort(PP_CompletionCallback callback) = 0;
On 2011/11/28 20:59:16, yzshen1 wrote:
> Is there any reason we need to let subclasses implement this method? The
> implementation (as well as the Task class) at both renderer / plugin side
looks
> the same to me.
> 
> Same comment for PostAbort() in UDPSocket.

Yes. Base class is placed into ppapi/shared_impl directory. This is
ideologically incorrect to include "base/message_loop.h" and other chrome
headers from this directory.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:26: Close();
On 2011/11/28 20:59:16, yzshen1 wrote:
> You shouldn't call it here. (See comment in ~TCPSocketImpl)

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:29: void UDPSocketImpl::Init(uint32
socket_id) {
On 2011/11/28 20:59:16, yzshen1 wrote:
> Sort the methods, please.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:41: memset(recvfrom_addr_.data, 0,
sizeof(recvfrom_addr_.data));
On 2011/11/28 20:38:43, dmichael wrote:
> sizeof->arraysize

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:41: memset(recvfrom_addr_.data, 0,
sizeof(recvfrom_addr_.data));
On 2011/11/28 20:59:16, yzshen1 wrote:
> Add include, please.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:55: if (bind_callback_.func)
On 2011/11/28 20:38:43, dmichael wrote:
> Here, too, I think it would be better to use some other way to check whether a
> bind is in progress. In the future, func can realistically be NULL with a Bind
> in progress. I think this comment applies pretty much everywhere you're using
a
> completion callback to decide if you should return PP_ERROR_INPROGRESS.

Added TODO comment.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:76: bytes_to_read_ = std::min(num_bytes,
kMaxReadSize);
On 2011/11/28 20:59:16, yzshen1 wrote:
> add include.

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.h:75: bool binded_;
On 2011/11/28 20:38:43, dmichael wrote:
> nit: binded->bound

Done.

http://codereview.chromium.org/8688002/diff/22001/webkit/glue/webkit_glue.gypi
File webkit/glue/webkit_glue.gypi (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/glue/webkit_glue.gyp...
webkit/glue/webkit_glue.gypi:290:
'../plugins/ppapi/ppb_tcp_socket_private_impl.cc',
On 2011/11/28 20:59:16, yzshen1 wrote:
> Sort it, please.

Done.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:411: // For TCPSocket_Private.
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please use PPB_TCPSocket_Private.

Done.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:428: // For UDPSocket_Private.
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please use PPB_UDPSocket_Private.

Done.

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

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc:10: #include
"webkit/plugins/ppapi/common.h"
On 2011/11/28 20:59:16, yzshen1 wrote:
> Why this is needed?

Deleted.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc:13: #include
"webkit/plugins/ppapi/plugin_module.h"
On 2011/11/28 20:59:16, yzshen1 wrote:
> Why this is needed?

Deleted.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.cc:43:
PPB_TCPSocket_Private_Impl::~PPB_TCPSocket_Private_Impl() {
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please call Disconnect() here, as the subclass at the proxy side does.

Done.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
File webkit/plugins/ppapi/ppb_tcp_socket_private_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.h:17: virtual void
SendConnect(const std::string& host, uint16_t port);
On 2011/11/28 20:59:16, yzshen1 wrote:
> Please use OVERRIDE.

Done.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_tc...
webkit/plugins/ppapi/ppb_tcp_socket_private_impl.h:28: virtual
~PPB_TCPSocket_Private_Impl();
On 2011/11/28 20:59:16, yzshen1 wrote:
> Is there any reason that we want to keep the destructor as private?
On 2011/11/28 20:59:16, yzshen1 wrote:> Why it needs to be private?
There are no possibility to create instance of this class. Objects will be
destroyed via base class destructor. So, destructor is private only for
consistency with private constructor and readability.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
File webkit/plugins/ppapi/ppb_udp_socket_private_impl.cc (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
webkit/plugins/ppapi/ppb_udp_socket_private_impl.cc:10: #include
"webkit/plugins/ppapi/common.h"
On 2011/11/28 20:59:16, yzshen1 wrote:
> Not needed.

Done.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
webkit/plugins/ppapi/ppb_udp_socket_private_impl.cc:13: #include
"webkit/plugins/ppapi/plugin_module.h"
On 2011/11/28 20:59:16, yzshen1 wrote:
> Not needed.

Done.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
webkit/plugins/ppapi/ppb_udp_socket_private_impl.cc:43:
PPB_UDPSocket_Private_Impl::~PPB_UDPSocket_Private_Impl() {
On 2011/11/28 20:59:16, yzshen1 wrote:
> Call Close(), please.

Done.

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
File webkit/plugins/ppapi/ppb_udp_socket_private_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/webkit/plugins/ppapi/ppb_ud...
webkit/plugins/ppapi/ppb_udp_socket_private_impl.h:26: virtual
~PPB_UDPSocket_Private_Impl();
On 2011/11/28 20:59:16, yzshen1 wrote:
> Why it needs to be private?

There are no possibility to create instance of this class. Objects will be
destroyed via base class destructor. So, destructor is private only for
consistency with private constructor and readability.

[dmichael (off chromium), 2011-11-29 20:10:10]

ppapi stuff lgtm

[yzshen1, 2011-11-29 20:19:32]

Thanks!

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

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1780:
DCHECK(tcp_sockets_.Lookup(socket_id));
Add comment about why we don't do DCHECK(tcp_sockets_.Lookup(socket_id)) here.

And do the same for UDPSocketClose().

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:11: #include "base/logging.h"
This is still needed.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:11: #include "base/logging.h"
This is still needed, right?

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

http://codereview.chromium.org/8688002/diff/28006/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1725: uint32 socket_id, const
std::string& server_name, uint16_t server_port) {
Nit: For definition / declaration, either put the whole signature in one line,
or put each parameter on a separate line (the first parameter can be on the same
line as the method name, in that case, all the remaining parameters should align
with it).

(It is okay to arrange parameters like this if it is a method/function
invocation.)

http://www.chromium.org/developers/coding-style

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

http://codereview.chromium.org/8688002/diff/28006/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:26: #include
"ppapi/shared_impl/tcp_socket_impl.h"
Sort, please. And remove the empty line.

http://codereview.chromium.org/8688002/diff/28006/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:8: #include <string>
You have included it in .h file.

http://codereview.chromium.org/8688002/diff/28006/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:7: #include <string>
You have included it in .h file.

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:8: #include <algorithm>
a blank line between c header and c++ header, please.

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:17: // PPB_TCPSocket_Private.  The functions
that actually send methods to
methods -> messages.

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:8: #include <algorithm>
an empty line between c and c++ headers, please.

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.h:17: // PPB_UDPSocket_Private.  The functions
that actually send methods to
methods -> messages?

[ygorshenin, 2011-11-30 11:50:53]

PTAL

Moved tests from http://codereview.chromium.org/8555002/ (already viewed by
Polina).

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

http://codereview.chromium.org/8688002/diff/22001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1780:
DCHECK(tcp_sockets_.Lookup(socket_id));
On 2011/11/29 20:19:32, yzshen1 wrote:
> Add comment about why we don't do DCHECK(tcp_sockets_.Lookup(socket_id)) here.
> 
> And do the same for UDPSocketClose().

Done.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:11: #include "base/logging.h"
On 2011/11/29 20:19:32, yzshen1 wrote:
> This is still needed, right?

No, this is not needed. Deleted.

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

http://codereview.chromium.org/8688002/diff/28006/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1725: uint32 socket_id, const
std::string& server_name, uint16_t server_port) {
On 2011/11/29 20:19:33, yzshen1 wrote:
> Nit: For definition / declaration, either put the whole signature in one line,
> or put each parameter on a separate line (the first parameter can be on the
same
> line as the method name, in that case, all the remaining parameters should
align
> with it).
> 
> (It is okay to arrange parameters like this if it is a method/function
> invocation.)
> 
> http://www.chromium.org/developers/coding-style

Done.

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

http://codereview.chromium.org/8688002/diff/28006/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:26: #include
"ppapi/shared_impl/tcp_socket_impl.h"
On 2011/11/29 20:19:33, yzshen1 wrote:
> Sort, please. And remove the empty line.

Done.

http://codereview.chromium.org/8688002/diff/28006/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:8: #include <string>
On 2011/11/29 20:19:33, yzshen1 wrote:
> You have included it in .h file. 

Done.

http://codereview.chromium.org/8688002/diff/28006/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:7: #include <string>
On 2011/11/29 20:19:33, yzshen1 wrote:
> You have included it in .h file.

Done.

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.cc:8: #include <algorithm>
On 2011/11/29 20:19:33, yzshen1 wrote:
> a blank line between c header and c++ header, please.

Done.

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:17: // PPB_TCPSocket_Private.  The functions
that actually send methods to
On 2011/11/29 20:19:33, yzshen1 wrote:
> methods -> messages.

Done.

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.cc (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.cc:8: #include <algorithm>
On 2011/11/29 20:19:33, yzshen1 wrote:
> an empty line between c and c++ headers, please.

Done.

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/udp_socke...
File ppapi/shared_impl/udp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/28006/ppapi/shared_impl/udp_socke...
ppapi/shared_impl/udp_socket_impl.h:17: // PPB_UDPSocket_Private.  The functions
that actually send methods to
On 2011/11/29 20:19:33, yzshen1 wrote:
> methods -> messages?

Done.

[Dmitry Polukhin, 2011-11-30 18:45:03]

Darin, please take a look to this CL we need OWNER review in content/browser/
and webkit/ but only 2 days left.

[yzshen1, 2011-11-30 21:11:03]

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:11: #include "base/logging.h"
On 2011/11/29 20:19:32, yzshen1 wrote:
> This is still needed.

This is still needed, because DCHECK is used in this file.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:11: #include "base/logging.h"
On 2011/11/30 11:50:54, ygorshenin wrote:
> On 2011/11/29 20:19:32, yzshen1 wrote:
> > This is still needed, right?
> 
> No, this is not needed. Deleted.

This is still needed, because DCHECK is used in this file.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:77: virtual void
PostAbort(PP_CompletionCallback callback) = 0;
On 2011/11/29 18:30:09, ygorshenin wrote:
> On 2011/11/28 20:59:16, yzshen1 wrote:
> > Is there any reason we need to let subclasses implement this method? The
> > implementation (as well as the Task class) at both renderer / plugin side
> looks
> > the same to me.
> > 
> > Same comment for PostAbort() in UDPSocket.
> 
> Yes. Base class is placed into ppapi/shared_impl directory. This is
> ideologically incorrect to include "base/message_loop.h" and other chrome
> headers from this directory.

I don't think it is an issue to use "base/message_loop.h" in shared_impl.
(I have also double-checked with vtl and brettw.)

http://codereview.chromium.org/8688002/diff/37017/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_socket.cc (right):

http://codereview.chromium.org/8688002/diff/37017/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_socket.cc:27: #include
"ppapi/proxy/ppb_tcp_socket_private_proxy.h"
Remove this one.

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

http://codereview.chromium.org/8688002/diff/37017/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1774: // TCPSocketDisconnect may
be called more than once.
because it can be called before TCPSocketConnect/TCPSocketConnectWithNetAddress
is called.

It is guaranteed that this will not be called more than once.

Same comment for UDPSocket.

http://codereview.chromium.org/8688002/diff/37017/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/37017/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:6: #define
PPAPI_SHARED_IMPL_TCP_SOCKET_IMPL_H_
Please move {tcp,udp}_socket_impl.{h,cc} into shared_impl/private

[ygorshenin, 2011-12-01 10:58:31]

PTAL

+tony@chromium.org for OWNER review in webkit/
+jam@chromium.org for OWNER review in content/

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:11: #include "base/logging.h"
On 2011/11/30 21:11:04, yzshen1 wrote:
> On 2011/11/29 20:19:32, yzshen1 wrote:
> > This is still needed.
> 
> This is still needed, because DCHECK is used in this file.

Restored.

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
File ppapi/proxy/ppb_udp_socket_private_proxy.cc (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/proxy/ppb_udp_socket_...
ppapi/proxy/ppb_udp_socket_private_proxy.cc:11: #include "base/logging.h"
On 2011/11/30 21:11:04, yzshen1 wrote:
> On 2011/11/30 11:50:54, ygorshenin wrote:
> > On 2011/11/29 20:19:32, yzshen1 wrote:
> > > This is still needed, right?
> > 
> > No, this is not needed. Deleted.
> 
> This is still needed, because DCHECK is used in this file.
> 

Restored.

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/22001/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:77: virtual void
PostAbort(PP_CompletionCallback callback) = 0;
On 2011/11/30 21:11:04, yzshen1 wrote:
> On 2011/11/29 18:30:09, ygorshenin wrote:
> > On 2011/11/28 20:59:16, yzshen1 wrote:
> > > Is there any reason we need to let subclasses implement this method? The
> > > implementation (as well as the Task class) at both renderer / plugin side
> > looks
> > > the same to me.
> > > 
> > > Same comment for PostAbort() in UDPSocket.
> > 
> > Yes. Base class is placed into ppapi/shared_impl directory. This is
> > ideologically incorrect to include "base/message_loop.h" and other chrome
> > headers from this directory.
> 
> I don't think it is an issue to use "base/message_loop.h" in shared_impl.
> (I have also double-checked with vtl and brettw.)
> 

Done.

http://codereview.chromium.org/8688002/diff/37017/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_socket.cc (right):

http://codereview.chromium.org/8688002/diff/37017/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_socket.cc:27: #include
"ppapi/proxy/ppb_tcp_socket_private_proxy.h"
On 2011/11/30 21:11:04, yzshen1 wrote:
> Remove this one.

Done.

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

http://codereview.chromium.org/8688002/diff/37017/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:1774: // TCPSocketDisconnect may
be called more than once.
On 2011/11/30 21:11:04, yzshen1 wrote:
> because it can be called before
TCPSocketConnect/TCPSocketConnectWithNetAddress
> is called.
> 
> It is guaranteed that this will not be called more than once.
> 
> Same comment for UDPSocket.

Done.

http://codereview.chromium.org/8688002/diff/37017/ppapi/shared_impl/tcp_socke...
File ppapi/shared_impl/tcp_socket_impl.h (right):

http://codereview.chromium.org/8688002/diff/37017/ppapi/shared_impl/tcp_socke...
ppapi/shared_impl/tcp_socket_impl.h:6: #define
PPAPI_SHARED_IMPL_TCP_SOCKET_IMPL_H_
On 2011/11/30 21:11:04, yzshen1 wrote:
> Please move {tcp,udp}_socket_impl.{h,cc} into shared_impl/private

Done.

[yzshen1, 2011-12-01 18:34:23]

Thanks!

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:255: void
TCPSocketPrivateImpl::PostAbort(PP_CompletionCallback callback) {
It seems this method can be merged into PostAbortAndClearIfNecessary(), doesn't
it?

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_socket_private_impl.h (right):

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.h:79: void
PostAbort(PP_CompletionCallback callback);
This could be removed.

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/u...
File ppapi/shared_impl/private/udp_socket_private_impl.h (right):

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/u...
ppapi/shared_impl/private/udp_socket_private_impl.h:69: void
PostAbort(PP_CompletionCallback callback);
This could be removed.

[tony, 2011-12-01 18:54:30]

The changes to webkit_glue.gypi LGTM.  Please ask an owner of
webkit/plugins/ppapi to review the changes in that directory.

[jam, 2011-12-01 19:50:31]

just looked at content changes. i'm unfamiliar with the code you're changing in
webkit/plugins/ppapi, so please ask one of the owners there to have a look

http://codereview.chromium.org/8688002/diff/51001/content/public/common/conte...
File content/public/common/content_switches.h (right):

http://codereview.chromium.org/8688002/diff/51001/content/public/common/conte...
content/public/common/content_switches.h:17: CONTENT_EXPORT extern const char
kAllowNaClSocketAPI[];
this doesn't belong in content, since it's NaCl related which is in chrome.

so move this out to chrome, and have it be propagated to the renderer by editing
ChromeContentBrowserClient::AppendExtraCommandLineSwitches instead of
RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:438:
command_line.GetSwitchValueASCII(switches::kAllowNaClSocketAPI);
here and in the header, please don't mention nacl in content. see
http://www.chromium.org/developers/content-module

if you want to have the core code in content, you can ask the embedder whether
to allow this feature or not through the ContentRendererClient interface.
Chrome's implementation can then use the nacl flag

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:4755: uint32 /* plugin_dispatcher_id */,
nit: here and below, commenting out parameter names because they're not used
goes against convention

[Dmitry Polukhin, 2011-12-01 19:52:21]

+ brettw@chromium.org for OWNER review in webkit/plugins/ppapi

Yuzhu, thank you a lot for reviewing our changes. We will definitely resolve all
your comments when we get back to office. Now it looks like we resolved all
major concerns. If it is the case, could you please bless this CL so we would
wait for owners review only? We have only 1 day to commit it to guarantee that
our changes will be in M17 (due to time difference it is even less for us).

[yzshen1, 2011-12-01 21:09:01]

lgtm

[brettw, 2011-12-02 03:56:13]

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:283: "okddffdblfhhnmhodogpojmfkjmhinfp"
What is this?

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:4760:
pepper_delegate_.OnTCPSocketConnectACK(
Is it possible to add a filter from the pepper delegate rather than have all
these modifications here? This is a lot of modifications to this critical file.

You can see the beginning of OnMessageReceived does some dispatch to various
observers before falling through to the regular message handling.

[Dmitry Polukhin, 2011-12-02 11:37:32]

http://codereview.chromium.org/8688002/diff/51001/content/public/common/conte...
File content/public/common/content_switches.h (right):

http://codereview.chromium.org/8688002/diff/51001/content/public/common/conte...
content/public/common/content_switches.h:17: CONTENT_EXPORT extern const char
kAllowNaClSocketAPI[];
On 2011/12/01 19:50:31, John Abd-El-Malek wrote:
> this doesn't belong in content, since it's NaCl related which is in chrome.
> 
> so move this out to chrome, and have it be propagated to the renderer by
editing
> ChromeContentBrowserClient::AppendExtraCommandLineSwitches instead of
> RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer

I see few other NaCl flags with TODOs about moving to other location. I also
added TODO to our flag. We will do it in a proper way after branch point without
hurry.

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:438:
command_line.GetSwitchValueASCII(switches::kAllowNaClSocketAPI);
On 2011/12/01 19:50:31, John Abd-El-Malek wrote:
> here and in the header, please don't mention nacl in content. see
> http://www.chromium.org/developers/content-module
> 
> if you want to have the core code in content, you can ask the embedder whether
> to allow this feature or not through the ContentRendererClient interface.
> Chrome's implementation can then use the nacl flag

Will do it in next cl, see my replay in header.

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:4755: uint32 /* plugin_dispatcher_id */,
On 2011/12/01 19:50:31, John Abd-El-Malek wrote:
> nit: here and below, commenting out parameter names because they're not used
> goes against convention

Done.

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:4760:
pepper_delegate_.OnTCPSocketConnectACK(
On 2011/12/02 03:56:14, brettw wrote:
> Is it possible to add a filter from the pepper delegate rather than have all
> these modifications here? This is a lot of modifications to this critical
file.
> 
> You can see the beginning of OnMessageReceived does some dispatch to various
> observers before falling through to the regular message handling.

Potential yes but not in given time frame before branch point. Added TODO. Other
interfaces do the same. So it is general thing to move bunch of messages
handlers to PepperPluginDelegateImpl.

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:255: void
TCPSocketPrivateImpl::PostAbort(PP_CompletionCallback callback) {
On 2011/12/01 18:34:23, yzshen1 wrote:
> It seems this method can be merged into PostAbortAndClearIfNecessary(),
doesn't
> it?

Done.

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_socket_private_impl.h (right):

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.h:79: void
PostAbort(PP_CompletionCallback callback);
On 2011/12/01 18:34:23, yzshen1 wrote:
> This could be removed.

Done.

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/u...
File ppapi/shared_impl/private/udp_socket_private_impl.h (right):

http://codereview.chromium.org/8688002/diff/51001/ppapi/shared_impl/private/u...
ppapi/shared_impl/private/udp_socket_private_impl.h:69: void
PostAbort(PP_CompletionCallback callback);
On 2011/12/01 18:34:23, yzshen1 wrote:
> This could be removed.

Done.

[jam, 2011-12-02 18:52:21]

On 2011/12/02 11:37:32, Dmitry Polukhin wrote:
>
http://codereview.chromium.org/8688002/diff/51001/content/public/common/conte...
> File content/public/common/content_switches.h (right):
> 
>
http://codereview.chromium.org/8688002/diff/51001/content/public/common/conte...
> content/public/common/content_switches.h:17: CONTENT_EXPORT extern const char
> kAllowNaClSocketAPI[];
> On 2011/12/01 19:50:31, John Abd-El-Malek wrote:
> > this doesn't belong in content, since it's NaCl related which is in chrome.
> > 
> > so move this out to chrome, and have it be propagated to the renderer by
> editing
> > ChromeContentBrowserClient::AppendExtraCommandLineSwitches instead of
> > RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer
> 
> I see few other NaCl flags with TODOs about moving to other location. I also
> added TODO to our flag. We will do it in a proper way after branch point
without
> hurry.

This isn't how we do things. You can't just override OWNERS while there are
still outstanding comments. We've been spending a large amount of time
uncoupling content from nacl. You're adding more work here. Additionally, I
wasn't even finished with my review.

[jam, 2011-12-02 19:17:48]

Based on discussion with others, I'm reverting this change. The main issues is
that this was improperly committed, because there were outstanding comments from
other reviewers, and also there were no OWNERS approval.

If you're in a rush to get this in, feel free to IM or email me and I will
review updated revisions. The two things that should be taken care of are the
nacl issue and filtering the messages outside of RenderViewImpl. While these may
seem minor, a number of people have been spending this whole year refactoring
this code to simplify RenderView and to separate nacl from content, and so this
is taking a step back.

[darin (slow to review), 2011-12-02 20:34:42]

http://codereview.chromium.org/8688002/diff/56007/content/public/common/conte...
File content/public/common/content_switches.h (right):

http://codereview.chromium.org/8688002/diff/56007/content/public/common/conte...
content/public/common/content_switches.h:18: CONTENT_EXPORT extern const char
kAllowNaClSocketAPI[];
Is this really the NaCl socket API?  The socket API is part of Pepper.  Should
this be kAllowPepperSocketAPI instead?

What is NaCl-specific about this CL?

[zel, 2011-12-02 20:51:42]

On 2011/12/02 19:17:48, John Abd-El-Malek wrote:
> Based on discussion with others, I'm reverting this change. The main issues is
> that this was improperly committed, because there were outstanding comments
from
> other reviewers, and also there were no OWNERS approval.
> 
> If you're in a rush to get this in, feel free to IM or email me and I will
> review updated revisions. The two things that should be taken care of are the
> nacl issue and filtering the messages outside of RenderViewImpl. While these
may
> seem minor, a number of people have been spending this whole year refactoring
> this code to simplify RenderView and to separate nacl from content, and so
this
> is taking a step back.

First, let me I apologize for creating pressure around landing this CL before
M17 branch point. We should not have shortcut the review process because of
that.

Dmitry, let's address John's existing comments first before we land this CL on
TOT. I will make sure that we merge this into the branch even if the branch gets
cut before this CL lands.

John, you said in your previous comment that you haven't completed your review
yet. I would appreciate if you can finish the missing part soon, so Dmitry does
not have to make several CR round trips across timezones. 

Thanks!

[jam, 2011-12-02 20:55:45]

On Fri, Dec 2, 2011 at 12:51 PM, <zelidrag@chromium.org> wrote:

> On 2011/12/02 19:17:48, John Abd-El-Malek wrote:
>
>> Based on discussion with others, I'm reverting this change. The main
>> issues is
>> that this was improperly committed, because there were outstanding
>> comments
>>
> from
>
>> other reviewers, and also there were no OWNERS approval.
>>
>
>  If you're in a rush to get this in, feel free to IM or email me and I will
>> review updated revisions. The two things that should be taken care of are
>> the
>> nacl issue and filtering the messages outside of RenderViewImpl. While
>> these
>>
> may
>
>> seem minor, a number of people have been spending this whole year
>> refactoring
>> this code to simplify RenderView and to separate nacl from content, and so
>>
> this
>
>> is taking a step back.
>>
>
> First, let me I apologize for creating pressure around landing this CL
> before
> M17 branch point. We should not have shortcut the review process because of
> that.
>
> Dmitry, let's address John's existing comments first before we land this
> CL on
> TOT. I will make sure that we merge this into the branch even if the
> branch gets
> cut before this CL lands.
>
> John, you said in your previous comment that you haven't completed your
> review
> yet. I would appreciate if you can finish the missing part soon, so Dmitry
> does
> not have to make several CR round trips across timezones.
>

Good point, I should have clarified that. The only other issue I had was
what Brett commented on as well. The IPCs should be dispatched directly
where they're needed.

>
> Thanks!
>
>
http://codereview.chromium.**org/8688002/<http://codereview.chromium.org/8688...
>

[yzshen1, 2011-12-02 21:19:26]

http://codereview.chromium.org/8688002/diff/56007/content/public/common/conte...
File content/public/common/content_switches.h (right):

http://codereview.chromium.org/8688002/diff/56007/content/public/common/conte...
content/public/common/content_switches.h:18: CONTENT_EXPORT extern const char
kAllowNaClSocketAPI[];
On 2011/12/02 20:34:42, darin wrote:
> Is this really the NaCl socket API?  The socket API is part of Pepper.  Should
> this be kAllowPepperSocketAPI instead?
> 
> What is NaCl-specific about this CL?

When the socket API is used by non-NaCl pepper plugins (Flapper), we don't
restrict it to a set of selective origins. So it seems appropriate to have NaCl
in the name.

But I think it is better not to enforce the origin restriction in render view.
It will affect not only NaCl, but also other in-process pepper plugins.

[brettw, 2011-12-02 21:26:32]

On Fri, Dec 2, 2011 at 12:55 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Fri, Dec 2, 2011 at 12:51 PM, <zelidrag@chromium.org> wrote:
>>
>> On 2011/12/02 19:17:48, John Abd-El-Malek wrote:
>>>
>>> Based on discussion with others, I'm reverting this change. The main
>>> issues is
>>> that this was improperly committed, because there were outstanding
>>> comments
>>
>> from
>>>
>>> other reviewers, and also there were no OWNERS approval.
>>
>>
>>> If you're in a rush to get this in, feel free to IM or email me and I
>>> will
>>> review updated revisions. The two things that should be taken care of are
>>> the
>>> nacl issue and filtering the messages outside of RenderViewImpl. While
>>> these
>>
>> may
>>>
>>> seem minor, a number of people have been spending this whole year
>>> refactoring
>>> this code to simplify RenderView and to separate nacl from content, and
>>> so
>>
>> this
>>>
>>> is taking a step back.
>>
>>
>> First, let me I apologize for creating pressure around landing this CL
>> before
>> M17 branch point. We should not have shortcut the review process because
>> of
>> that.
>>
>> Dmitry, let's address John's existing comments first before we land this
>> CL on
>> TOT. I will make sure that we merge this into the branch even if the
>> branch gets
>> cut before this CL lands.
>>
>> John, you said in your previous comment that you haven't completed your
>> review
>> yet. I would appreciate if you can finish the missing part soon, so Dmitry
>> does
>> not have to make several CR round trips across timezones.
>
>
> Good point, I should have clarified that. The only other issue I had was
> what Brett commented on as well. The IPCs should be dispatched directly
> where they're needed.

My suggestion should be a 10-minute change and will make your patch a
fair bit smaller. I realize that the other code does it the way you do
it now, but your addition of a big block of a bunch of forwarding
functions makes the way we've been doing it seem like it's no longer
scaling. Because of the size and criticality of RenderView, we're
particularly sensitive to additions of code there. You don't have to
fix the existing code, I'll take a TODO to convert those over to the
new filter.

Brett

[brettw, 2011-12-02 21:43:13]

http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_vie...
content/renderer/render_view_impl.cc:282: static const char*
kPredefinedAllowedSocketOrigins[] = {
Can you add a comment here explaining that these are whitelisted extension IDs?
Also, when you move CanUseSocketAPIs, this can be moved to the pepper plugin
delegate impl file.

http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_vie...
content/renderer/render_view_impl.cc:598: bool
RenderViewImpl::CanUseSocketAPIs() {
Can you move this code to the pepper plugin delegate impl? It's not needed in
RenderView, and we should try hard to have as little in RenderView as possible.

[jam, 2011-12-02 21:54:34]

http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_vie...
content/renderer/render_view_impl.cc:282: static const char*
kPredefinedAllowedSocketOrigins[] = {
On 2011/12/02 21:43:13, brettw wrote:
> Can you add a comment here explaining that these are whitelisted extension
IDs?
> Also, when you move CanUseSocketAPIs, this can be moved to the pepper plugin
> delegate impl file.

actually, this should also be in the chrome file that implements
ContentRendererClient, since content module doesn't know about extensions (it's
a chrome feature)

[darin (slow to review), 2011-12-02 22:41:14]

On Fri, Dec 2, 2011 at 1:19 PM, <yzshen@chromium.org> wrote:

>
> http://codereview.chromium.**org/8688002/diff/56007/**
>
content/public/common/content_**switches.h<http://codereview.chromium.org/8688002/diff/56007/content/public/common/content_switches.h>
> File content/public/common/content_**switches.h (right):
>
> http://codereview.chromium.**org/8688002/diff/56007/**
>
content/public/common/content_**switches.h#newcode18<http://codereview.chromium.org/8688002/diff/56007/content/public/common/content_switches.h#newcode18>
> content/public/common/content_**switches.h:18: CONTENT_EXPORT extern const
> char kAllowNaClSocketAPI[];
> On 2011/12/02 20:34:42, darin wrote:
>
>> Is this really the NaCl socket API?  The socket API is part of Pepper.
>>
>  Should
>
>> this be kAllowPepperSocketAPI instead?
>>
>
>  What is NaCl-specific about this CL?
>>
>
> When the socket API is used by non-NaCl pepper plugins (Flapper), we
> don't restrict it to a set of selective origins. So it seems appropriate
> to have NaCl in the name.
>
> But I think it is better not to enforce the origin restriction in render
> view. It will affect not only NaCl, but also other in-process pepper
> plugins.
>

Yes, the origin restrictions do seem out of place in content/.  It seems
like
policy decisions should be made under src/chrome/.

-Darin



>
>
http://codereview.chromium.**org/8688002/<http://codereview.chromium.org/8688...
>

[Dmitry Polukhin, 2011-12-05 11:29:05]

CL to re-land changes is here http://codereview.chromium.org/8804006 please add
you comments to new CL.

http://codereview.chromium.org/8688002/diff/51001/content/public/common/conte...
File content/public/common/content_switches.h (right):

http://codereview.chromium.org/8688002/diff/51001/content/public/common/conte...
content/public/common/content_switches.h:17: CONTENT_EXPORT extern const char
kAllowNaClSocketAPI[];
On 2011/12/01 19:50:31, John Abd-El-Malek wrote:
> this doesn't belong in content, since it's NaCl related which is in chrome.
> 
> so move this out to chrome, and have it be propagated to the renderer by
editing
> ChromeContentBrowserClient::AppendExtraCommandLineSwitches instead of
> RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer

Done.

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:283: "okddffdblfhhnmhodogpojmfkjmhinfp"
On 2011/12/02 03:56:14, brettw wrote:
> What is this?

Added comment.

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:438:
command_line.GetSwitchValueASCII(switches::kAllowNaClSocketAPI);
On 2011/12/01 19:50:31, John Abd-El-Malek wrote:
> here and in the header, please don't mention nacl in content. see
> http://www.chromium.org/developers/content-module
> 
> if you want to have the core code in content, you can ask the embedder whether
> to allow this feature or not through the ContentRendererClient interface.
> Chrome's implementation can then use the nacl flag

Done.

http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:4760:
pepper_delegate_.OnTCPSocketConnectACK(
On 2011/12/02 03:56:14, brettw wrote:
> Is it possible to add a filter from the pepper delegate rather than have all
> these modifications here? This is a lot of modifications to this critical
file.
> 
> You can see the beginning of OnMessageReceived does some dispatch to various
> observers before falling through to the regular message handling.

RenderViewObserver is huge interface but for message filtering we need to
implement much smaller interface IPC::Channel::Listener. If you think that
PepperPluginDelegateImpl should implement RenderViewObserver, please let me and
I'll use observers mechanism instead of explicit message forwarding to
pepper_delegate_.

http://codereview.chromium.org/8688002/diff/56007/content/public/common/conte...
File content/public/common/content_switches.h (right):

http://codereview.chromium.org/8688002/diff/56007/content/public/common/conte...
content/public/common/content_switches.h:18: CONTENT_EXPORT extern const char
kAllowNaClSocketAPI[];
On 2011/12/02 21:19:27, yzshen1 wrote:
> On 2011/12/02 20:34:42, darin wrote:
> > Is this really the NaCl socket API?  The socket API is part of Pepper. 
Should
> > this be kAllowPepperSocketAPI instead?
> > 
> > What is NaCl-specific about this CL?
> 
> When the socket API is used by non-NaCl pepper plugins (Flapper), we don't
> restrict it to a set of selective origins. So it seems appropriate to have
NaCl
> in the name.
> 
> But I think it is better not to enforce the origin restriction in render view.
> It will affect not only NaCl, but also other in-process pepper plugins.
> 

Actually I think that TCP/UDP should be exposed to limited list of trusted
plugins. I think it's dangerous to give these interfaces to all trusted plugins
(some of these interface have Flash, i.e. it should be enforced that only Flash
uses them).

As for enforcement in render, I think it is better to do in browser during
processing TCPSocket_Create/UDPSocket_Create. I tried to do it but failed to do
it easy because Pepper message filter in browser miss context what page sent the
request (or which exactly plugin did it).

http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_vie...
content/renderer/render_view_impl.cc:282: static const char*
kPredefinedAllowedSocketOrigins[] = {
On 2011/12/02 21:54:35, John Abd-El-Malek wrote:
> On 2011/12/02 21:43:13, brettw wrote:
> > Can you add a comment here explaining that these are whitelisted extension
> IDs?
> > Also, when you move CanUseSocketAPIs, this can be moved to the pepper plugin
> > delegate impl file.
> 
> actually, this should also be in the chrome file that implements
> ContentRendererClient, since content module doesn't know about extensions
(it's
> a chrome feature)

Done.

http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_vie...
content/renderer/render_view_impl.cc:598: bool
RenderViewImpl::CanUseSocketAPIs() {
On 2011/12/02 21:43:13, brettw wrote:
> Can you move this code to the pepper plugin delegate impl? It's not needed in
> RenderView, and we should try hard to have as little in RenderView as
possible.

Done.

[yzshen, 2011-12-05 19:26:14]

On Mon, Dec 5, 2011 at 3:29 AM, <dpolukhin@chromium.org> wrote:

> CL to re-land changes is here
http://codereview.chromium.**org/8804006<http://codereview.chromium.org/88040...
add
> you comments to new CL.
>
>
>
> http://codereview.chromium.**org/8688002/diff/51001/**
>
content/public/common/content_**switches.h<http://codereview.chromium.org/8688002/diff/51001/content/public/common/content_switches.h>
> File content/public/common/content_**switches.h (right):
>
> http://codereview.chromium.**org/8688002/diff/51001/**
>
content/public/common/content_**switches.h#newcode17<http://codereview.chromium.org/8688002/diff/51001/content/public/common/content_switches.h#newcode17>
> content/public/common/content_**switches.h:17: CONTENT_EXPORT extern const
> char kAllowNaClSocketAPI[];
>
> On 2011/12/01 19:50:31, John Abd-El-Malek wrote:
>
>> this doesn't belong in content, since it's NaCl related which is in
>>
> chrome.
>
>  so move this out to chrome, and have it be propagated to the renderer
>>
> by editing
>
>> ChromeContentBrowserClient::**AppendExtraCommandLineSwitches instead of
>> RenderProcessHostImpl::**PropagateBrowserCommandLineToR**enderer
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8688002/diff/51001/**
>
content/renderer/render_view_**impl.cc<http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_view_impl.cc>
> File content/renderer/render_view_**impl.cc (right):
>
> http://codereview.chromium.**org/8688002/diff/51001/**
>
content/renderer/render_view_**impl.cc#newcode283<http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_view_impl.cc#newcode283>
> content/renderer/render_view_**impl.cc:283:
> "**okddffdblfhhnmhodogpojmfkjmhin**fp"
>
> On 2011/12/02 03:56:14, brettw wrote:
>
>> What is this?
>>
>
> Added comment.
>
>
> http://codereview.chromium.**org/8688002/diff/51001/**
>
content/renderer/render_view_**impl.cc#newcode438<http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_view_impl.cc#newcode438>
> content/renderer/render_view_**impl.cc:438:
> command_line.**GetSwitchValueASCII(switches::**kAllowNaClSocketAPI);
> On 2011/12/01 19:50:31, John Abd-El-Malek wrote:
>
>> here and in the header, please don't mention nacl in content. see
>>
http://www.chromium.org/**developers/content-module<http://www.chromium.org/d...
>>
>
>  if you want to have the core code in content, you can ask the embedder
>>
> whether
>
>> to allow this feature or not through the ContentRendererClient
>>
> interface.
>
>> Chrome's implementation can then use the nacl flag
>>
>
> Done.
>
> http://codereview.chromium.**org/8688002/diff/51001/**
>
content/renderer/render_view_**impl.cc#newcode4760<http://codereview.chromium.org/8688002/diff/51001/content/renderer/render_view_impl.cc#newcode4760>
> content/renderer/render_view_**impl.cc:4760:
> pepper_delegate_.**OnTCPSocketConnectACK(
> On 2011/12/02 03:56:14, brettw wrote:
>
>> Is it possible to add a filter from the pepper delegate rather than
>>
> have all
>
>> these modifications here? This is a lot of modifications to this
>>
> critical file.
>
>  You can see the beginning of OnMessageReceived does some dispatch to
>>
> various
>
>> observers before falling through to the regular message handling.
>>
>
> RenderViewObserver is huge interface but for message filtering we need
> to implement much smaller interface IPC::Channel::Listener. If you think
> that PepperPluginDelegateImpl should implement RenderViewObserver,
> please let me and I'll use observers mechanism instead of explicit
> message forwarding to pepper_delegate_.
>
>
> http://codereview.chromium.**org/8688002/diff/56007/**
>
content/public/common/content_**switches.h<http://codereview.chromium.org/8688002/diff/56007/content/public/common/content_switches.h>
> File content/public/common/content_**switches.h (right):
>
> http://codereview.chromium.**org/8688002/diff/56007/**
>
content/public/common/content_**switches.h#newcode18<http://codereview.chromium.org/8688002/diff/56007/content/public/common/content_switches.h#newcode18>
> content/public/common/content_**switches.h:18: CONTENT_EXPORT extern const
> char kAllowNaClSocketAPI[];
> On 2011/12/02 21:19:27, yzshen1 wrote:
>
>> On 2011/12/02 20:34:42, darin wrote:
>> > Is this really the NaCl socket API?  The socket API is part of
>>
> Pepper.  Should
>
>> > this be kAllowPepperSocketAPI instead?
>> >
>> > What is NaCl-specific about this CL?
>>
>
>  When the socket API is used by non-NaCl pepper plugins (Flapper), we
>>
> don't
>
>> restrict it to a set of selective origins. So it seems appropriate to
>>
> have NaCl
>
>> in the name.
>>
>
>  But I think it is better not to enforce the origin restriction in
>>
> render view.
>
>> It will affect not only NaCl, but also other in-process pepper
>>
> plugins.
>
>
> Actually I think that TCP/UDP should be exposed to limited list of
> trusted plugins. I think it's dangerous to give these interfaces to all
> trusted plugins (some of these interface have Flash, i.e. it should be
> enforced that only Flash uses them).


I agree that maybe not every trusted plugin should be allowed to use
TCP/UDP.

However, the current implementation of access control seems inconsistent
and confusing:
- in-process: only a selective set of origins are allowed to use TCP/UDP
interface. (NaCl uses this path.)
- out-of-proces: no restriction. (Pepper Flash uses this path.)
What is more, the way of checking page origins doesn't work for trusted
plugins. Consider Pepper Flash: it can be on any page.

I wish this could be addressed.
But I am okay with adding a TODO (feel free to assign to me, if you would
like to) and leave it to a future CL.

As for enforcement in render, I think it is better to do in browser
> during processing TCPSocket_Create/UDPSocket_**Create. I tried to do it
> but failed to do it easy because Pepper message filter in browser miss
> context what page sent the request (or which exactly plugin did it).




>
>
> http://codereview.chromium.**org/8688002/diff/56007/**
>
content/renderer/render_view_**impl.cc<http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_view_impl.cc>
> File content/renderer/render_view_**impl.cc (right):
>
> http://codereview.chromium.**org/8688002/diff/56007/**
>
content/renderer/render_view_**impl.cc#newcode282<http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_view_impl.cc#newcode282>
> content/renderer/render_view_**impl.cc:282: static const char*
> kPredefinedAllowedSocketOrigin**s[] = {
> On 2011/12/02 21:54:35, John Abd-El-Malek wrote:
>
>> On 2011/12/02 21:43:13, brettw wrote:
>> > Can you add a comment here explaining that these are whitelisted
>>
> extension
>
>> IDs?
>> > Also, when you move CanUseSocketAPIs, this can be moved to the
>>
> pepper plugin
>
>> > delegate impl file.
>>
>
>  actually, this should also be in the chrome file that implements
>> ContentRendererClient, since content module doesn't know about
>>
> extensions (it's
>
>> a chrome feature)
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8688002/diff/56007/**
>
content/renderer/render_view_**impl.cc#newcode598<http://codereview.chromium.org/8688002/diff/56007/content/renderer/render_view_impl.cc#newcode598>
> content/renderer/render_view_**impl.cc:598: bool
> RenderViewImpl::**CanUseSocketAPIs() {
> On 2011/12/02 21:43:13, brettw wrote:
>
>> Can you move this code to the pepper plugin delegate impl? It's not
>>
> needed in
>
>> RenderView, and we should try hard to have as little in RenderView as
>>
> possible.
>
> Done.
>
>
http://codereview.chromium.**org/8688002/<http://codereview.chromium.org/8688...
>



-- 
Best regards,
Yuzhu Shen.