linux plugins: eliminate GtkWidget* from plugin process

linux plugins: eliminate GtkWidget* from plugin process

[retry with mac hopefully fixed this time]

GtkWidget* (and its wrapper, gfx::NativeView) only work within a
single process.  Plugins already work with a lower-level type that
works across processes -- an X Window id.  The parent of a plugin
is an HWND on windows, but it's an X Window id on X.  So we
introduce a new typedef wrapping that and push it through all the
places in the code that needs it.

Since we no longer have a GtkSocket in the WebPluginDelegateImpl,
we need to do a bit more work in the WebViewDelegate (aka the
browser process in the multiproc world).

We also need the plugin host (the browser) to track the
GtkSockets that are hosting the plugin, as well as destroy them
when necessary.  To do this we require the plugin owner to
grab the plug-removed signal rather than putting its handler in
GtkPluginContainer; this is the way it worked before I'd refactored
into GtkPluginContainer so it shouldn't be so bad.

This change still only works in test_shell, but the refactoring
should translate to the multiprocess case pretty easily.

[Evan Martin, 2009-06-23 01:08:53]

Not ready for commit, just showing where I'm headed.

Unfortunately the WebPluginDelegateImpl API is such that it wants the parent
handle ready to go *before* we've actually committed to instantiating the
plugin.  Need to poke around a bit to figure out what we really want to do. 
Maybe it's ok because we only fail to instantiate the plugin on error and we'll
need to handle that specially anyway.

[Evan Martin, 2009-06-23 02:02:21]

Ok, now I've got enough working so you can see where moving/resizing will need
to live from now on.

There are only two plugin in this world: the one owned by Flash that we never
see, and the one on the test webview delegate.  We no longer have a window to
mess around with inside the webplugin_delegate_impl.

The positioning and sizing don't currently work in this patch.  :(  But xwininfo
-tree does show that I have those two subwindows twice when I load the basic
flash test.

[Evan Martin, 2009-06-23 02:02:44]

On 2009/06/23 02:02:21, Evan Martin wrote:
> Ok, now I've got enough working so you can see where moving/resizing will need
> to live from now on.
> 
> There are only two plugin in this world: the one owned by Flash that we never

only two *plugin-related windows*

> see, and the one on the test webview delegate.  We no longer have a window to
> mess around with inside the webplugin_delegate_impl.
> 
> The positioning and sizing don't currently work in this patch.  :(  But
xwininfo
> -tree does show that I have those two subwindows twice when I load the basic
> flash test.

[Antoine Labour, 2009-06-23 02:26:21]

It looks promising !

http://codereview.chromium.org/146009/diff/10/1009
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/146009/diff/10/1009#newcode126
Line 126: // TODO(evanm): figure out if we need a similar hack here.
That one is definitely needed by flash.
It sounds reasonable to add a field in PluginInstance for the renderer window
XID though.

[Evan Martin, 2009-06-24 01:30:19]

Still not ready for commit, but FYI this code now works again.

The plugin impl now takes in a plain X window and parents the plugin to it,
while the hosting impl manages the GtkSocket and positioning.

Crashes when I close, though -- still need to clean up all the destruction.

[Evan Martin, 2009-06-25 18:06:29]

I believe this is ready for commit.

John: please take a look at all the non-Linux-specific files, as well as at
least glance the Linux-specific changes in WebPluginDelegateImpl.  (I really do
promise to unfork that file soon.)

Antoine: the rest.

[Antoine Labour, 2009-06-25 18:22:05]

http://codereview.chromium.org/146009/diff/1044/2010
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/146009/diff/1044/2010#newcode280
Line 280: extra->colormap = DefaultColormap(GDK_DISPLAY(), 0);
For visual, depth, and colormap, should we take the values from the XID using
XGetWindowAttributes ?

http://codereview.chromium.org/146009/diff/1044/2011
File webkit/glue/webplugin.h (right):

http://codereview.chromium.org/146009/diff/1044/2011#newcode28
Line 28: NPAPI::PluginWindowHandle window;
Can you add a comment that on Windows, window is the actual plugin window,
whereas on Linux it's the gtksocket ?

[jam, 2009-06-25 18:25:35]

lgtm, just nitting

http://codereview.chromium.org/146009/diff/1044/2001
File base/gfx/native_widget_types.h (right):

