linux: OOP windowed plugins

linux: OOP windowed plugins

There are still a few issues, but that's a start.
- only windowed plugins
- we can't currently create the gtksocket in background tabs, because their gtkwidgets are not yet in the hierarchy, so they can't be realized (that's what gives the XID).
- the plugin process talks to the browser process through the renderer process to create/destroy the gtksockets, because the plugin doesn't know which renderer it's talking to. We need a bit more plumbing to be able to have direct IPC.
- some code is duplicated between chrome and test_shell. We should probably refactor it, but I'm not sure where the common part should live.

Patch from Antoine Labour <piman@google.com>;, with some touchups by me.

[Antoine Labour, 2009-07-01 00:59:40]

evan: review
others: FYI, but feel free to chime in.

[Evan Martin, 2009-07-01 01:42:32]

John and I were just discussing this problem, that "the plugin process talks to
the browser process through the renderer process to create/destroy the
gtksockets, because the plugin doesn't know which renderer it's talking to". 
Couldn't we use the gfx::NativeViewId of the parent?  I guess we'd still need an
extra map on the side, mapping NativeViewIds to RenderViewHosts?

http://codereview.chromium.org/146078/diff/3020/4005
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/146078/diff/3020/4005#newcode474
Line 474: gtk_init(&argc, const_cast<char***>(&argv));
Maybe put a comment in here like "from the plugin's perspective, it's embedded
in a GTK-based browser like Firefox" to explain why we need this special extra
init here.

http://codereview.chromium.org/146078/diff/3020/4009
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/146078/diff/3020/4009#newcode26
Line 26: #include "webkit/glue/plugins/gtk_plugin_container.h"
alphabetize

http://codereview.chromium.org/146078/diff/3020/4009#newcode461
Line 461: const WebPluginGeometry& move = plugin_window_moves[i];
maybe add a TODO to refacotr this against test_shell?

or maybe we could stick a class in webkit/glue like i did with the plugin
container to manage this sort of thing?

http://codereview.chromium.org/146078/diff/3020/4009#newcode491
Line 491: if (move.window_rect.x() != current_x || move.window_rect.y() !=
current_y) {
80 cols

http://codereview.chromium.org/146078/diff/3020/4010
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/146078/diff/3020/4010#newcode82
Line 82: GtkWidget* MapIDToWidget(gfx::PluginWindowHandle id);
Comments on these member functions.

I see what you mean about duplicated code, hrmmm....

http://codereview.chromium.org/146078/diff/3020/4010#newcode89
Line 89: gboolean OnPlugRemoved(GtkSocket* socket);
Seems like a lot of plumbing for a static function that returns TRUE...

http://codereview.chromium.org/146078/diff/3020/4010#newcode147
Line 147: typedef std::map<gfx::PluginWindowHandle, GtkWidget*>
PluginWindowToWidgetMap;
Comments on these member variables.  Whitespace above.

http://codereview.chromium.org/146078/diff/3020/4020
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/146078/diff/3020/4020#newcode251
Line 251: params.containing_window = 0;
I think that perhaps this should be NativeViewId (see mail at beginning of
review).

[Antoine Labour, 2009-07-01 02:32:24]

On 2009/07/01 01:42:32, Evan Martin wrote:
> John and I were just discussing this problem, that "the plugin process talks
to
> the browser process through the renderer process to create/destroy the
> gtksockets, because the plugin doesn't know which renderer it's talking to". 
> Couldn't we use the gfx::NativeViewId of the parent?  I guess we'd still need
an
> extra map on the side, mapping NativeViewIds to RenderViewHosts?

Yeah, I was considering reverting that part of the patch. Did your
NativeViewId->XID IPC ever make it in ? I can probably resurrect it otherwise.

[jam, 2009-07-01 02:35:37]

http://codereview.chromium.org/146078/diff/3020/4005
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/146078/diff/3020/4005#newcode474
Line 474: gtk_init(&argc, const_cast<char***>(&argv));
nit: I think this belongs in PluginMain()

http://codereview.chromium.org/146078/diff/3020/4011
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/146078/diff/3020/4011#newcode552
Line 552: render_widget_helper_->CreatePluginContainer(opener_id, reply_msg,
this);
There's no need to manually intercept the message in ResourceMessageFilter and
go back and forth to the UI thread.  You can just send it as a routed sync
message, and it'll be dispatched in RenderViewHost.

You might need to surround the "if (msg.is_sync() &&
!msg.is_caller_pumping_messages()) {" check with an ifdef for Windows.

http://codereview.chromium.org/146078/diff/3020/4016
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/146078/diff/3020/4016#newcode924
Line 924: IPC_SYNC_MESSAGE_CONTROL1_1(ViewHostMsg_CreatePluginContainer,
Note: if now, or in the future, there is any sync calls on Linux from the
browser to the plugin process (whether from the system or our code), this can
cause deadlocks.  Are you guys 100% sure this doesn't happen?  On Windows,
painting/scrolling/events etc cause the OS to do sync calls to child windows.

[Antoine Labour, 2009-07-01 07:23:59]

http://codereview.chromium.org/146078/diff/3020/4005
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/146078/diff/3020/4005#newcode474
Line 474: gtk_init(&argc, const_cast<char***>(&argv));
On 2009/07/01 02:35:38, John Abd-El-Malek wrote:
> nit: I think this belongs in PluginMain()

Done.

http://codereview.chromium.org/146078/diff/3020/4005#newcode474
Line 474: gtk_init(&argc, const_cast<char***>(&argv));
On 2009/07/01 01:42:32, Evan Martin wrote:
> Maybe put a comment in here like "from the plugin's perspective, it's embedded
> in a GTK-based browser like Firefox" to explain why we need this special extra
> init here.

Done.

http://codereview.chromium.org/146078/diff/3020/4009
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/146078/diff/3020/4009#newcode26
Line 26: #include "webkit/glue/plugins/gtk_plugin_container.h"
On 2009/07/01 01:42:32, Evan Martin wrote:
> alphabetize

Done.

http://codereview.chromium.org/146078/diff/3020/4009#newcode461
Line 461: const WebPluginGeometry& move = plugin_window_moves[i];
On 2009/07/01 01:42:32, Evan Martin wrote:
> maybe add a TODO to refacotr this against test_shell?
> 
> or maybe we could stick a class in webkit/glue like i did with the plugin
> container to manage this sort of thing?

Done.

http://codereview.chromium.org/146078/diff/3020/4010
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/146078/diff/3020/4010#newcode82
Line 82: GtkWidget* MapIDToWidget(gfx::PluginWindowHandle id);
On 2009/07/01 01:42:32, Evan Martin wrote:
> Comments on these member functions.
> 
> I see what you mean about duplicated code, hrmmm....

I factored the code into a separate class.

http://codereview.chromium.org/146078/diff/3020/4010#newcode89
Line 89: gboolean OnPlugRemoved(GtkSocket* socket);
On 2009/07/01 01:42:32, Evan Martin wrote:
> Seems like a lot of plumbing for a static function that returns TRUE...

Done.

http://codereview.chromium.org/146078/diff/3020/4010#newcode147
Line 147: typedef std::map<gfx::PluginWindowHandle, GtkWidget*>
PluginWindowToWidgetMap;
On 2009/07/01 01:42:32, Evan Martin wrote:
> Comments on these member variables.  Whitespace above.

These moved to a separate class.

http://codereview.chromium.org/146078/diff/3020/4011
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/146078/diff/3020/4011#newcode552
Line 552: render_widget_helper_->CreatePluginContainer(opener_id, reply_msg,
this);
On 2009/07/01 02:35:38, John Abd-El-Malek wrote:
> There's no need to manually intercept the message in ResourceMessageFilter and
> go back and forth to the UI thread.  You can just send it as a routed sync
> message, and it'll be dispatched in RenderViewHost.
> 
> You might need to surround the "if (msg.is_sync() &&
> !msg.is_caller_pumping_messages()) {" check with an ifdef for Windows.

Ok, I thought there might be a better way of doing it :)
Done.

http://codereview.chromium.org/146078/diff/3020/4016
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/146078/diff/3020/4016#newcode924
Line 924: IPC_SYNC_MESSAGE_CONTROL1_1(ViewHostMsg_CreatePluginContainer,
On 2009/07/01 02:35:38, John Abd-El-Malek wrote:
> Note: if now, or in the future, there is any sync calls on Linux from the
> browser to the plugin process (whether from the system or our code), this can
> cause deadlocks.  Are you guys 100% sure this doesn't happen?  On Windows,
> painting/scrolling/events etc cause the OS to do sync calls to child windows.

Those don't happen. Contrary to Windows, there's no magic going on with hidden
synchronous communication between processes.
Anything related to painting or events goes through the X socket, which is
properly handled by the UI message loop.
In addition, it is never the case that a process will block on waiting for
another one to execute its event loop, X as the central point manages all this,
asynchronously (e.g. X never blocks waiting for its clients to process events).
If we ever add a side-channel communication pipe or socket, all we really need
is to add its FD and handlers to the main loop.

http://codereview.chromium.org/146078/diff/3020/4020
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/146078/diff/3020/4020#newcode251
Line 251: params.containing_window = 0;
On 2009/07/01 01:42:32, Evan Martin wrote:
> I think that perhaps this should be NativeViewId (see mail at beginning of
> review). 

Yeah, I changed it back, and merged your MapNativeViewId IPC.

[jam, 2009-07-01 17:09:30]

lgtm

http://codereview.chromium.org/146078/diff/3077/3078
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/146078/diff/3077/3078#newcode297
Line 297: void PluginProcessHost::OnMapNativeViewId(gfx::NativeViewId id,
nit: better to put this at the end of the message handlers below.  That's more
consistent with the rest of the code.  The Windows only dispatchers were put at
the top in this case so they can be in the same ifdef as the large
PluginDownloadUrlHelper class which needed to be at the top.

http://codereview.chromium.org/146078/diff/3077/3099
File webkit/tools/test_shell/webview_host.h (right):

http://codereview.chromium.org/146078/diff/3077/3099#newcode57
Line 57: // Helper class that creates and moves plguin containers.
nit: spelling

[Evan Martin, 2009-07-03 01:23:17]

Really sorry, this slipped off my radar.  If I don't review something within an
hour or so in the future please ping me again.

http://codereview.chromium.org/146078/diff/3077/3080
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/146078/diff/3077/3080#newcode658
Line 658: #if defined(OS_WIN)
Put a comment in here about why this doesn't apply to mac/linux.

http://codereview.chromium.org/146078/diff/3077/3084
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/146078/diff/3077/3084#newcode38
Line 38: gtk_fixed_set_has_window(GTK_FIXED(widget), true);
s/true/TRUE/

http://codereview.chromium.org/146078/diff/3077/3084#newcode478
Line 478: return;
this test isn't necessary anymore

http://codereview.chromium.org/146078/diff/3077/3084#newcode479
Line 479: for (unsigned int i = 0; i < plugin_window_moves.size(); ++i) {
use size_t for iterator

http://codereview.chromium.org/146078/diff/3077/3085
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/146078/diff/3077/3085#newcode19
Line 19: class GtkPluginContainerHost;
this forward declare doesn't seem necessary, since you're including the full
header above

http://codereview.chromium.org/146078/diff/3077/3085#newcode24
Line 24: typedef struct _GtkSocket GtkSocket;
this forward declare doesn't seem necessary?

http://codereview.chromium.org/146078/diff/3077/3089
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/146078/diff/3077/3089#newcode82
Line 82: gtk_init(&argc, const_cast<char***>(&argv_pointer));
Didn't you already add a gtk_init elsewhere?

are we concerned about gtk mutating the strings passed in?  i know it'll munge
argv to extract its own flags.  i thought the earlier call to gtk_init was
nearer to main() which made this easier.

http://codereview.chromium.org/146078/diff/3077/3093
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/146078/diff/3077/3093#newcode647
Line 647: windowless_ = window != static_cast<gfx::PluginWindowHandle>(0);
dunno if it'd be clearer, but you could do
  windowless_ = !!window;

http://codereview.chromium.org/146078/diff/3077/3095
File webkit/glue/plugins/gtk_plugin_container_host.cc (right):

http://codereview.chromium.org/146078/diff/3077/3095#newcode7
Line 7: #include <gtk/gtk.h>
extra newline after system header

http://codereview.chromium.org/146078/diff/3077/3095#newcode15
Line 15: static gboolean AlwaysTrue(void *unused) {
star on left

http://codereview.chromium.org/146078/diff/3077/3095#newcode130
Line 130: gboolean GtkPluginContainerHost::UnrealizeCallback(GtkWidget *widget,
stars on left

http://codereview.chromium.org/146078/diff/3077/3096
File webkit/glue/plugins/gtk_plugin_container_host.h (right):

http://codereview.chromium.org/146078/diff/3077/3096#newcode15
Line 15: // Helper class that creates and manages plugin containers (GtkSocket).
We use the term "host" for the browser-side half of something that exists across
browser/renderer processes.  Can you use another name for this? 
PluginContainerManager would do.

http://codereview.chromium.org/146078/diff/3077/3096#newcode29
Line 29: // Moves a plugin container.
This comment is clear from the name.  Could you add something about "takes an
update from webkit and moves/reshapes(?) the plugin accordingly"

http://codereview.chromium.org/146078/diff/3077/3096#newcode36
Line 36: // Call back for when the plugin container loses its XID, so that it
can be
Callback

http://codereview.chromium.org/146078/diff/3077/3096#newcode38
Line 38: static gboolean UnrealizeCallback(GtkWidget *widget, void *user_data);
stars on the left (GtkWidget* widget, void* user_data)

http://codereview.chromium.org/146078/diff/3077/3096#newcode40
Line 40: // Parent of the plugin containers.
Is this the GtkFixed?  Maybe mention that here?

http://codereview.chromium.org/146078/diff/3077/3098
File webkit/tools/test_shell/test_webview_delegate_gtk.cc (right):

http://codereview.chromium.org/146078/diff/3077/3098#newcode233
Line 233: GtkPluginContainerHost &plugin_container_host =
& on left.  i guess this will be a pointer once you make the ohter change

http://codereview.chromium.org/146078/diff/3077/3099
File webkit/tools/test_shell/webview_host.h (right):

http://codereview.chromium.org/146078/diff/3077/3099#newcode43
Line 43: #if defined(OS_LINUX)
merge this ifdef with the one above it?

if it returns a ref, it should be a const ref, otherwise it should return a
pointer.  const modifier on getters too

http://codereview.chromium.org/146078/diff/3077/3101
File webkit/webkit.gyp (right):

http://codereview.chromium.org/146078/diff/3077/3101#newcode4659
Line 4659: ['exclude', r'gtk_plugin_container_host\.(cc|h)']],
maybe just exclude r'^gtk_'  ?