PPB_UDPSocket_Private is switched to the new Pepper proxy.

PPB_UDPSocket_Private is switched to the new Pepper proxy.

BUG=164370
TEST=browser_tests:*UDPSocketPrivate*


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

[brettw, 2012-12-05 19:08:46]

-> dmichael, can you take this review instead?

[dmichael (off chromium), 2012-12-12 23:23:38]

https://codereview.chromium.org/11441012/diff/7002/chrome/browser/nacl_host/n...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/11441012/diff/7002/chrome/browser/nacl_host/n...
chrome/browser/nacl_host/nacl_process_host.cc:771: ipc_proxy_channel_.get(), //
process_.get(),  // sender
Oops, this was my mistake. Please just remove the commented-out code (but leave
// sender)

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

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:38: new
PepperMessageFilter(plugin_child_process_type,
I'm not sure I understand why you are changing the process type stuff. My
understanding was that it was just for "sanity checking", and part of the value
is that we can see locally what type it is. E.g., here, it's nice to know at a
glance that its type is "NACL".

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:50:
ppapi::PERMISSION_PRIVATE)) {
This probably won't work for whitelisted NaCl apps, will it? Do you have a
strategy for dealing with those?

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_udp_socket_private_host.h
(right):

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.h:33: :
public ppapi::host::ResourceHost,
would it fit to move this up to the previous line?

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.h:76: const
SocketPermissionRequest& params,
would this fit on the previous line?

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_utils.h (right):

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_utils.h:17: class PepperUtils {
This is a pretty generic class name for something socket specific...  is this
going to be used for more than UDP? Maybe TCP?

If it's only needed in UDP, I don't see a need to separate it out. If it will be
used elsewhere...  how about PepperSocketUtils, or something?

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_utils.h:26: };
If you do need this class, make sure to declare (but don't implement) a private
constructor. Or...  just use a namespace instead of a class?

https://codereview.chromium.org/11441012/diff/7002/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11441012/diff/7002/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1359: uint32 /* socket_id */)
^^^^ Do we still need these? If so...  why?

[ygorshenin1, 2012-12-18 12:07:16]

PTAL

https://codereview.chromium.org/11441012/diff/7002/chrome/browser/nacl_host/n...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/11441012/diff/7002/chrome/browser/nacl_host/n...
chrome/browser/nacl_host/nacl_process_host.cc:771: ipc_proxy_channel_.get(), //
process_.get(),  // sender
On 2012/12/12 23:23:38, dmichael wrote:
> Oops, this was my mistake. Please just remove the commented-out code (but
leave
> // sender)

Done.

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

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:38: new
PepperMessageFilter(plugin_child_process_type,
Because there are different policies for nacl- and out-of-process- plugins.
Please, take a look at:
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h....
The purpose of this CL is to switch UDP interface to the new proxy on the one
hand and to be as more close to the previous implementation as possible on
another.

On 2012/12/12 23:23:38, dmichael wrote:
> I'm not sure I understand why you are changing the process type stuff. My
> understanding was that it was just for "sanity checking", and part of the
value
> is that we can see locally what type it is. E.g., here, it's nice to know at a
> glance that its type is "NACL".

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:50:
ppapi::PERMISSION_PRIVATE)) {
Creation of UDP sockets actually does nothing. All work related to socket
initialization is hidden in Bind(), so from my point of view Create() is a safe
method. As you can see, permission check is performed for Bind(), Send() or
Recv() calls. Also, please take a look on a previous implementation of UDP
sockets:
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h....
Permissions were checked on potentially dangerous operations like Bind() or IO,
not for creation.

On 2012/12/12 23:23:38, dmichael wrote:
> This probably won't work for whitelisted NaCl apps, will it? Do you have a
> strategy for dealing with those?

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_udp_socket_private_host.h
(right):

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.h:33: :
public ppapi::host::ResourceHost,
No, because the total length of these two lines is 83 symbols.

On 2012/12/12 23:23:38, dmichael wrote:
> would it fit to move this up to the previous line?

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.h:76: const
SocketPermissionRequest& params,
On 2012/12/12 23:23:38, dmichael wrote:
> would this fit on the previous line?

Done.

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_utils.h (right):

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_utils.h:17: class PepperUtils {
On 2012/12/12 23:23:38, dmichael wrote:
> This is a pretty generic class name for something socket specific...  is this
> going to be used for more than UDP? Maybe TCP?
> 
> If it's only needed in UDP, I don't see a need to separate it out. If it will
be
> used elsewhere...  how about PepperSocketUtils, or something?

Done.

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_utils.h:26: };
On 2012/12/12 23:23:38, dmichael wrote:
> If you do need this class, make sure to declare (but don't implement) a
private
> constructor. Or...  just use a namespace instead of a class?

Done.

https://codereview.chromium.org/11441012/diff/7002/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11441012/diff/7002/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1359: uint32 /* socket_id */)
On 2012/12/12 23:23:38, dmichael wrote:
> ^^^^ Do we still need these? If so...  why?

I kept old UDP IPC messages for support of in-process plugins. Deleted after
discussion with Brett (https://codereview.chromium.org/11411357/).

[dmichael (off chromium), 2012-12-22 00:35:08]

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

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:38: new
PepperMessageFilter(plugin_child_process_type,
On 2012/12/18 12:07:16, ygorshenin1 wrote:
> Because there are different policies for nacl- and out-of-process- plugins.
> Please, take a look at:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h....
> The purpose of this CL is to switch UDP interface to the new proxy on the one
> hand and to be as more close to the previous implementation as possible on
> another.
> 
But don't we only come in to this code path if the process type is NACL? I
understand why the process type code exists, but I'm not sure why you're
changing the type and removing the specific value from call-sites.
See:
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h...
It looks like we only get here for NaCl, so it's not clear to me why you needed
to add a new parameter to CreateExternalPluginProcess. And that constructor
doesn't work if the value is not PROCESS_TYPE_NACL_LOADER. I'm fine with using
content::ProcessType, though.

> On 2012/12/12 23:23:38, dmichael wrote:
> > I'm not sure I understand why you are changing the process type stuff. My
> > understanding was that it was just for "sanity checking", and part of the
> value
> > is that we can see locally what type it is. E.g., here, it's nice to know at
a
> > glance that its type is "NACL".
>

[ygorshenin1, 2012-12-24 14:41:42]

PTAL.

I merged PPB_UDPSocket_Private to UDPSocketPrivateResource and
PepperUDPSocketPrivateShared to PepperUDPSocketPrivateResource (to be consistent
with https://codereview.chromium.org/11411357/).

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

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:38: new
PepperMessageFilter(plugin_child_process_type,
Because I don't understand why do we need PepperMessageFilter::ProcessType while
there are already exists ProcessType enum. And why not to check
plugin_child_process_type for sanity one more time in the corresponding
PepperMessageFilter constructor?

On 2012/12/22 00:35:08, dmichael wrote:
> On 2012/12/18 12:07:16, ygorshenin1 wrote:
> > Because there are different policies for nacl- and out-of-process- plugins.
> > Please, take a look at:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h....
> > The purpose of this CL is to switch UDP interface to the new proxy on the
one
> > hand and to be as more close to the previous implementation as possible on
> > another.
> > 
> But don't we only come in to this code path if the process type is NACL? I
> understand why the process type code exists, but I'm not sure why you're
> changing the type and removing the specific value from call-sites.
> See:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h...
> It looks like we only get here for NaCl, so it's not clear to me why you
needed
> to add a new parameter to CreateExternalPluginProcess. And that constructor
> doesn't work if the value is not PROCESS_TYPE_NACL_LOADER. I'm fine with using
> content::ProcessType, though.
> 
> > On 2012/12/12 23:23:38, dmichael wrote:
> > > I'm not sure I understand why you are changing the process type stuff. My
> > > understanding was that it was just for "sanity checking", and part of the
> > value
> > > is that we can see locally what type it is. E.g., here, it's nice to know
at
> a
> > > glance that its type is "NACL".
> > 
>

[ygorshenin1, 2013-01-09 08:16:51]

friendly ping

[dmichael (off chromium), 2013-01-10 05:57:16]

https://codereview.chromium.org/11441012/diff/7002/chrome/browser/nacl_host/n...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/11441012/diff/7002/chrome/browser/nacl_host/n...
chrome/browser/nacl_host/nacl_process_host.cc:775: process_->GetData().type,
I don't think you need this param. If you want to make sure it's the right type,
you can DCHECK it.

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

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:38: new
PepperMessageFilter(plugin_child_process_type,
On 2012/12/24 14:41:43, ygorshenin1 wrote:
> Because I don't understand why do we need PepperMessageFilter::ProcessType
while
> there are already exists ProcessType enum.
I'm fine with using content::ProcessType. Makes sense.

> And why not to check
> plugin_child_process_type for sanity one more time in the corresponding
> PepperMessageFilter constructor?
We already do check it there, I think. What I don't understand is why you're
changing the signature of CreateExternalPluginProcess to add a ProcessType
parameter, when we always know that the type is PROCESS_TYPE_NACL_LOADER. It's a
minor point, but I think I like it a bit better without the parameter, and using
PROCESS_TYPE_NACL_LOADER explicitly.


> 
> On 2012/12/22 00:35:08, dmichael wrote:
> > On 2012/12/18 12:07:16, ygorshenin1 wrote:
> > > Because there are different policies for nacl- and out-of-process-
plugins.
> > > Please, take a look at:
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h....
> > > The purpose of this CL is to switch UDP interface to the new proxy on the
> one
> > > hand and to be as more close to the previous implementation as possible on
> > > another.
> > > 
> > But don't we only come in to this code path if the process type is NACL? I
> > understand why the process type code exists, but I'm not sure why you're
> > changing the type and removing the specific value from call-sites.
> > See:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h...
> > It looks like we only get here for NaCl, so it's not clear to me why you
> needed
> > to add a new parameter to CreateExternalPluginProcess. And that constructor
> > doesn't work if the value is not PROCESS_TYPE_NACL_LOADER. I'm fine with
using
> > content::ProcessType, though.
> > 
> > > On 2012/12/12 23:23:38, dmichael wrote:
> > > > I'm not sure I understand why you are changing the process type stuff.
My
> > > > understanding was that it was just for "sanity checking", and part of
the
> > > value
> > > > is that we can see locally what type it is. E.g., here, it's nice to
know
> at
> > a
> > > > glance that its type is "NACL".
> > > 
> > 
>

https://codereview.chromium.org/11441012/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc
(right):

https://codereview.chromium.org/11441012/diff/39001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:162:
data,
This is going to implicitly do a copy, which you then copy again in DoSendTo in
to the net::IOBuffer. Is it possible to just use net::IOBuffer or
IOBufferWithSize as your param?

https://codereview.chromium.org/11441012/diff/39001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:318: void
PepperUDPSocketPrivateHost::SendSendToReply(bool succeeded,
Why have the succeeded parameter? You only call this on success. Same for
SendBindReply and SendRecvFromReply.

https://codereview.chromium.org/11441012/diff/39001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:342:
SendBindReply(false, addr);
Shouldn't you release the context in the error cases as well? Otherwise, it
seems like you'll never break out of returning PP_ERROR_INPROGRESS

[ygorshenin1, 2013-01-10 11:36:11]

PTAL

https://codereview.chromium.org/11441012/diff/7002/chrome/browser/nacl_host/n...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/11441012/diff/7002/chrome/browser/nacl_host/n...
chrome/browser/nacl_host/nacl_process_host.cc:775: process_->GetData().type,
On 2013/01/10 05:57:16, dmichael wrote:
> I don't think you need this param. If you want to make sure it's the right
type,
> you can DCHECK it.

Done.

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

https://codereview.chromium.org/11441012/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:38: new
PepperMessageFilter(plugin_child_process_type,
OK, agree. Done.

On 2013/01/10 05:57:16, dmichael wrote:
> On 2012/12/24 14:41:43, ygorshenin1 wrote:
> > Because I don't understand why do we need PepperMessageFilter::ProcessType
> while
> > there are already exists ProcessType enum.
> I'm fine with using content::ProcessType. Makes sense.
> 
> > And why not to check
> > plugin_child_process_type for sanity one more time in the corresponding
> > PepperMessageFilter constructor?
> We already do check it there, I think. What I don't understand is why you're
> changing the signature of CreateExternalPluginProcess to add a ProcessType
> parameter, when we always know that the type is PROCESS_TYPE_NACL_LOADER. It's
a
> minor point, but I think I like it a bit better without the parameter, and
using
> PROCESS_TYPE_NACL_LOADER explicitly.
> 
> 
> > 
> > On 2012/12/22 00:35:08, dmichael wrote:
> > > On 2012/12/18 12:07:16, ygorshenin1 wrote:
> > > > Because there are different policies for nacl- and out-of-process-
> plugins.
> > > > Please, take a look at:
> > > >
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h....
> > > > The purpose of this CL is to switch UDP interface to the new proxy on
the
> > one
> > > > hand and to be as more close to the previous implementation as possible
on
> > > > another.
> > > > 
> > > But don't we only come in to this code path if the process type is NACL? I
> > > understand why the process type code exists, but I'm not sure why you're
> > > changing the type and removing the specific value from call-sites.
> > > See:
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/renderer_h...
> > > It looks like we only get here for NaCl, so it's not clear to me why you
> > needed
> > > to add a new parameter to CreateExternalPluginProcess. And that
constructor
> > > doesn't work if the value is not PROCESS_TYPE_NACL_LOADER. I'm fine with
> using
> > > content::ProcessType, though.
> > > 
> > > > On 2012/12/12 23:23:38, dmichael wrote:
> > > > > I'm not sure I understand why you are changing the process type stuff.
> My
> > > > > understanding was that it was just for "sanity checking", and part of
> the
> > > > value
> > > > > is that we can see locally what type it is. E.g., here, it's nice to
> know
> > at
> > > a
> > > > > glance that its type is "NACL".
> > > > 
> > > 
> > 
>

https://codereview.chromium.org/11441012/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc
(right):

https://codereview.chromium.org/11441012/diff/39001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:162:
data,
Sure, thanks!
Done.

On 2013/01/10 05:57:16, dmichael wrote:
> This is going to implicitly do a copy, which you then copy again in DoSendTo
in
> to the net::IOBuffer. Is it possible to just use net::IOBuffer or
> IOBufferWithSize as your param?

https://codereview.chromium.org/11441012/diff/39001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:318: void
PepperUDPSocketPrivateHost::SendSendToReply(bool succeeded,
Not exactly. These methods are also called from Send*Error() methods and we need
|succeeded| parameter (see reply to your comment below).

On 2013/01/10 05:57:16, dmichael wrote:
> Why have the succeeded parameter? You only call this on success. Same for
> SendBindReply and SendRecvFromReply.

https://codereview.chromium.org/11441012/diff/39001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:342:
SendBindReply(false, addr);
{send_to|recv_from|bind}_context_ are released in corresponding Send*Reply()
methods. So they're also released in Send*Error() methods, because they're just
wrappers for Send*Reply(). This allows to avoid code duplication.

On 2013/01/10 05:57:16, dmichael wrote:
> Shouldn't you release the context in the error cases as well? Otherwise, it
> seems like you'll never break out of returning PP_ERROR_INPROGRESS

[dmichael (off chromium), 2013-01-10 17:30:46]

A few couple of nits, and 1 thing I care about
(PostOnUIThreadWithRenderViewHostAndReply probably doesn't belong in
content/public). o/w lgtm

Adding some OWNERS...

+brettw for OWNERS review of:
content/browser/ppapi_plugin_process_host.cc
content/browser/renderer_host/render_process_host_impl.cc
content/content_browser.gypi
webkit/glue/webkit_glue.gypi

+raymes for OWNERS review of:
content/browser/renderer_host/pepper/*
content/renderer/pepper/*

+jschuh for security/OWNERS review of:
ppapi/proxy/ppapi_messages.h

https://chromiumcodereview.appspot.com/11441012/diff/39001/content/browser/re...
File content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc
(right):

https://chromiumcodereview.appspot.com/11441012/diff/39001/content/browser/re...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:318: void
PepperUDPSocketPrivateHost::SendSendToReply(bool succeeded,
On 2013/01/10 11:36:12, ygorshenin1 wrote:
> Not exactly. These methods are also called from Send*Error() methods and we
need
> |succeeded| parameter (see reply to your comment below).
> 
> On 2013/01/10 05:57:16, dmichael wrote:
> > Why have the succeeded parameter? You only call this on success. Same for
> > SendBindReply and SendRecvFromReply.
> 
I see, thanks for explaining.

https://chromiumcodereview.appspot.com/11441012/diff/39001/content/public/bro...
File content/public/browser/browser_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/11441012/diff/39001/content/public/bro...
content/public/browser/browser_ppapi_host.h:127: // Backend for
PostOnUIThreadWithRenderViewAndReply. This converts the IDs
nit: PostOnUIThreadWithRenderView+Host+AndReply

https://chromiumcodereview.appspot.com/11441012/diff/42004/content/browser/re...
File content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc
(right):

https://chromiumcodereview.appspot.com/11441012/diff/42004/content/browser/re...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:160:
NOTREACHED();
A comment that this was checked in the plugin side might be helpful.

https://chromiumcodereview.appspot.com/11441012/diff/42004/content/public/bro...
File content/public/browser/browser_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/11441012/diff/42004/content/public/bro...
content/public/browser/browser_ppapi_host.h:104: bool
PostOnUIThreadWithRenderViewHostAndReply(
This is cool, but I think it probably doesn't belong here. For now, you could
just stick it in an unnamed namespace in pepper_udp_socket_private_host.cc, and
we can move it somewhere later if/when it gets used again. But in any case, I
think content/public should be kept minimal; this is kind of an internal
implementation detail.

[ygorshenin1, 2013-01-11 11:42:45]

Thanks! PTAL

https://codereview.chromium.org/11441012/diff/39001/content/public/browser/br...
File content/public/browser/browser_ppapi_host.h (right):

https://codereview.chromium.org/11441012/diff/39001/content/public/browser/br...
content/public/browser/browser_ppapi_host.h:127: // Backend for
PostOnUIThreadWithRenderViewAndReply. This converts the IDs
On 2013/01/10 17:30:47, dmichael wrote:
> nit: PostOnUIThreadWithRenderView+Host+AndReply

Done.

https://codereview.chromium.org/11441012/diff/42004/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc
(right):

https://codereview.chromium.org/11441012/diff/42004/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_udp_socket_private_host.cc:160:
NOTREACHED();
On 2013/01/10 17:30:47, dmichael wrote:
> A comment that this was checked in the plugin side might be helpful.

Done.

https://codereview.chromium.org/11441012/diff/42004/content/public/browser/br...
File content/public/browser/browser_ppapi_host.h (right):

https://codereview.chromium.org/11441012/diff/42004/content/public/browser/br...
content/public/browser/browser_ppapi_host.h:104: bool
PostOnUIThreadWithRenderViewHostAndReply(
OK, I'll move these methods to pepper_socket_utils.h. They're certainly will be
used by TCP and TCP server sockets.

On 2013/01/10 17:30:47, dmichael wrote:
> This is cool, but I think it probably doesn't belong here. For now, you could
> just stick it in an unnamed namespace in pepper_udp_socket_private_host.cc,
and
> we can move it somewhere later if/when it gets used again. But in any case, I
> think content/public should be kept minimal; this is kind of an internal
> implementation detail.

[dmichael (off chromium), 2013-01-11 17:49:46]

lgtm; but you'll need some OWNERS approvals (OWNERS, see my previous message for
who should look at what. Thanks!)

[dmichael (off chromium), 2013-01-11 17:50:26]

+jschuh for ppapi/proxy/ppapi_messages.h
(I forgot to actually add him to reviewers before)

[raymes, 2013-01-13 20:47:57]

https://chromiumcodereview.appspot.com/11441012/diff/55001/content/browser/re...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://chromiumcodereview.appspot.com/11441012/diff/55001/content/browser/re...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:53:
if (host_->GetPpapiHost()->permissions().HasPermission(
Please use GetPermissions() as in the cases below.

https://chromiumcodereview.appspot.com/11441012/diff/55001/content/browser/re...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:55:
message.type() == PpapiHostMsg_UDPSocketPrivate_Create::ID) {
This part is unusual and subtle. Can you explain why this is needed? Is there an
alternative to having this special case? We should at least add a comment here
explaining where the permissions check is done.

https://chromiumcodereview.appspot.com/11441012/diff/55001/content/renderer/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://chromiumcodereview.appspot.com/11441012/diff/55001/content/renderer/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:98: return
kInvalidResource;
nit: I think it's ok just to return 0 here since we already do that in this file
(e.g. see CreateBrowserFont()). I realize we're not consistent across files with
this.

[ygorshenin1, 2013-01-14 08:47:43]

PTAL

https://codereview.chromium.org/11441012/diff/55001/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11441012/diff/55001/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:53:
if (host_->GetPpapiHost()->permissions().HasPermission(
On 2013/01/13 20:47:58, raymes wrote:
> Please use GetPermissions() as in the cases below.

Done.

https://codereview.chromium.org/11441012/diff/55001/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:55:
message.type() == PpapiHostMsg_UDPSocketPrivate_Create::ID) {
On 2013/01/13 20:47:58, raymes wrote:
> This part is unusual and subtle. Can you explain why this is needed? Is there
an
> alternative to having this special case? We should at least add a comment here
> explaining where the permissions check is done.

Done.

https://codereview.chromium.org/11441012/diff/55001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/11441012/diff/55001/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:98: return
kInvalidResource;
On 2013/01/13 20:47:58, raymes wrote:
> nit: I think it's ok just to return 0 here since we already do that in this
file
> (e.g. see CreateBrowserFont()). I realize we're not consistent across files
with
> this.

Done.

[ygorshenin1, 2013-01-15 17:21:29]

friendly ping

[raymes, 2013-01-15 20:58:11]

OWNERS lgtm for the noted files.

It would be nice if doing the permissions checks were simpler but I'm not sure
if that's possible. 

brettw previously looked at adding PostOnUIThreadWithRenderViewHostAndReply as a
generic utility. If there are more uses of this in the future we could consider
moving it back as such. brettw feel free to chime in.

https://codereview.chromium.org/11441012/diff/75003/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11441012/diff/75003/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:74:
message.type() == PpapiHostMsg_UDPSocketPrivate_Create::ID) {
I still think this could be a little clearer. Can we pull UDP out into a
separate section. So we would have:

// Private interfaces.
...

// UDPSocketPrivate interface.
// Permissions for UDPSocketPrivate interface will be checked at the...
if (message.type() == PpapiHostMsg_UDPSocketPrivate_Create::ID) {
  return ...
}

[ygorshenin1, 2013-01-16 08:13:50]

Thanks!

Brett, PTAL.

https://codereview.chromium.org/11441012/diff/75003/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11441012/diff/75003/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:74:
message.type() == PpapiHostMsg_UDPSocketPrivate_Create::ID) {
On 2013/01/15 20:58:11, raymes wrote:
> I still think this could be a little clearer. Can we pull UDP out into a
> separate section. So we would have:
> 
> // Private interfaces.
> ...
> 
> // UDPSocketPrivate interface.
> // Permissions for UDPSocketPrivate interface will be checked at the...
> if (message.type() == PpapiHostMsg_UDPSocketPrivate_Create::ID) {
>   return ...
> }

Done.

[ygorshenin1, 2013-01-18 05:04:10]

friendly ping

[jschuh, 2013-01-18 05:52:04]

rubberstamp ipc lgtm, since this doesn't change behavior

[ygorshenin1, 2013-01-18 07:42:46]

Brett, I need your OWNER review of:
content/browser/ppapi_plugin_process_host.cc
content/browser/renderer_host/render_process_host_impl.cc
content/content_browser.gypi
webkit/glue/webkit_glue.gypi
chrome/browser/nacl_host/nacl_process_host.cc

[brettw, 2013-01-18 20:58:50]

owners LGTM for aforementioned stuff. Thanks for doing this!

[commit-bot: I haz the power, 2013-01-21 10:37:51]

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

[ygorshenin1, 2013-01-21 10:38:09]

Many thanks!

[commit-bot: I haz the power, 2013-01-21 15:21:58]

Change committed as 177915