Hook up content/renderer to the PPAPI host.

Hook up content/renderer to the PPAPI host.

This hooks up the machinery in content/renderer to the PPAPI host infrastructure. Nothing uses this yet.

This will allow us to implement resources in content/renderer/pepper via the new resource creation impl. The new in-process resource creation will allow new-style IPC-only resources to be created in-process

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

[brettw, 2012-06-30 05:42:02]

This is actually pretty simple. It hooks up the host stuff from the previous
patch but still doesn't use it for anything.

[dmichael (off chromium), 2012-07-02 19:34:57]

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/con...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/con...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:27: return
scoped_ptr<ResourceHost>();
Maybe add a TODO?

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/con...
File content/renderer/pepper/content_renderer_pepper_host_factory.h (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/con...
content/renderer/pepper/content_renderer_pepper_host_factory.h:17:
ContentRendererPepperHostFactory(RenderViewImpl* render_view);
nit: explicit

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:15: // Note that
the code in the creation functions in this file should generall be
generall->generally

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:16: // the same
as that in pepper/proxy/resource_creation_proxy.cc. See
pepper->ppapi?

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:46: return
host_.OnMessageReceived(*msg);
Are there any cases where the proxy code assumes asynchronous messages?
Something to watch out for. As-is, it seems like if the proxy tries to do an
async request/reply currently, this will cause synchronous behavior and a nested
upcall while the plugin's Send is still on the stack. You might have to check if
the message is asynchronous, and if so post a task.

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69:
IPC_BEGIN_MESSAGE_MAP(HostToPluginRouter, *msg)
Having this MESSAGE_MAP stuff in a Send is weird enough it might warrant a
comment here. I guess it's because you can't depend on whatever Listener class
you're going to have in ppapi/proxy that presumably will do pretty much what
OnMsgResourceReply below does?

Might you be able to have something in ppapi/shared_impl that does the
dispatching to call OnReplyReceived on the right Resource, so you don't have to
have that in 2 places? (Doesn't have to be this CL).

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:82: if
(!resource) {
Are we sure this can't happen? I guess right now, with the in-process case being
always synchronous, it should be safe. But I don't think that will hold if you
make it behave asynchronously (as you might need to).

Or are you doing something to bump the ref on a resource while it has a response
pending from the host?

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_in_process_resource_creation.h (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.h:32: // the
temporary "two different classes."
nit: classes->styles?

http://codereview.chromium.org/10578043/diff/6009/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/10578043/diff/6009/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource_creation_impl.cc:1: 
nit: got an extra line

[brettw, 2012-07-03 16:52:53]

new snap up

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/con...
File content/renderer/pepper/content_renderer_pepper_host_factory.h (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/con...
content/renderer/pepper/content_renderer_pepper_host_factory.h:17:
ContentRendererPepperHostFactory(RenderViewImpl* render_view);
You always get me on explicit!

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:46: return
host_.OnMessageReceived(*msg);
You're right, I should probably do this asynchronously. I did it in both
directions.

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69:
IPC_BEGIN_MESSAGE_MAP(HostToPluginRouter, *msg)
The while point of shared_impl is to be shared between the impl and the proxy,
and one of the main goals here is to make that unnecessary. So I think
shared_impl should go away. I think a lookup and a dispatch isn't so bad to
duplicate.

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:82: if
(!resource) {
Good point, I removed the assert & added a comment.

[dmichael (off chromium), 2012-07-03 17:08:52]

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/con...
File content/renderer/pepper/content_renderer_pepper_host_factory.h (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/con...
content/renderer/pepper/content_renderer_pepper_host_factory.h:17:
ContentRendererPepperHostFactory(RenderViewImpl* render_view);
On 2012/07/03 16:52:54, brettw wrote:
> You always get me on explicit!
I was starting to wonder if you leave them out just to make me feel useful ;-)

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:46: return
host_.OnMessageReceived(*msg);
On 2012/07/03 16:52:54, brettw wrote:
> You're right, I should probably do this asynchronously. I did it in both
> directions.
But now I think it won't work with synchronous messages. I think you have to
check which kind it is, right? I'm guessing you can just call directly for sync
messages...  if not, we can fix it later.

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:69:
IPC_BEGIN_MESSAGE_MAP(HostToPluginRouter, *msg)
On 2012/07/03 16:52:54, brettw wrote:
> The while point of shared_impl is to be shared between the impl and the proxy,
> and one of the main goals here is to make that unnecessary. So I think
> shared_impl should go away. I think a lookup and a dispatch isn't so bad to
> duplicate.
Yeah, it's pretty short now. I was afraid of whether it might get more
complicated, but it's certainly fine for now.

[brettw, 2012-07-03 17:22:53]

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:46: return
host_.OnMessageReceived(*msg);
There are no synchronous resource messages (everything is nested in one of the
Call and Reply messages which are async). When we get to filtered input events,
we'll have to add a different type of message and I'm going to worry about that
later.

[dmichael (off chromium), 2012-07-03 17:30:16]

lgtm

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

http://codereview.chromium.org/10578043/diff/6009/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:46: return
host_.OnMessageReceived(*msg);
On 2012/07/03 17:22:53, brettw wrote:
> There are no synchronous resource messages (everything is nested in one of the
> Call and Reply messages which are async). When we get to filtered input
events,
> we'll have to add a different type of message and I'm going to worry about
that
> later.
I'm not sure how it's going to support stuff like PPB_Buffer_Dev (at least in
its current form) with purely asynchronous messages...  e.g.:
PP_Resource buffer = buffer_if->Create(instance, 1024);
if (buffer)
  void* bytes = buffer_if->Map(buffer);

...but I'm willing to let you figure that stuff out later.

[commit-bot: I haz the power, 2012-07-03 17:55:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/10578043/14001

[commit-bot: I haz the power, 2012-07-03 18:44:18]

Try job failure for 10578043-14001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...