Wire up windowless plugins. Mostly Mac related, some cross

Wire up windowless plugins.  Mostly Mac related, some cross
platform aspects.
BUG=10809
TEST=none


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

[Evan Martin, 2009-07-09 18:52:59]

stopping 'cause you need to sync :)

http://codereview.chromium.org/113637/diff/4001/4005
File chrome/common/transport_dib_mac.cc (left):

http://codereview.chromium.org/113637/diff/4001/4005#oldcode48
Line 48: return false;
surprised that compiled :)

http://codereview.chromium.org/113637/diff/4001/4009
File chrome/plugin/webplugin_delegate_stub.cc (right):

http://codereview.chromium.org/113637/diff/4001/4009#newcode139
Line 139: NOTIMPLEMENTED() << " need to figure out nativeview id business";
this is old, you need to sync

[Amanda Walker, 2009-07-09 19:05:43]

Yes, that's why I didn't push it out for review immediately :-).
Syncing up now and cleaning up some loose ends.

On Thu, Jul 9, 2009 at 2:52 PM, <evan@chromium.org> wrote:
> stopping 'cause you need to sync :)
>
>
> http://codereview.chromium.org/113637/diff/4001/4005
> File chrome/common/transport_dib_mac.cc (left):
>
> http://codereview.chromium.org/113637/diff/4001/4005#oldcode48
> Line 48: return false;
> surprised that compiled :)
>
> http://codereview.chromium.org/113637/diff/4001/4009
> File chrome/plugin/webplugin_delegate_stub.cc (right):
>
> http://codereview.chromium.org/113637/diff/4001/4009#newcode139
> Line 139: NOTIMPLEMENTED() << " need to figure out nativeview id
> business";
> this is old, you need to sync
>
> http://codereview.chromium.org/113637
>

[Amanda Walker, 2009-07-10 00:03:20]

Still a work in progress, but time to get some actual review eyeballs on it
while I make sure I haven't broken things under Windows.

[jam, 2009-07-10 00:16:48]

Shouldn't impact Windows (other than the obvious ifdef in
WebPluginDelegateProxy::CreateBitmap())

http://codereview.chromium.org/113637/diff/4025/5017
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/113637/diff/4025/5017#newcode127
Line 127: NewRunnableFunction(PluginCarbonEventLoop), kPluginUpdateIntervalMs);
I'm confused, why are there Carbon events on the IO thread?

[Evan Martin, 2009-07-10 01:16:05]

the stuff that affects linux lgtm

http://codereview.chromium.org/113637/diff/4025/5017
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/113637/diff/4025/5017#newcode33
Line 33: const int kPluginUpdateIntervalMs = 20; // 20ms = 50Hz
docs on this and function below

http://codereview.chromium.org/113637/diff/4025/5018
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/113637/diff/4025/5018#newcode524
Line 524: void WebPluginProxy::UpdateTransform() {
may wanna put a NOTIMPLEMENTED in here just in case

http://codereview.chromium.org/113637/diff/4025/5022
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/113637/diff/4025/5022#newcode1777
Line 1777: #if defined(OS_WIN)  // In-proc plugins aren't supported on Linux or
Mac.
I wonder if instead we should put the #ifdef inside the if statement and do a
NOTIMPLEMENTED on lin/mac with the contents of that comment.

http://codereview.chromium.org/113637/diff/4025/5023
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/113637/diff/4025/5023#newcode461
Line 461: #if 0
prolly don't want to commit with this here, right?

http://codereview.chromium.org/113637/diff/4025/5023#newcode499
Line 499: file_util::ReadFileToString(FilePath("/proc/sys/kernel/shmmax"),
&contents);
aw, sweet of you to think of us

http://codereview.chromium.org/113637/diff/4025/5023#newcode538
Line 538: }
prolly unnecessary

http://codereview.chromium.org/113637/diff/4025/5023#newcode726
Line 726: windowless_ = window == NULL;
Does this compile on Linux?  I should've put a comment in there, but
PluginWindowHandle is an integer on Linux, so the previous test sidestepped the
type mismatches.

http://codereview.chromium.org/113637/diff/4025/5025
File webkit/glue/plugins/plugin_host.cc (right):

http://codereview.chromium.org/113637/diff/4025/5025#newcode633
Line 633: // TODO: implement me
you can kill this now