http://codereview.chromium.org/146009/diff/1044/2001#newcode119
Line 119: namespace NPAPI {
I think it's confusing to put this in an NPAPI namespace since it's not defined
in headers from NPAPI but in our code.  Why not put it in webplugin.h?  Or keep
it in this file, but keep it in the gfx namespace.

http://codereview.chromium.org/146009/diff/1044/2003
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/146009/diff/1044/2003#newcode61
Line 61: Send(new PluginHostMsg_SetWindow(route_id_,
gfx::IdFromNativeView(window)));
should we just change PluginHostMsg_SetWindow to take in a PluginWindowHandle?

[Antoine Labour, 2009-06-25 19:09:51]

http://codereview.chromium.org/146009/diff/1044/2003
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/146009/diff/1044/2003#newcode61
Line 61: Send(new PluginHostMsg_SetWindow(route_id_,
gfx::IdFromNativeView(window)));
On 2009/06/25 18:25:35, John Abd-El-Malek wrote:
> should we just change PluginHostMsg_SetWindow to take in a PluginWindowHandle?

Good point, this is only used to set the value into the WebPluginGeometry
anyway.

[Antoine Labour, 2009-06-25 19:45:16]

http://codereview.chromium.org/146009/diff/1049/1064
File webkit/tools/test_shell/test_webview_delegate_gtk.cc (right):

http://codereview.chromium.org/146009/diff/1049/1064#newcode96
Line 96: shell_->webViewHost()->CreatePluginContainer();
Thinking about this some more... this breaks windowless plugins, where you don't
want to create the gtksocket, and the parent should be the frame.
CreatePluginContainer should really be called from
WebPluginDelegateImpl::WindowedCreatePlugin

[Evan Martin, 2009-06-25 20:59:11]

not ready to go due to Antoine's last comment, but the rest fixed.

http://codereview.chromium.org/146009/diff/1044/2001
File base/gfx/native_widget_types.h (right):

http://codereview.chromium.org/146009/diff/1044/2001#newcode119
Line 119: namespace NPAPI {
On 2009/06/25 18:25:35, John Abd-El-Malek wrote:
> I think it's confusing to put this in an NPAPI namespace since it's not
defined
> in headers from NPAPI but in our code.  Why not put it in webplugin.h?  Or
keep
> it in this file, but keep it in the gfx namespace.

I initially had it in webplugin.h, but since it appears all over our code base
it added a lot of dependencies on webplugin.h.

I'll change it to gfx::.

http://codereview.chromium.org/146009/diff/1044/2003
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/146009/diff/1044/2003#newcode61
Line 61: Send(new PluginHostMsg_SetWindow(route_id_,
gfx::IdFromNativeView(window)));
On 2009/06/25 18:25:35, John Abd-El-Malek wrote:
> should we just change PluginHostMsg_SetWindow to take in a PluginWindowHandle?

Yes.  I'm going to revisit this in the next change, where we finish the plumbing
through to the multi-proc world.

http://codereview.chromium.org/146009/diff/1044/2010
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/146009/diff/1044/2010#newcode280
Line 280: extra->colormap = DefaultColormap(GDK_DISPLAY(), 0);
On 2009/06/25 18:22:05, Antoine Labour wrote:
> For visual, depth, and colormap, should we take the values from the XID using
> XGetWindowAttributes ?

I'll add a TODO -- I expect this won't matter but it can't hurt.

http://codereview.chromium.org/146009/diff/1044/2011
File webkit/glue/webplugin.h (right):

http://codereview.chromium.org/146009/diff/1044/2011#newcode28
Line 28: NPAPI::PluginWindowHandle window;
On 2009/06/25 18:22:05, Antoine Labour wrote:
> Can you add a comment that on Windows, window is the actual plugin window,
> whereas on Linux it's the gtksocket ?

Done.

http://codereview.chromium.org/146009/diff/1049/1064
File webkit/tools/test_shell/test_webview_delegate_gtk.cc (right):

http://codereview.chromium.org/146009/diff/1049/1064#newcode96
Line 96: shell_->webViewHost()->CreatePluginContainer();
On 2009/06/25 19:45:17, Antoine Labour wrote:
> Thinking about this some more... this breaks windowless plugins, where you
don't
> want to create the gtksocket, and the parent should be the frame.
> CreatePluginContainer should really be called from
> WebPluginDelegateImpl::WindowedCreatePlugin

Oh, lame!  WindowedCreatePlugin happens at the wrong place (in the multi-proc
window, we IPC over the ::Create call, but not that one).  I will have to think
about it some more...

[Antoine Labour, 2009-06-25 21:55:19]

http://codereview.chromium.org/146009/diff/1049/1064
File webkit/tools/test_shell/test_webview_delegate_gtk.cc (right):

http://codereview.chromium.org/146009/diff/1049/1064#newcode96
Line 96: shell_->webViewHost()->CreatePluginContainer();
On 2009/06/25 20:59:12, Evan Martin wrote:
> On 2009/06/25 19:45:17, Antoine Labour wrote:
> > Thinking about this some more... this breaks windowless plugins, where you
> don't
> > want to create the gtksocket, and the parent should be the frame.
> > CreatePluginContainer should really be called from
> > WebPluginDelegateImpl::WindowedCreatePlugin
> 
> Oh, lame!  WindowedCreatePlugin happens at the wrong place (in the multi-proc
> window, we IPC over the ::Create call, but not that one).  I will have to
think
> about it some more...

I think we can spin it around: get the WebPluginDelegate ask the WebPlugin to
create the GtkSocket for it, that would then ask the browser. It's 2 process
hops, but I think we can make it work relatively easily. Going straight to the
browser may work too but it sounds harder actually.

[Evan Martin, 2009-06-25 23:36:27]

I've uploaded a new version that shuts down ok.

Would you mind if I commit this, then tackle the refactoring to get the socket
creation fixed for windowless in a subsequent change?  It still displays right
now and doesn't crash so I think it's ok...

[Antoine Labour, 2009-06-26 00:08:42]

On 2009/06/25 23:36:27, Evan Martin wrote:
> I've uploaded a new version that shuts down ok.
> 
> Would you mind if I commit this, then tackle the refactoring to get the socket
> creation fixed for windowless in a subsequent change?  It still displays right
> now and doesn't crash so I think it's ok...

Yeah, that's ok. LGTM.