Refactoring of new IPC-only pepper implementation

This adds new Content interfaces wrapping the Pepper host in both the browser and the renderer so Chrome-implemented resources can access them. There aren't any Chrome resources yet so this is currently unused, but this will be a good place to put stuff. This enabled me to delete the old pepper_instance_state_accessor and move its methods onto the RendererPpapiHost.

This makes the PluginModule own the new RendererPpapiHostImpl object. Previously this was a bit confused. It used to be owned by the dispatcher wrapper for the out-of-process case, and there was one per-instance (whoops!) for the in-process case. Now these cases are uniform. There is a new interface on PluginModule to allow owning this pointer.

The in-process fake IPC channel should be per-plugin instead of per-instance, so I moved the routing from PepperInProcessResourceCreation to a new object that's owned by the host impl. The PepperInProcessResourceCreation just references this.

[brettw, 2012-07-24 19:28:20]

This is on top of this patch http://codereview.chromium.org/10803050/

[raymes, 2012-07-25 02:21:25]

On 2012/07/24 19:28:20, brettw wrote:
> This is on top of this patch http://codereview.chromium.org/10803050/

For reference, these 2 CLs were closely related when I was reviewing this:
https://chromiumcodereview.appspot.com/10696132
https://chromiumcodereview.appspot.com/10572040

[raymes, 2012-07-25 16:53:22]

Some high level comments.

