linux: fix windowless Flash when scrolling

linux: fix windowless Flash when scrolling

Apparently on linux windowless Flash needs a paint after a NPP_SetWindow, otherwise it cancels its timer to do InvalidateRect.

Also in this change: WebKit calls WebPluginImpl::setFrameRect at each paint, even if the rects haven't changed. That used to always send a message to the plugin process, which is unnecessary (that case is always a no-op in the WebPluginDelegateImpl), so I fixed that too.

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

[Evan Martin, 2009-08-05 17:32:08]

Again, I think John needs to be the reviewer of this.

http://codereview.chromium.org/159907/diff/1/3
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/159907/diff/1/3#newcode624
Line 624: plugin_->InvalidateRect(clip_rect_);
Why isn't this necessary for the initial paint?  Or is it just that you need it
for subsequent SetWindow()s?

Is this the reason the flash painting looks like it's lagged when you scroll
windows off and on the page quickly?

http://codereview.chromium.org/159907/diff/1/6
File webkit/glue/webplugin_impl.h (right):

http://codereview.chromium.org/159907/diff/1/6#newcode369
Line 369: gfx::Rect window_rect_;
docs on these (are they related to the variable above, or is that just missing
whitespace?)

[piman, 2009-08-06 00:26:56]

http://codereview.chromium.org/159907/diff/1/3
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/159907/diff/1/3#newcode624
Line 624: plugin_->InvalidateRect(clip_rect_);
On 2009/08/05 17:32:08, Evan Martin wrote:
> Why isn't this necessary for the initial paint?  Or is it just that you need
it
> for subsequent SetWindow()s?

When we initially create the plugin, we draw it at least once (the entire area
is considered invalidated), which is what kicks the invalidate timer in Flash.

> 
> Is this the reason the flash painting looks like it's lagged when you scroll
> windows off and on the page quickly?

I'm not sure which case you are referring to exactly... it could be ? This only
touches windowless plugins, so the lag when scrolling youtube for example won't
be affected.

http://codereview.chromium.org/159907/diff/1/6
File webkit/glue/webplugin_impl.h (right):

http://codereview.chromium.org/159907/diff/1/6#newcode369
Line 369: gfx::Rect window_rect_;
On 2009/08/05 17:32:08, Evan Martin wrote:
> docs on these (are they related to the variable above, or is that just missing
> whitespace?)

Done.

[piman, 2009-08-06 17:22:25]

On Tue, Aug 4, 2009 at 11:03 PM, <piman@chromium.org> wrote:
> Reviewers: Evan Martin, John Abd-El-Malek, Amanda Walker,
>
> Description:
> linux: fix windowless Flash when scrolling
>
> Apparently on linux windowless Flash needs a paint after a
> NPP_SetWindow, otherwise it cancels its timer to do InvalidateRect.
>
> Also in this change: WebKit calls WebPluginImpl::setFrameRect at each
> paint, even if the rects haven't changed. That used to always send a
> message to the plugin process, which is unnecessary (that case is always
> a no-op in the WebPluginDelegateImpl), so I fixed that too.
>
> BUG=3D18423
>
> Please review this at http://codereview.chromium.org/159907
>
> Affected files:
> =A0M chrome/plugin/webplugin_proxy.cc
> =A0M webkit/glue/plugins/webplugin_delegate_impl_gtk.cc
> =A0M webkit/glue/webplugin_delegate.h
> =A0M webkit/glue/webplugin_impl.h
> =A0M webkit/glue/webplugin_impl.cc
>
>
>

John: ping ?

Antoine

[jam, 2009-08-06 18:20:39]

http://codereview.chromium.org/159907/diff/3001/4004
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/159907/diff/3001/4004#newcode750
Line 750: if (first_geometry_update_ || window_rect != window_rect_ ||
This code is pretty fragile, we went through a lot of iterations to get it
right.  Sometimes 'cleanup' changes end up causing regressions here, so need to
be very careful.

We used to do a check if the window rects change, but then stopped in
http://src.chromium.org/viewvc/chrome?view=rev&revision=13822.  I realize the
code isn't the same before and after, so test with that bug at least on Windows
and make sure there are no regressions.  Also, note that this is an async
message so it's pretty cheap.

[piman, 2009-08-06 23:46:40]

On Thu, Aug 6, 2009 at 11:20 AM, <jam@chromium.org> wrote:
>
> http://codereview.chromium.org/159907/diff/3001/4004
> File webkit/glue/webplugin_impl.cc (right):
>
> http://codereview.chromium.org/159907/diff/3001/4004#newcode750
> Line 750: if (first_geometry_update_ || window_rect !=3D window_rect_ ||
> This code is pretty fragile, we went through a lot of iterations to get
> it right. =A0Sometimes 'cleanup' changes end up causing regressions here,
> so need to be very careful.
>
> We used to do a check if the window rects change, but then stopped in
> http://src.chromium.org/viewvc/chrome?view=3Drev&revision=3D13822. =A0I
> realize the code isn't the same before and after, so test with that bug
> at least on Windows and make sure there are no regressions. =A0Also, note
> that this is an async message so it's pretty cheap.

So I checked that bug on both linux and windows, and everything still
works as expected.
I verified that all the WebPluginDelegateImpl (all platforms including
mac) do a no-op on both the windowed and windowless paths when neither
the window_rect nor the clip_rect change.

The message is async, but there's still data to munge to generate the
message, and extra syscalls, and possibly context switches which are
expensive on single-proc, best to avoid if possible, since that
happens for every paint of every plugin.
It also make debugging on the plugin side a bit easier if you're
looking at scroll etc. because your BP won't hit unless it's a real
move.

If you think it's too risky, we can always separate that part of the change=
.


>
> http://codereview.chromium.org/159907
>

[jam, 2009-08-07 00:10:29]

On Thu, Aug 6, 2009 at 4:46 PM, Antoine Labour <piman@chromium.org> wrote:

> On Thu, Aug 6, 2009 at 11:20 AM, <jam@chromium.org> wrote:
> >
> > http://codereview.chromium.org/159907/diff/3001/4004
> > File webkit/glue/webplugin_impl.cc (right):
> >
> > http://codereview.chromium.org/159907/diff/3001/4004#newcode750
> > Line 750: if (first_geometry_update_ || window_rect != window_rect_ ||
> > This code is pretty fragile, we went through a lot of iterations to get
> > it right.  Sometimes 'cleanup' changes end up causing regressions here,
> > so need to be very careful.
> >
> > We used to do a check if the window rects change, but then stopped in
> > http://src.chromium.org/viewvc/chrome?view=rev&revision=13822.  I
> > realize the code isn't the same before and after, so test with that bug
> > at least on Windows and make sure there are no regressions.  Also, note
> > that this is an async message so it's pretty cheap.
>
> So I checked that bug on both linux and windows, and everything still
> works as expected.
> I verified that all the WebPluginDelegateImpl (all platforms including
> mac) do a no-op on both the windowed and windowless paths when neither
> the window_rect nor the clip_rect change.
>
> The message is async, but there's still data to munge to generate the
> message, and extra syscalls, and possibly context switches which are
> expensive on single-proc, best to avoid if possible, since that
> happens for every paint of every plugin.
> It also make debugging on the plugin side a bit easier if you're
> looking at scroll etc. because your BP won't hit unless it's a real
> move.
>
> If you think it's too risky, we can always separate that part of the
> change.


cool, if you verified it, then lgtm.  the depending code must has changed
since when it was needed.


>
>
>
> >
> > http://codereview.chromium.org/159907
> >
>