Add a content API to connect a Native Client module to an out-of-process PPAPI proxy.

Add a content API to connect a Native Client module to an out-of-process PPAPI proxy.
This change adds a 'CreateNaClPluginModule' method to the content/renderer API, and
to PepperPluginDelegateImpl, which implements it.
BUG=116317
TEST=none

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

[bbudge, 2012-10-16 01:46:41]

This makes apps that use WebSockets work.

Things in particular to look at:
1) I am passing a raw PluginInstance pointer around. It seems more consistent
with other usage but I'm not sure it's safe. Could the instance be deleted out
from under us?
2) Will PepperHungPluginFilter work with a fake path and process id? We could
get the process id if necessary by changing the private NaCl interface.

[bbudge, 2012-10-16 04:03:02]

Added missing files.

[jam, 2012-10-16 06:21:37]

nacl is a chrome feature and not in content. can you do this another way?

[bbudge, 2012-10-16 11:52:50]

On 2012/10/16 06:21:37, John Abd-El-Malek wrote:
> nacl is a chrome feature and not in content. can you do this another way?

The only other way is to copy the Pepper code inside content that is needed to
set up an out-of-process proxy. In this case, we need to create a
RenderPpapiHostImpl in order to support the new resource architecture. I can
rename it so it is more generic, similarly to how we did it with
content/public/browser/pepper_helper.h.

[bbudge, 2012-10-16 11:54:26]

On 2012/10/16 11:52:50, bbudge1 wrote:
> On 2012/10/16 06:21:37, John Abd-El-Malek wrote:
> > nacl is a chrome feature and not in content. can you do this another way?
> 
> The only other way is to copy the Pepper code inside content that is needed to
> set up an out-of-process proxy. In this case, we need to create a
> RenderPpapiHostImpl in order to support the new resource architecture. I can
> rename it so it is more generic, similarly to how we did it with
> content/public/browser/pepper_helper.h.

By 'rename it' I'm referring to the new API function, CreateNaClPluginModule.

[brettw, 2012-10-16 18:23:52]

Let's call this stuff an "External" plugin module instead. I think this stuff is
not nacl-specific, but rather just has to do with an embedder wanting to run
some "other type" of Pepper plugin.

I'm trying to think of a better way to integrate this than just having a random
helper function in a file. I notice you don't actually use the return value of
the webkit::ppapi::PluginModule.

What about this: in RendererPpapiHost we could have a static function
"CreateExternalPlugin" that's passed the stuff you need and returns a
RendererPpapiHost*. This makes it more of a factory function for the
RendererPpapiHost, which is really the public interface in content for a pepper
plugin (you can get the RenderView, WebPluginContainer, etc. from there, and
conceivably we could add a getter for the PluginModule if we really needed it.

http://codereview.chromium.org/11140046/diff/1009/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/11140046/diff/1009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:418:
PepperPluginDelegateImpl::CreateNaClPluginModule(
Let's call this "CreateExternalPluginModule" and pass the nacl stuff we need
(like the file path) as parameters.

[bbudge, 2012-10-16 21:03:38]

I moved the API change to RendererPpapiHost as you suggested. I still call
PepperPluginDelegateImpl to do the actual work. I needed to add a PluginInstance
parameter to get to the PluginDelegate.

http://codereview.chromium.org/11140046/diff/1009/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/11140046/diff/1009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:418:
PepperPluginDelegateImpl::CreateNaClPluginModule(
On 2012/10/16 18:23:53, brettw wrote:
> Let's call this "CreateExternalPluginModule" and pass the nacl stuff we need
> (like the file path) as parameters.

Done.

[jam, 2012-10-16 21:14:43]

much better, thanks. lgtm for public

https://codereview.chromium.org/11140046/diff/17002/content/public/renderer/r...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/11140046/diff/17002/content/public/renderer/r...
content/public/renderer/renderer_ppapi_host.h:71: static RendererPpapiHost*
CreateExternalPluginModule(
nit: static methods go first in the api

https://codereview.chromium.org/11140046/diff/17002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://codereview.chromium.org/11140046/diff/17002/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:442:
content::RendererPpapiHostImpl::CreateOnModuleForOutOfProcess(
nit: no need for content::

[brettw, 2012-10-16 23:37:32]

http://codereview.chromium.org/11140046/diff/17002/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

http://codereview.chromium.org/11140046/diff/17002/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:70: // otherwise NULL.
Can the comment here make it clear what the file path is used for? It's weird
because you don't actually pass a valid file path.

http://codereview.chromium.org/11140046/diff/17002/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/11140046/diff/17002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:423:
scoped_refptr<PepperHungPluginFilter> hung_filter(
This code is just copied and pasted from the previous function. If you moved the
PluginModule creation above, I think you could just factor this into a helper
function to start a proxy on a given plugin module.

[bbudge, 2012-10-17 00:18:44]

Ready for another round.

http://codereview.chromium.org/11140046/diff/17002/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

http://codereview.chromium.org/11140046/diff/17002/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:70: // otherwise NULL.
On 2012/10/16 23:37:32, brettw wrote:
> Can the comment here make it clear what the file path is used for? It's weird
> because you don't actually pass a valid file path.

Done. This is only used to construct a PepperHungPluginFilter internally. I left
it a FilePath to make it a little easier. It looks like it's only used to report
hangs with some kind of identifier for the plugin. I'll hook this up later.

http://codereview.chromium.org/11140046/diff/17002/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:71: static RendererPpapiHost*
CreateExternalPluginModule(
On 2012/10/16 21:14:43, John Abd-El-Malek wrote:
> nit: static methods go first in the api

Done.

http://codereview.chromium.org/11140046/diff/17002/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/11140046/diff/17002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:423:
scoped_refptr<PepperHungPluginFilter> hung_filter(
On 2012/10/16 23:37:32, brettw wrote:
> This code is just copied and pasted from the previous function. If you moved
the
> PluginModule creation above, I think you could just factor this into a helper
> function to start a proxy on a given plugin module.

Done. Good eye.

http://codereview.chromium.org/11140046/diff/17002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:442:
content::RendererPpapiHostImpl::CreateOnModuleForOutOfProcess(
On 2012/10/16 21:14:43, John Abd-El-Malek wrote:
> nit: no need for content::

Done.

[brettw, 2012-10-17 05:01:13]

lgtm

[commit-bot: I haz the power, 2012-10-17 10:48:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/11140046/34002

[commit-bot: I haz the power, 2012-10-17 11:45:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[bbudge, 2012-10-17 11:54:26]

Fix Windows linkage error by adding missing CONTENT_EXPORT.

[commit-bot: I haz the power, 2012-10-17 11:55:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/11140046/19016

[commit-bot: I haz the power, 2012-10-17 12:36:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/11140046/35001

[commit-bot: I haz the power, 2012-10-17 14:34:59]

Change committed as 162407