http://codereview.chromium.org/113637/diff/4025/5025#newcode658
Line 658: // TODO: implement me
you can kill this now

http://codereview.chromium.org/113637/diff/4025/5027
File webkit/glue/plugins/webplugin_delegate_impl_mac.mm (left):

http://codereview.chromium.org/113637/diff/4025/5027#oldcode184
Line 184: if (windowless_)
Prolly better off leaving code like this in, since it'll make it easier to
unfork.  It'll just be always true on mac.

[Evan Martin, 2009-07-10 01:16:20]

+piman, master of plugins

[Antoine Labour, 2009-07-10 01:39:02]

http://codereview.chromium.org/113637/diff/4025/5017
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/113637/diff/4025/5017#newcode117
Line 117: }
Careful: this change moves the gtk_init code inside the kPluginStartupDialog
test.
Fortunately in the top-of-trunk version this code has moved (make sure you
update !)

http://codereview.chromium.org/113637/diff/4025/5023
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/113637/diff/4025/5023#newcode499
Line 499: file_util::ReadFileToString(FilePath("/proc/sys/kernel/shmmax"),
&contents);
On 2009/07/10 01:16:05, Evan Martin wrote:
> aw, sweet of you to think of us

+1 :)

http://codereview.chromium.org/113637/diff/4025/5023#newcode552
Line 552: BitBlt(background_hdc, rect.x()-plugin_rect_.x(),
rect.y()-plugin_rect_.y(),
maybe use offset_rect here since you computed these values already ?

http://codereview.chromium.org/113637/diff/4025/5023#newcode575
Line 575: rect.x()-plugin_rect_.x(), rect.y()-plugin_rect_.y(), SRCCOPY);
maybe use offset_rect here since you computed these values already ?

http://codereview.chromium.org/113637/diff/4025/5023#newcode726
Line 726: windowless_ = window == NULL;
On 2009/07/10 01:16:05, Evan Martin wrote:
> Does this compile on Linux?  I should've put a comment in there, but
> PluginWindowHandle is an integer on Linux, so the previous test sidestepped
the
> type mismatches.

Indeed, gfx::NativeViewFromId can't be called from the renderer on linux. I
think the previous version was fine, maybe add a window=NULL on mac, but that
should be done in the WebPluginDelegateImpl.

http://codereview.chromium.org/113637/diff/4025/5025
File webkit/glue/plugins/plugin_host.cc (right):

http://codereview.chromium.org/113637/diff/4025/5025#newcode637
Line 637: plugin->webplugin()->Invalidate();
This is overkill if the region is not the full plugin. Ideally we would extract
the proper rect and pass that to InvalidateRect, but that's platform dependent.
Can you add a TODO though ?

[Amanda Walker, 2009-07-10 16:34:44]

Addressed comments, took plugin_list_mac.mm out of the patch so that you have to
uncomment a line to enable plugins (since it still has problems with plugins
that try to invoke javascript, such as YouTube)

http://codereview.chromium.org/113637/diff/4025/5017
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/113637/diff/4025/5017#newcode33
Line 33: const int kPluginUpdateIntervalMs = 20; // 20ms = 50Hz
On 2009/07/10 01:16:05, Evan Martin wrote:
> docs on this and function below

Done.

http://codereview.chromium.org/113637/diff/4025/5017#newcode117
Line 117: }
Fixed by syncing :-).

http://codereview.chromium.org/113637/diff/4025/5017#newcode127
Line 127: NewRunnableFunction(PluginCarbonEventLoop), kPluginUpdateIntervalMs);
On 2009/07/10 00:16:48, John Abd-El-Malek wrote:
> I'm confused, why are there Carbon events on the IO thread?

Idle and timer events are delivered to the main thread, which uses a
MessageLoopForIO in the plugin process (since we don't want to service UI
related events), but the actual I/O happens on the PluginThread, unless I'm
misunderstanding something.  The Carbon event receiver needs to stay on the main
thread, but we could conceivably spin off yet another thread to run the message
loop, and wake up the main thread periodically.  That seemed a bit of overkill,
but I can do it if we need to.