https://chromiumcodereview.appspot.com/10815073/diff/1/chrome/renderer/pepper...
File chrome/renderer/pepper/pepper_flash_renderer_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/1/chrome/renderer/pepper...
chrome/renderer/pepper/pepper_flash_renderer_message_filter.cc:13:
PepperFlashRendererMessageFilter::PepperFlashRendererMessageFilter(
Thought: what if everything in the new model was implemented as a resource? You
would be able to remove this stuff. What would be the drawback?

https://chromiumcodereview.appspot.com/10815073/diff/1/content/browser/ppapi_...
File content/browser/ppapi_plugin_process_host.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/1/content/browser/ppapi_...
content/browser/ppapi_plugin_process_host.cc:130: host_impl_.reset(new
content::BrowserPpapiHostImpl(this, permissions));
How do messages get routed to the BrowserPppapiHostImpl?

https://chromiumcodereview.appspot.com/10815073/diff/1/content/browser/render...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h
(right):

https://chromiumcodereview.appspot.com/10815073/diff/1/content/browser/render...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:15:
class ContentBrowserPepperHostFactory : public ppapi::host::HostFactory {
I realize this is not directly related to this CL but I think it would be nicer
if HostFactory and friends were named ResourceHostFactory (since it's creating
ResourceHosts and not PpapiHosts).

Even clearer (though longer) might be:
we have PluginResource
PluginResourceHost
PluginResourceHostFactory

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
File content/renderer/pepper/pepper_in_process_resource_creation.h (right):

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
content/renderer/pepper/pepper_in_process_resource_creation.h:27: // This class
allows us to run new-style IPC-only resources in-process. We use the
long line

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
content/renderer/pepper/renderer_ppapi_host_impl.cc:110: return
!!GetAndValidateInstance(instance);
Looks like you forgot to implement this properly?

What other methods would you expect to go in here?

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
File content/renderer/pepper/renderer_ppapi_host_impl.h (right):

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
content/renderer/pepper/renderer_ppapi_host_impl.h:50: // Factory functiosn to
create in process or out-of-process host impls. The
"functions"

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
content/renderer/pepper/renderer_ppapi_host_impl.h:55: // idoes not pass
ownership, it's just for the information of the caller.
"does"

[raymes, 2012-07-29 15:53:24]

Will still probably take one more pass over this.

http://codereview.chromium.org/10815073/diff/6002/content/public/renderer/ren...
File content/public/renderer/renderer_ppapi_host.h (right):

http://codereview.chromium.org/10815073/diff/6002/content/public/renderer/ren...
content/public/renderer/renderer_ppapi_host.h:21: // PpapiHost in the browser
process.
browser->renderer

http://codereview.chromium.org/10815073/diff/6002/ppapi/examples/2d/paint_man...
File ppapi/examples/2d/paint_manager_example.cc (right):

http://codereview.chromium.org/10815073/diff/6002/ppapi/examples/2d/paint_man...
ppapi/examples/2d/paint_manager_example.cc:63:
chooser_.Show(callback_factory_.NewCallbackWithOutput(&MyInstance::GotFile));
Were you going to hook this up? Seems like it's not adding anything to the
example currently.

http://codereview.chromium.org/10815073/diff/6002/ppapi/proxy/dispatch_reply_...
File ppapi/proxy/dispatch_reply_message.h (right):

http://codereview.chromium.org/10815073/diff/6002/ppapi/proxy/dispatch_reply_...
ppapi/proxy/dispatch_reply_message.h:5: #ifndef
PPAPI_PROXY_DISPATCH_RESPLY_MESSAGE_H_
RESPLY->REPLY

http://codereview.chromium.org/10815073/diff/6002/ppapi/proxy/dispatch_reply_...
ppapi/proxy/dispatch_reply_message.h:10: // We want to take a
ResourceMessageREplyParams as a parameter.
-ResourceMessageReplyParams
-Consider a similar comment in ppapi/host/dispatch_host_message.h

[brettw, 2012-07-30 05:46:36]

https://chromiumcodereview.appspot.com/10815073/diff/1/chrome/renderer/pepper...
File chrome/renderer/pepper/pepper_flash_renderer_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/1/chrome/renderer/pepper...
chrome/renderer/pepper/pepper_flash_renderer_message_filter.cc:13:
PepperFlashRendererMessageFilter::PepperFlashRendererMessageFilter(
I may actually end up doing that. I'm going to try to implement gamepad as a
resource even though its not exposed to the plugin. We'll see if that works and
maybe I can rip this out.

https://chromiumcodereview.appspot.com/10815073/diff/1/content/browser/ppapi_...
File content/browser/ppapi_plugin_process_host.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/1/content/browser/ppapi_...
content/browser/ppapi_plugin_process_host.cc:130: host_impl_.reset(new
content::BrowserPpapiHostImpl(this, permissions));
They don't yet. The browser side still has some stuff to do.

https://chromiumcodereview.appspot.com/10815073/diff/1/content/browser/render...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h
(right):

https://chromiumcodereview.appspot.com/10815073/diff/1/content/browser/render...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:15:
class ContentBrowserPepperHostFactory : public ppapi::host::HostFactory {
On 2012/07/25 16:53:22, raymes wrote:
> I realize this is not directly related to this CL but I think it would be
nicer
> if HostFactory and friends were named ResourceHostFactory (since it's creating
> ResourceHosts and not PpapiHosts).

Sounds good, I'll make a note.

> Even clearer (though longer) might be:
> we have PluginResource
> PluginResourceHost
> PluginResourceHostFactory

I'll consider this too when I do that.

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/1/content/renderer/peppe...
content/renderer/pepper/renderer_ppapi_host_impl.cc:110: return
!!GetAndValidateInstance(instance);
On 2012/07/25 16:53:22, raymes wrote:
> Looks like you forgot to implement this properly?

Whoops! Copied from the old implementation.

> What other methods would you expect to go in here?

Not sure, but I think there will be a bunch of stuff. I'd like to limit the
resource host's impls on WebKit.

https://chromiumcodereview.appspot.com/10815073/diff/6002/ppapi/examples/2d/p...
File ppapi/examples/2d/paint_manager_example.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/6002/ppapi/examples/2d/p...
ppapi/examples/2d/paint_manager_example.cc:63:
chooser_.Show(callback_factory_.NewCallbackWithOutput(&MyInstance::GotFile));
Ignore this file, I was using it as a manual test. I'll revert before checkin.

[brettw, 2012-07-30 05:46:52]

(new snap up)

[raymes, 2012-07-30 17:23:31]

lgtm with some nits.

Also not sure if having more tests (e.g. for the in-process routing stuff) makes
sense.

https://chromiumcodereview.appspot.com/10815073/diff/8015/content/content_bro...
File content/content_browser.gypi (right):

https://chromiumcodereview.appspot.com/10815073/diff/8015/content/content_bro...
content/content_browser.gypi:589:
'browser/renderer_host/pepper/browser_ppapi_host_impl.h',
These are out of order.

https://chromiumcodereview.appspot.com/10815073/diff/8015/content/renderer/pe...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/8015/content/renderer/pe...
content/renderer/pepper/pepper_in_process_resource_creation.cc:12: #include
"content/renderer/pepper/pepper_in_process_router.h"
Order is wrong

https://chromiumcodereview.appspot.com/10815073/diff/8015/content/renderer/pe...
File content/renderer/pepper/renderer_ppapi_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10815073/diff/8015/content/renderer/pe...
content/renderer/pepper/renderer_ppapi_host_impl.cc:112: return NULL;
return false

https://chromiumcodereview.appspot.com/10815073/diff/8015/ppapi/shared_impl/r...
File ppapi/shared_impl/resource.h (right):

https://chromiumcodereview.appspot.com/10815073/diff/8015/ppapi/shared_impl/r...
ppapi/shared_impl/resource.h:178: // sends).
This comment is out of date now

https://chromiumcodereview.appspot.com/10815073/diff/8015/webkit/plugins/ppap...
File webkit/plugins/ppapi/plugin_module.h (right):

https://chromiumcodereview.appspot.com/10815073/diff/8015/webkit/plugins/ppap...
webkit/plugins/ppapi/plugin_module.h:67: class EmbedderState {
Not sure if it makes sense to be more descriptive about what this is for (rather
than just calling it embedderstate).