Hook up the PPB_Flash_Print interface to new host system.

Hook up the PPB_Flash_Print interface to new host system.

This adds the ability to implement "instance" messages (as opposed to resource messages) to PpapiHost via a message filter interface. The ownership model for these filters works just like RenderViewObserver. All non-resource messages are sent through this list of filters.

This adds the ability to add such filters in the Chrome layer (as opposed to just content) by plumbing through some notifications. This patch responds to the trivial "Flash print" interface by calling the existing function in the renderer.

This doesn't change the in-process case. Making this code path work in process will require that the "core" instance interface be done first or at the same time. As a result, the old in-process implementation is kept (it forwards to the same function in the end).

This patch adds a HostResourceFactory for Chrome but doesn't hook it up yet. There is a TODO for this. I need to conver the host factories to a filter-like system to allow dynamic adding of filters from the Chrome layer. I'll do this in a follow-up patch.

TEST=manual
BUG=none

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

[brettw, 2012-07-20 05:03:40]

This is mostly just a bunch of plumbing to implement one trivial function "the
new way" in Chrome.

John: content client and RenderViewImpl integration.
David: everything else.

[jam, 2012-07-20 06:07:00]

lgtm

[brettw, 2012-07-23 04:18:40]

David: hold off on looking at this for a bit.

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

I guess you can review this.

I changed some minor parts of this in a patch on top of this:
http://codereview.chromium.org/10815073/

which cleans up some of the weird parts of the old design.

[dmichael (off chromium), 2012-07-25 16:57:27]

http://codereview.chromium.org/10803050/diff/2001/chrome/renderer/pepper/chro...
File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc (right):

http://codereview.chromium.org/10803050/diff/2001/chrome/renderer/pepper/chro...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc:14:
ChromeRendererPepperHostFactory::ChromeRendererPepperHostFactory(){
nit: space between ) and {

http://codereview.chromium.org/10803050/diff/2001/chrome/renderer/pepper/chro...
File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.h (right):

http://codereview.chromium.org/10803050/diff/2001/chrome/renderer/pepper/chro...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.h:17: : public
ppapi::host::HostFactory {
nit: it looks like your base class could fit on the line above

http://codereview.chromium.org/10803050/diff/2001/chrome/renderer/pepper/chro...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.h:19: explicit
ChromeRendererPepperHostFactory();
nit: explicit is unnecessary

http://codereview.chromium.org/10803050/diff/2001/chrome/renderer/pepper/pepp...
File chrome/renderer/pepper/pepper_flash_renderer_message_filter.h (right):

http://codereview.chromium.org/10803050/diff/2001/chrome/renderer/pepper/pepp...
chrome/renderer/pepper/pepper_flash_renderer_message_filter.h:21: explicit
PepperFlashRendererMessageFilter(ppapi::host::PpapiHost* host);
optional: It seems like this is following the same pattern as
RenderViewObservers, but the pattern might be clearer (and impossible to get
wrong) if you made the constructor private and had a static Create function that
only heap allocates the object (and maybe doesn't even return a pointer). The
"new Blah(renderview)" stuff threw me for a loop at first.

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

http://codereview.chromium.org/10803050/diff/2001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_in_process_resource_creation.cc:10: #include
"content/renderer/render_view_impl.h"
nit: include order

http://codereview.chromium.org/10803050/diff/2001/ppapi/host/instance_message...
File ppapi/host/instance_message_filter.cc (right):

http://codereview.chromium.org/10803050/diff/2001/ppapi/host/instance_message...
ppapi/host/instance_message_filter.cc:26: PpapiHostDestroyed(host);
If the host is in the process of being destroyed, maybe passing host is
potentially a little dangerous. I would take that param out, unless you're sure
you need it.

Should we consider having |delete this| here at the end, so we can't forget? Or
are you anticipating some of these filters outliving the host? Alternatively,
the PpapiHostDestroyed virtual function could be pure-virtual, just to force
derived classes to think about it.

http://codereview.chromium.org/10803050/diff/2001/ppapi/host/instance_message...
File ppapi/host/instance_message_filter.h (right):

http://codereview.chromium.org/10803050/diff/2001/ppapi/host/instance_message...
ppapi/host/instance_message_filter.h:44: void CallPpapiHostDestroyed();
nit: Since it does a little more than call PpapiHostDestroyed, maybe should be
PpapiHostWasDestroyed()?

[brettw, 2012-07-26 16:18:12]

I actually redid the ownership completely and just pass them is as scoped_ptrs
and got rid of the weird special observer iteration stuff. I don't think there
is a use-case for dynamically removing observers for this feature. It should be
much easier to follow now.

https://chromiumcodereview.appspot.com/10803050/diff/2001/ppapi/host/instance...
File ppapi/host/instance_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10803050/diff/2001/ppapi/host/instance...
ppapi/host/instance_message_filter.cc:26: PpapiHostDestroyed(host);
This seems reasonable. I'm not actually sure how this will end up being used.

[dmichael (off chromium), 2012-07-27 15:44:57]

lgtm

This looks simpler, thanks.

[commit-bot: I haz the power, 2012-07-27 19:37:13]

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

[commit-bot: I haz the power, 2012-07-27 20:57:40]

Try job failure for 10803050-13001 (retry) on mac_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...