linux (and some posix): multiprocess plugins compiling.

linux (and some posix): multiprocess plugins compiling.

The goal of this change is to *not* make any behavioral change, but to
instead just get all the plugin-related files linking on Linux with
a bunch of NOTIMPLEMENTED()s in the appropriate places.  It's enormous
enough already without any refactorings or new features.

Changes include:
 - Lots of gcc warning fixes.
 - Use portable replacements for Windows-specific functions (_strdup, etc.).
 - Use TransportDIB instead of just shared memory in the plugin messaging.
   Note that this is not fleshed out on Linux and on Windows it just hacks
   in the existing handles so there should be no functional change.
 - Fix --plugin-launcher to use cross-platform APIs.

[Evan Martin, 2009-04-16 21:15:55]

john: primary reviewer.
agl: would you glance at the transport dib-related changes
(plugin_messages_internal.h, webplugin_delegate_stub.*, webplugin_proxy.*)
amanda: just FYI

[Evan Martin, 2009-04-16 21:16:07]

+dean FYI

[agl, 2009-04-16 22:06:02]

LGTM. Evan and I chatted about how TransportDIBs work and the future directions
for this.

[jam, 2009-04-16 23:02:54]

very cool

note I'm not that familiar with TransportDIB so I'll take your word on the
Webplugin_delegate_proxy usage.

some comments:

http://codereview.chromium.org/79020/diff/1/4
File base/process.h (right):

http://codereview.chromium.org/79020/diff/1/4#newcode34
Line 34: ~Process() { Close(); }
This is a big change in semantics, the code before wasn't calling Close as an
oversight..

As an example, look at how ChildProcessInfo uses Process.  That object is cloned
many times, i.e. to go across threads. Closing the handle in one of them would
cause the other ChildProcessInfo to have invalid handles.

http://codereview.chromium.org/79020/diff/1/8
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/79020/diff/1/8#newcode474
Line 474: base::LaunchApp(cmd_line.argv(), fds_to_map, false, &process);
this looks like a copy of the code in BrowserRenderProcessHost.  Can it be
refactored?  (it'll also be needed when workers are ported).

http://codereview.chromium.org/79020/diff/1/20
File chrome/plugin/chrome_plugin_host.cc (right):

http://codereview.chromium.org/79020/diff/1/20#newcode422
Line 422: handler = handler;
hmm this is kinda ugly, is there no other way to silence the error?  What if
there's just a "new PluginRequestHandlerProxy(plugin, cprequest);"?

http://codereview.chromium.org/79020/diff/1/25
File chrome/plugin/npobject_util.cc (right):

http://codereview.chromium.org/79020/diff/1/25#newcode16
Line 16: #include "chrome/common/win_util.h"
This doesn't look like it's still needed..

http://codereview.chromium.org/79020/diff/1/25#newcode193
Line 193: object_stub = object_stub;
like the other warning, can you get rid of this by removing the "NPObjectStub*
object_stub = "?

http://codereview.chromium.org/79020/diff/1/27
File chrome/plugin/plugin_channel.cc (right):

http://codereview.chromium.org/79020/diff/1/27#newcode103
Line 103: #endif
I think it was a mistake to make this use InterlockedIncrement, since this is
only used on one thread.  so just a return ++last_id is fine..

http://codereview.chromium.org/79020/diff/1/27#newcode116
Line 116: renderer_process_.Close();
you'll want to add another Close() in the dtor, after getting rid of the change
to make Process() close the handle in its dtor.

http://codereview.chromium.org/79020/diff/1/31
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/79020/diff/1/31#newcode64
Line 64: CommandLine compiler_warning = parsed_command_line;
this hurts my eyes every time I see it :)  is there no cleaner way of avoiding
this??

http://codereview.chromium.org/79020/diff/1/31#newcode92
Line 92: // we will.
I think you can remove the TODO(port) without worrying, since I doubt you'll try
plugin sandboxing any time soon, and if you do, you might have better ways of
testing that code..

http://codereview.chromium.org/79020/diff/1/32
File chrome/plugin/plugin_thread.cc (right):

