Improve scrolling performance when there are many windowed plugins in a page....

Improve scrolling performance when there are many windowed plugins in a page.

This works by parenting windowed plugins with an HWND that's hosted in the browser process, so that no synchronous cross process messages are used when scrolling. 
BUG=5428
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8239

[Peter Kasting, 2009-01-15 05:41:19]

How do we prevent a hung plugin from hanging the browser UI in general, then?  I
thought divorcing the HWND trees was the only way to make certain of this.  (I
realize that synchronous messages could result in this too, but only if we were
in a situation where we actually needed to send one.)

[darin (slow to review), 2009-01-15 06:12:06]

On 2009/01/15 05:41:19, pkasting wrote:
> How do we prevent a hung plugin from hanging the browser UI in general, then? 
I
> thought divorcing the HWND trees was the only way to make certain of this.

This CL doesn't affect that issue at all.  A faulty plugin can hose the entire
HWND hierarchy.  We have never had a solution to that problem.

[ananta, 2009-01-15 14:15:05]

If the issues occur because of Window messages like
WM_WINDOWPOSCHANGING/WM_WINDOWPOSCHANGED being sent to the plugin process, does
inserting a new parent window created by the browser in the window hierarchy
actually solve this issue fully?. The plugin window is still a child of that
window. Does it continue to receive any messages during the scroll operation?

-Ananta

On 2009/01/15 03:21:11, John Abd-El-Malek wrote:
>

[jam, 2009-01-15 21:44:17]

Well now the plugin window's position isn't changing, so it doesn't get a
WM_WINDOWPOSCHANGING/WM_WINDOWPOSCHANGED.  It's top left corner is always 0,0.

Testing this on embed3.html from the test shell tests shows a huge improvement.

[darin (slow to review), 2009-01-15 23:13:38]

LG

http://codereview.chromium.org/18082/diff/317/79
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/18082/diff/317/79#newcode353
Line 353: FilePath plugin_path, IPC::Message* reply_msg)
nit: no need to wrap this line

http://codereview.chromium.org/18082/diff/317/79#newcode392
Line 392: wcex.lpszClassName  = L"NativeWindowClassWrapper";
we usually prefix our class names with "Chrome_"

http://codereview.chromium.org/18082/diff/317/76
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/18082/diff/317/76#newcode138
Line 138: // to do expensive operations if we didn't actually move.
are you sure we don't need this to handle the case of exposing a plugin while
scrolling the page?

as i seem to recall, we would generate dummy setFrameRect calls during scrolling
to force the plugin to actually get re-positioned relative to the viewport.

see frameRectsChanged down below.  we can probably just modify that code to call
impl_->setFrameRect

[jam, 2009-01-16 01:20:13]

http://codereview.chromium.org/18082/diff/317/79
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/18082/diff/317/79#newcode353
Line 353: FilePath plugin_path, IPC::Message* reply_msg)
On 2009/01/15 23:13:38, darin wrote:
> nit: no need to wrap this line

Done.

http://codereview.chromium.org/18082/diff/317/79#newcode392
Line 392: wcex.lpszClassName  = L"NativeWindowClassWrapper";
On 2009/01/15 23:13:38, darin wrote:
> we usually prefix our class names with "Chrome_"

I was following what webplugin_delegate_impl.cc did (the window's class is
"NativeWindowClass").  I figure I'd add Wrapper around it to help debugging.

http://codereview.chromium.org/18082/diff/317/76
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/18082/diff/317/76#newcode138
Line 138: // to do expensive operations if we didn't actually move.
On 2009/01/15 23:13:38, darin wrote:
> are you sure we don't need this to handle the case of exposing a plugin while
> scrolling the page?
> 
> as i seem to recall, we would generate dummy setFrameRect calls during
scrolling
> to force the plugin to actually get re-positioned relative to the viewport.
> 
> see frameRectsChanged down below.  we can probably just modify that code to
call
> impl_->setFrameRect

good catch, I just tested on a page where the plugin was originally offscreen
(embed2.html) and then when i scroll to the bottom sometimes it doesn't show
now.  If I take this optimization out then it always works.

I'm not sure what you mean by modifying the code below in frameRectsChange?  It
already calls impl_->setFrameRect.

[jam, 2009-01-16 01:41:21]

http://codereview.chromium.org/18082/diff/317/76
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/18082/diff/317/76#newcode138
Line 138: // to do expensive operations if we didn't actually move.
strangely, I undid this change but the test page (embed2.html) still sometimes
fails when the plugin is originally off screen, so something else must be the
cause.. investigating.

[jam, 2009-01-16 20:12:19]

Updated cl.  The problem with offscreen plugins was caused by NPP_Write being
called before NPP_SetWindow.  I made WebPluginContainer to override setparent so
that we can call NPP_SetWindow right away.  This simplifies the code because we
can get rid of the force_geometry_update_ code.

[darin (slow to review), 2009-01-16 22:08:39]

http://codereview.chromium.org/18082/diff/317/79
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/18082/diff/317/79#newcode392
Line 392: wcex.lpszClassName  = L"NativeWindowClassWrapper";
OK

http://codereview.chromium.org/18082/diff/103/112
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/18082/diff/103/112#newcode91
Line 91: HWND /* window */)
nit: indentation

[darin (slow to review), 2009-01-16 22:08:45]

LGTM