http://codereview.chromium.org/113637/diff/4025/5018
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/113637/diff/4025/5018#newcode524
Line 524: void WebPluginProxy::UpdateTransform() {
Done.

http://codereview.chromium.org/113637/diff/4025/5022
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/113637/diff/4025/5022#newcode1777
Line 1777: #if defined(OS_WIN)  // In-proc plugins aren't supported on Linux or
Mac.
Done.

http://codereview.chromium.org/113637/diff/4025/5023
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/113637/diff/4025/5023#newcode461
Line 461: #if 0
Fixed.

http://codereview.chromium.org/113637/diff/4025/5023#newcode538
Line 538: }
On 2009/07/10 01:16:05, Evan Martin wrote:
> prolly unnecessary

Just keeping the prior logic intact, but I'll admit I haven't encountered this
condition in the field :-).  jam, can you chime in here?

http://codereview.chromium.org/113637/diff/4025/5023#newcode552
Line 552: BitBlt(background_hdc, rect.x()-plugin_rect_.x(),
rect.y()-plugin_rect_.y(),
Done.

http://codereview.chromium.org/113637/diff/4025/5023#newcode575
Line 575: rect.x()-plugin_rect_.x(), rect.y()-plugin_rect_.y(), SRCCOPY);
Done.

http://codereview.chromium.org/113637/diff/4025/5023#newcode726
Line 726: windowless_ = window == NULL;
Done.

http://codereview.chromium.org/113637/diff/4025/5025
File webkit/glue/plugins/plugin_host.cc (right):

http://codereview.chromium.org/113637/diff/4025/5025#newcode637
Line 637: plugin->webplugin()->Invalidate();
Done.

http://codereview.chromium.org/113637/diff/4025/5025#newcode658
Line 658: // TODO: implement me
Done.

http://codereview.chromium.org/113637/diff/4025/5027
File webkit/glue/plugins/webplugin_delegate_impl_mac.mm (left):

http://codereview.chromium.org/113637/diff/4025/5027#oldcode184
Line 184: if (windowless_)
Done.

[TVL, 2009-07-10 17:05:00]

http://codereview.chromium.org/113637/diff/5072/4052
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/113637/diff/5072/4052#newcode11
Line 11: #include "base/scoped_cftyperef.h"
doesn't the header already include this?

http://codereview.chromium.org/113637/diff/5072/4057
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/113637/diff/5072/4057#newcode424
Line 424: // so only do so if we've actually recreated the shared memory
bitmaps.
what happens when we can't mmap any more memory for this?  (since we use it for
a bunch of stuff)

http://codereview.chromium.org/113637/diff/5072/4060
File webkit/glue/plugins/webplugin_delegate_impl_mac.mm (right):

http://codereview.chromium.org/113637/diff/5072/4060#newcode50
Line 50: const int kFlashIdleThrottleDelayMs = 20;  // 20ms (50Hz)
does this need to match the carbon event one?  should they share a single
constant?

http://codereview.chromium.org/113637/diff/5072/4060#newcode150
Line 150: NSPoint windowOffset = {0.0, 0.0};
Why use this offset if it's zero, zero?  If this is really an offset, does the
bottom & right also need the offset incase this was non zero?

Don't we have a helper to just covert the gfc::Rect to a Rect? (since that what
the code below does?

[TVL, 2009-07-10 17:06:40]

http://codereview.chromium.org/113637/diff/5072/4057
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/113637/diff/5072/4057#newcode424
Line 424: // so only do so if we've actually recreated the shared memory
bitmaps.
On 2009/07/10 17:05:00, TVL wrote:
> what happens when we can't mmap any more memory for this?  (since we use it
for
> a bunch of stuff)

Just to clarify:

24+" monitor, full screen mode, plugin at full frame.  Between the renderer,
plugin host, etc. that's a lot of memory, seems like it could fail on us.

[Amanda Walker, 2009-07-10 17:16:54]

http://codereview.chromium.org/113637/diff/5072/4052
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/113637/diff/5072/4052#newcode11
Line 11: #include "base/scoped_cftyperef.h"
On 2009/07/10 17:05:00, TVL wrote:
> doesn't the header already include this?

So it does.  Fixed.

http://codereview.chromium.org/113637/diff/5072/4057
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/113637/diff/5072/4057#newcode424
Line 424: // so only do so if we've actually recreated the shared memory
bitmaps.
A 30" monitor full screen would be 16MB, which does not seem too likely to be a
problem.  But if we can't map the memory, we'll get a negative fd and
CreateBitmap will fail (resulting in a plugin that won't draw).

[Antoine Labour, 2009-07-10 18:28:43]

LGTM