http://codereview.chromium.org/79020/diff/1/32#newcode92
Line 92: base::KillProcess(base::GetCurrentProcessHandle(), 0, /* wait= */
false);
nit: I'm really not a fan of putting variable names beside arguments, especially
combined with our 80 character limit..

http://codereview.chromium.org/79020/diff/1/33
File chrome/plugin/plugin_thread.h (right):

http://codereview.chromium.org/79020/diff/1/33#newcode11
Line 11: #include "webkit/glue/plugins/plugin_lib.h"
we shouldn't have to include glue code just to get access to cross platform
module pointers.  Can you move that code to base?

http://codereview.chromium.org/79020/diff/1/34
File chrome/plugin/webplugin_delegate_stub.cc (right):

http://codereview.chromium.org/79020/diff/1/34#newcode280
Line 280: stub = stub;
eyes hurt!

http://codereview.chromium.org/79020/diff/1/36
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/79020/diff/1/36#newcode61
Line 61: NOTIMPLEMENTED();
I would take out the NOTIMPLEMENTED, this is a performance optimization on
Windows (creating an intermediate HWND in the browser process to speed up
scrolling).

http://codereview.chromium.org/79020/diff/1/36#newcode95
Line 95: NOTIMPLEMENTED();
ditto

http://codereview.chromium.org/79020/diff/1/39
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/79020/diff/1/39#newcode68
Line 68: multibyte_response_expected_ = (existing_stream != 0);
I think the NULL is more readable..

http://codereview.chromium.org/79020/diff/1/39#newcode205
Line 205: TransportDIB::Id(),
on Linux 
typedef int Id;

so will Id() return 0 or just a random number?

http://codereview.chromium.org/79020/diff/1/39#newcode700
Line 700: stub = stub;  // Touch variable to make compiler happy.
eyes!

http://codereview.chromium.org/79020/diff/1/41
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/79020/diff/1/41#newcode658
Line 658: // Notify the plugin that its parameters have changed.
nit: is this comment really necessary?  I think it's obvious by the function
name

[Amanda Walker, 2009-04-16 23:53:46]

Overall lg, a few nits and some suggestions about PlatformCanvas methods.

http://codereview.chromium.org/79020/diff/1/21
File chrome/plugin/npobject_proxy.cc (right):

http://codereview.chromium.org/79020/diff/1/21#newcode306
Line 306: return obj->_class->invalidate(obj);
returning a value from a void-valued function seems odd, even if it evidently
works.

