PPAPI/NaCl: Reinitialize some stuff when the ipc proxy starts.

PPAPI/NaCl: Reinitialize some stuff when the ipc proxy starts.

Also refactor the PPP_Instance version checking so I don't have to write the fall-back from 1.1 to 1.0 yet again.

BUG=116317
TEST=


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

[dmichael (off chromium), 2012-06-12 21:25:46]

http://codereview.chromium.org/10543029/diff/2003/ppapi/proxy/ppapi_proxy_tes...
File ppapi/proxy/ppapi_proxy_test.cc (right):

http://codereview.chromium.org/10543029/diff/2003/ppapi/proxy/ppapi_proxy_tes...
ppapi/proxy/ppapi_proxy_test.cc:279: status_receiver_.release()));
This was causing a crash when I tried to run the unit tests...  HostDispatcher
expects to take ownership.

[brettw, 2012-06-14 17:58:35]

http://codereview.chromium.org/10543029/diff/12002/ppapi/shared_impl/ppp_inst...
File ppapi/shared_impl/ppp_instance_combined.h (right):

http://codereview.chromium.org/10543029/diff/12002/ppapi/shared_impl/ppp_inst...
ppapi/shared_impl/ppp_instance_combined.h:19: typedef const
void*(GetInterfaceFunc)(const char*);
You can use PPP_GetInterface_Func and delete this.

http://codereview.chromium.org/10543029/diff/12002/ppapi/shared_impl/ppp_inst...
ppapi/shared_impl/ppp_instance_combined.h:22: // interface.
Can you mention that this can return NULL if the plugin doesn't support any
instance interface?

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_module.cc:500:
out_of_process_proxy_->AddInstance(instance);
Can you add a comment about why this is necessary here but not in the
"InitAsProxied" case?

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_module.cc:523: 
I'd just remove this blank line, I don't think it adds anything.

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2114: ::ppapi::ViewData
empty_view;
I don't get why this is the "empty" view. Wouldn't we want to send the real view
info?

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:101: // function.
Can you document that this can return NULL?

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:521: bool
suppress_did_change_view_;
Do we still use this?

[dmichael (off chromium), 2012-06-14 19:37:18]

Addressed comments. I'm seeing a failure for OutOfProcessPPAPITest.Fullscreen on
Mac trybots right now; not sure if it's related to the CL yet.

http://codereview.chromium.org/10543029/diff/12002/ppapi/shared_impl/ppp_inst...
File ppapi/shared_impl/ppp_instance_combined.h (right):

http://codereview.chromium.org/10543029/diff/12002/ppapi/shared_impl/ppp_inst...
ppapi/shared_impl/ppp_instance_combined.h:19: typedef const
void*(GetInterfaceFunc)(const char*);
On 2012/06/14 17:58:35, brettw wrote:
> You can use PPP_GetInterface_Func and delete this.
Turns out I wasn't using it anymore anyway...  instead, I use base::Callback
(which allows me to bind the module pointer so I can use
PluginModule::GetPluginInterface here). Deleted.

http://codereview.chromium.org/10543029/diff/12002/ppapi/shared_impl/ppp_inst...
ppapi/shared_impl/ppp_instance_combined.h:22: // interface.
On 2012/06/14 17:58:35, brettw wrote:
> Can you mention that this can return NULL if the plugin doesn't support any
> instance interface?

Done.

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_module.cc:500:
out_of_process_proxy_->AddInstance(instance);
On 2012/06/14 17:58:35, brettw wrote:
> Can you add a comment about why this is necessary here but not in the
> "InitAsProxied" case?

Done.

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_module.cc:523: 
On 2012/06/14 17:58:35, brettw wrote:
> I'd just remove this blank line, I don't think it adds anything.

Done.

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2114: ::ppapi::ViewData
empty_view;
On 2012/06/14 17:58:35, brettw wrote:
> I don't get why this is the "empty" view. Wouldn't we want to send the real
view
> info?
This is for the "previous" view; SendDidChangeView always sends the current
view_data_. Currently, the only important thing about the previous view_data
parameter is whether it's different than the previous view (in fact, since I
clear sent_inital_did_change_view_, that doesn't matter either). So this might
be overkill. But I wanted it to look like the initial call to SendDidChangeView
as possible (e.g., in case we decide to provide the previous view in some way in
the API).

I updated the comments to try to make it clearer.

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:101: // function.
On 2012/06/14 17:58:35, brettw wrote:
> Can you document that this can return NULL?

Done.

http://codereview.chromium.org/10543029/diff/12002/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:521: bool
suppress_did_change_view_;
On 2012/06/14 17:58:35, brettw wrote:
> Do we still use this?
Oops, meant to delete that. Done, thanks.

[brettw, 2012-06-14 23:49:57]

lgtm

[commit-bot: I haz the power, 2012-06-15 17:49:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10543029/10006

[commit-bot: I haz the power, 2012-06-15 18:53:00]

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

[bbudge, 2012-06-16 01:15:32]

IIRC, the Fullscreen tests are sensitive to DidChangeView event count. Perhaps
the consolidation is causing them to break. I can have a look next week.

[commit-bot: I haz the power, 2012-06-19 17:57:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10543029/26001

[commit-bot: I haz the power, 2012-06-19 19:03:41]

Change committed as 143006

[bbudge, 2012-06-19 23:29:46]

http://codereview.chromium.org/10543029/diff/10006/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/10543029/diff/10006/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_module.cc:505:
out_of_process_proxy_->AddInstance(instance);
I think I'm doing this in chrome/renderer/pepper/ppb_nacl_private_impl.cc and
that it's better done there (matches what the normal out-of-process proxy init
code looks like.) It's harmless to do it twice though.

[dmichael (off chromium), 2012-06-20 20:28:55]

On Tue, Jun 19, 2012 at 5:29 PM, <bbudge@chromium.org> wrote:

>
> http://codereview.chromium.**org/10543029/diff/10006/**
>
webkit/plugins/ppapi/plugin_**module.cc<http://codereview.chromium.org/10543029/diff/10006/webkit/plugins/ppapi/plugin_module.cc>
> File webkit/plugins/ppapi/plugin_**module.cc (right):
>
> http://codereview.chromium.**org/10543029/diff/10006/**
>
webkit/plugins/ppapi/plugin_**module.cc#newcode505<http://codereview.chromium.org/10543029/diff/10006/webkit/plugins/ppapi/plugin_module.cc#newcode505>
> webkit/plugins/ppapi/plugin_**module.cc:505:
> out_of_process_proxy_->**AddInstance(instance);
> I think I'm doing this in
> chrome/renderer/pepper/ppb_**nacl_private_impl.cc and that it's better
> done there (matches what the normal out-of-process proxy init code looks
> like.) It's harmless to do it twice though.
>
I did it here because PluginModule is also responsible for doing it in the
normal out-of-process case. I think you can argue it either way; but we
probably shouldn't do it twice (so if we ever want/need to "move" it or
change it, we do it in one place).

This is already landed, but if you think it belongs elsewhere, let's change
it in another CL (maybe your channel one, or a separate one).


>
http://codereview.chromium.**org/10543029/<http://codereview.chromium.org/105...
>