http://codereview.chromium.org/79020/diff/1/39
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/79020/diff/1/39#newcode765
Line 765: // platforms.  It's just the final drawToHDC that kills us.
This should be very easily portable--drawToContext() should work on the Mac, and
the Gtk platform canvas should have something similar (or maybe, now that we
have gfx::NativeDrawingContext, we should just rename drawToHDC drawToContext
and make the method name & signature cross-platform like most of the rest of
PlatformCanvas{Win,Mac,Gtk}.

http://codereview.chromium.org/79020/diff/1/39#newcode784
Line 784: #endif
similar comment as above--PlatformCanvas should already have everything we need
here.

[Evan Martin, 2009-04-21 02:01:10]

New patch (two smaller commits of pieces of this).

http://codereview.chromium.org/79020/diff/1/20
File chrome/plugin/chrome_plugin_host.cc (right):

http://codereview.chromium.org/79020/diff/1/20#newcode422
Line 422: handler = handler;
Here and elsewhere, I just wanted to double-check with you that we're not
leaking, since we call "new" but never assign it to a variable.

http://codereview.chromium.org/79020/diff/1/31
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/79020/diff/1/31#newcode64
Line 64: CommandLine compiler_warning = parsed_command_line;
In this particular case, it's just because the above hasn't been ported yet.  So
it'll be fixed when we remove the NOTIMPLEMENTED().

http://codereview.chromium.org/79020/diff/1/36
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/79020/diff/1/36#newcode61
Line 61: NOTIMPLEMENTED();
I'd prefer to leave it in until I've checked whether we can do the same sort of
optimization on Linux.  (NOTIMPLEMENTEDs help track down "this is code that you
ought to look at again".)

http://codereview.chromium.org/79020/diff/1/38
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/79020/diff/1/38#newcode1877
Line 1877: #if defined(OS_WIN) || defined(OS_LINUX)
I think I will remove this change before committing, so that we don't crash on
Linux (since the rest of the plugin code doesn't yet work).

http://codereview.chromium.org/79020/diff/1/39
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/79020/diff/1/39#newcode68
Line 68: multibyte_response_expected_ = (existing_stream != 0);
It's because of intptr_t.  I could static_cast if you like?

http://codereview.chromium.org/79020/diff/1/39#newcode205
Line 205: TransportDIB::Id(),
0.  Pretty sure of this, and I checked with a local compile just to be sure.

http://codereview.chromium.org/79020/diff/1/41
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/79020/diff/1/41#newcode658
Line 658: // Notify the plugin that its parameters have changed.
I had added it for contrast to the call below.  (Two calls side by side both
talkign about geometry -- this one tells the plugin, the next one tells the
window hosting the plugin.)

[jam, 2009-04-21 05:04:45]

http://codereview.chromium.org/79020/diff/1/31
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/79020/diff/1/31#newcode64
Line 64: CommandLine compiler_warning = parsed_command_line;
On 2009/04/21 02:01:10, Evan Martin wrote:
> In this particular case, it's just because the above hasn't been ported yet. 
So
> it'll be fixed when we remove the NOTIMPLEMENTED().

what's the compiler warning?

http://codereview.chromium.org/79020/diff/1/36
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/79020/diff/1/36#newcode61
Line 61: NOTIMPLEMENTED();
On 2009/04/21 02:01:10, Evan Martin wrote:
> I'd prefer to leave it in until I've checked whether we can do the same sort
of
> optimization on Linux.  (NOTIMPLEMENTEDs help track down "this is code that
you
> ought to look at again".)

this code is about to be taken out by me to fix a hang, and is moving to
render_widget_host_view_win.cc...

[Evan Martin, 2009-04-21 17:10:25]

http://codereview.chromium.org/79020/diff/1/31
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/79020/diff/1/31#newcode64
Line 64: CommandLine compiler_warning = parsed_command_line;
On 2009/04/21 05:04:45, John Abd-El-Malek wrote:
> On 2009/04/21 02:01:10, Evan Martin wrote:
> > In this particular case, it's just because the above hasn't been ported yet.

> So
> > it'll be fixed when we remove the NOTIMPLEMENTED().
> 
> what's the compiler warning?

That parsed_command_line is unused.  (It's declared at the top.)

Another option is to move the parsed_command_line variable into the #if WIN
block.

http://codereview.chromium.org/79020/diff/1/36
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/79020/diff/1/36#newcode61
Line 61: NOTIMPLEMENTED();
On 2009/04/21 05:04:45, John Abd-El-Malek wrote:
> On 2009/04/21 02:01:10, Evan Martin wrote:
> > I'd prefer to leave it in until I've checked whether we can do the same sort
> of
> > optimization on Linux.  (NOTIMPLEMENTEDs help track down "this is code that
> you
> > ought to look at again".)
> 
> this code is about to be taken out by me to fix a hang, and is moving to
> render_widget_host_view_win.cc...

Ok, I'll resync after your commit.

[jam, 2009-04-21 20:54:53]

http://codereview.chromium.org/79020/diff/1/31
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/79020/diff/1/31#newcode64
Line 64: CommandLine compiler_warning = parsed_command_line;
On 2009/04/21 17:10:25, Evan Martin wrote:
> On 2009/04/21 05:04:45, John Abd-El-Malek wrote:
> > On 2009/04/21 02:01:10, Evan Martin wrote:
> > > In this particular case, it's just because the above hasn't been ported
yet.
> 
> > So
> > > it'll be fixed when we remove the NOTIMPLEMENTED().
> > 
> > what's the compiler warning?
> 
> That parsed_command_line is unused.  (It's declared at the top.)
> 
> Another option is to move the parsed_command_line variable into the #if WIN
> block.

yeah I think that's a better option..

[Evan Martin, 2009-04-22 00:30:09]

OK, new patch up

[Evan Martin, 2009-04-23 01:10:17]

ping.  resynced with trunk.

[jam, 2009-04-23 01:33:45]

lgtm