linux: add windowless plugin plumbing

linux: add windowless plugin plumbing

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

[Antoine Labour, 2009-07-21 06:52:24]

Evan: please review
John, Amanda: FYI

That gets basic windowless flash working (e.g. www.dailymotion.com, or
http://www.communitymx.com/content/source/E5141/wmodetrans.htm )

Some stuff left to do:
- BackgroundChanged should really be implemented. Right now it defaults to true,
meaning we may draw transparent plugins more often than necessary
- There are a bunch of copies going on. See if we can save a few.
- We can get into a state where the plugin requested an invalidate but never got
the draw and further invalidates just get queued. Need to investigate more, I'm
surprised it doesn't happen on Windows as well.

[jam, 2009-07-21 17:05:37]

lgtm

On 2009/07/21 06:52:24, Antoine Labour wrote:
> Evan: please review
> John, Amanda: FYI
> 
> That gets basic windowless flash working (e.g. www.dailymotion.com, or
> http://www.communitymx.com/content/source/E5141/wmodetrans.htm )
> 
> Some stuff left to do:
> - BackgroundChanged should really be implemented. Right now it defaults to
true,
> meaning we may draw transparent plugins more often than necessary

Note: this is a 'must', not a 'should', otherwise performance will be very slow
when scrolling.  The good thing is that it should be simple to check.  There
could be more (memory) efficient ways of doing this, Evan once suggested keeping
a hash of the bits.

> - There are a bunch of copies going on. See if we can save a few.

The reason there are so many copies is to keep painting fully asynchronous (but
as per above, that won't happen till BackgroundChanged is implemented). 
Surprisingly enough even for me, with all the copies and allocations, scrolling
is much faster this way.

> - We can get into a state where the plugin requested an invalidate but never
got
> the draw and further invalidates just get queued. Need to investigate more,
I'm
> surprised it doesn't happen on Windows as well.

This is pretty tricky, and we've had to be very careful on Windows to avoid this
situation (it's come up before).  I'm pretty sure this can't happen on Windows
anymore.  Here are some previous fixes:
http://codereview.chromium.org/11492/diff/244/74,
http://codereview.chromium.org/10792, http://codereview.chromium.org/6257

[Antoine Labour, 2009-07-22 00:01:55]

On Tue, Jul 21, 2009 at 5:05 PM, <jam@chromium.org> wrote:
> lgtm
>
> On 2009/07/21 06:52:24, Antoine Labour wrote:
>>
>> Evan: please review
>> John, Amanda: FYI
>
>> That gets basic windowless flash working (e.g. www.dailymotion.com, or
>> http://www.communitymx.com/content/source/E5141/wmodetrans.htm )
>
>> Some stuff left to do:
>> - BackgroundChanged should really be implemented. Right now it
>
> defaults to true,
>>
>> meaning we may draw transparent plugins more often than necessary
>
> Note: this is a 'must', not a 'should', otherwise performance will be
> very slow when scrolling. =A0The good thing is that it should be simple t=
o
> check. =A0There could be more (memory) efficient ways of doing this, Evan
> once suggested keeping a hash of the bits.

Hashing could be a good idea, but since we may be looking at a
different rect every time, we'd end up having to look at all the
pixels, so I'm not sure it's a win.

>
>> - There are a bunch of copies going on. See if we can save a few.
>
> The reason there are so many copies is to keep painting fully
> asynchronous (but as per above, that won't happen till BackgroundChanged
> is implemented). =A0Surprisingly enough even for me, with all the copies
> and allocations, scrolling is much faster this way.
>
>> - We can get into a state where the plugin requested an invalidate but
>
> never got
>>
>> the draw and further invalidates just get queued. Need to investigate
>
> more, I'm
>>
>> surprised it doesn't happen on Windows as well.
>
> This is pretty tricky, and we've had to be very careful on Windows to
> avoid this situation (it's come up before). =A0I'm pretty sure this can't
> happen on Windows anymore. =A0Here are some previous fixes:
> http://codereview.chromium.org/11492/diff/244/74,
> http://codereview.chromium.org/10792,
> http://codereview.chromium.org/6257

Actually I could reproduce it on Windows, there's a race condition
between the plugin process's InvalidateRect (plugin-initiated) and the
renderer's UpdateGeometry (e.g. if scrolling). It's not necessarily
easy to get into, but it could happen in some cases if the plugin's
paint is slow and you scroll at the same time. It's easy to reproduce
with a debugger, I can show you.


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

[Evan Martin, 2009-07-22 00:07:26]

LGTM

sad we don't share more code with mac...

http://codereview.chromium.org/159128/diff/8/10
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/159128/diff/8/10#newcode581
Line 581: int width = delegate_->GetRect().width();
Typically we use capitalization like "GetRect()" to indicate "this is expensive,
and shouldn't be used like an accessor".  Is that not the case here?

http://codereview.chromium.org/159128/diff/8/14
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/159128/diff/8/14#newcode377
Line 377: std::max(1, width), std::max(1, height), sys_visual->depth);
80 cols

http://codereview.chromium.org/159128/diff/8/14#newcode386
Line 386: // Draw a rectangle on a Cairo surface.
comment no longer correct ("surface")

[Antoine Labour, 2009-07-22 00:28:08]

http://codereview.chromium.org/159128/diff/8/10
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/159128/diff/8/10#newcode581
Line 581: int width = delegate_->GetRect().width();
On 2009/07/22 00:07:26, Evan Martin wrote:
> Typically we use capitalization like "GetRect()" to indicate "this is
expensive,
> and shouldn't be used like an accessor".  Is that not the case here?

This is just calling WebPluginDelegate::GetRect() which is virtual (which
explains the naming I assume) but in practice here is
WebPluginDelegateImpl::GetRect which is trivial.

http://codereview.chromium.org/159128/diff/8/14
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/159128/diff/8/14#newcode377
Line 377: std::max(1, width), std::max(1, height), sys_visual->depth);
On 2009/07/22 00:07:26, Evan Martin wrote:
> 80 cols

Done.

http://codereview.chromium.org/159128/diff/8/14#newcode386
Line 386: // Draw a rectangle on a Cairo surface.
On 2009/07/22 00:07:26, Evan Martin wrote:
> comment no longer correct ("surface")

Done.

[Amanda Walker, 2009-07-22 02:22:13]

We could probably share a bunch of this code among all three platforms if we
added a few convenience functions (notably "copy a rectangle of bits from
one gfx::NativeDrawingContext to another")...
--Amanda


On Tue, Jul 21, 2009 at 8:07 PM, <evan@chromium.org> wrote:

> LGTM
>
> sad we don't share more code with mac...
>
>
> http://codereview.chromium.org/159128/diff/8/10
> File chrome/plugin/webplugin_proxy.cc (right):
>
> http://codereview.chromium.org/159128/diff/8/10#newcode581
> Line 581: int width = delegate_->GetRect().width();
> Typically we use capitalization like "GetRect()" to indicate "this is
> expensive, and shouldn't be used like an accessor".  Is that not the
> case here?
>
> http://codereview.chromium.org/159128/diff/8/14
> File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):
>
> http://codereview.chromium.org/159128/diff/8/14#newcode377
> Line 377: std::max(1, width), std::max(1, height), sys_visual->depth);
> 80 cols
>
> http://codereview.chromium.org/159128/diff/8/14#newcode386
> Line 386: // Draw a rectangle on a Cairo surface.
> comment no longer correct ("surface")
>
>
> http://codereview.chromium.org/159128
>



-- 
"Portability is generally the result of advance planning rather than trench
warfare involving #ifdef" -- Henry Spencer (1992)

[Antoine Labour, 2009-07-22 02:27:20]

On Wed, Jul 22, 2009 at 2:21 AM, Amanda Walker<amanda@chromium.org> wrote:
> We could probably share a bunch of this code among all three platforms if=
 we
> added a few convenience functions (notably "copy a rectangle of bits from
> one gfx::NativeDrawingContext to another")...
> --Amanda

Yeah, I thought I'd do that as a second pass. Of course every single
one of these blits is different ever so slightly so I know I'll break
some platforms I can't test as I try to refactor. Starting with a
clean state will help.

Antoine

>
> On Tue, Jul 21, 2009 at 8:07 PM, <evan@chromium.org> wrote:
>>
>> LGTM
>>
>> sad we don't share more code with mac...
>>
>>
>> http://codereview.chromium.org/159128/diff/8/10
>> File chrome/plugin/webplugin_proxy.cc (right):
>>
>> http://codereview.chromium.org/159128/diff/8/10#newcode581
>> Line 581: int width =3D delegate_->GetRect().width();
>> Typically we use capitalization like "GetRect()" to indicate "this is
>> expensive, and shouldn't be used like an accessor". =A0Is that not the
>> case here?
>>
>> http://codereview.chromium.org/159128/diff/8/14
>> File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):
>>
>> http://codereview.chromium.org/159128/diff/8/14#newcode377
>> Line 377: std::max(1, width), std::max(1, height), sys_visual->depth);
>> 80 cols
>>
>> http://codereview.chromium.org/159128/diff/8/14#newcode386
>> Line 386: // Draw a rectangle on a Cairo surface.
>> comment no longer correct ("surface")
>>
>> http://codereview.chromium.org/159128
>
>
>
> --
> "Portability is generally the result of advance planning rather than tren=
ch
> warfare involving #ifdef" -- Henry Spencer (1992)
>
>



--=20
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

[Antoine Labour, 2009-07-22 04:53:07]

On Wed, Jul 22, 2009 at 2:26 AM, Antoine Labour<piman@google.com> wrote:
> On Wed, Jul 22, 2009 at 2:21 AM, Amanda Walker<amanda@chromium.org> wrote:
>> We could probably share a bunch of this code among all three platforms if we
>> added a few convenience functions (notably "copy a rectangle of bits from
>> one gfx::NativeDrawingContext to another")...
>> --Amanda
>
> Yeah, I thought I'd do that as a second pass. Of course every single
> one of these blits is different ever so slightly so I know I'll break
> some platforms I can't test as I try to refactor. Starting with a
> clean state will help.
>
> Antoine

http://codereview.chromium.org/159190

Not ready for review yet (I need to check that it compiles and run on
mac & win - and I don't have a mac), but fyi.
While going through the code to refactor things, I noticed what seems
to be some bugs on mac (e.g. reverting the blits, or blitting a
surface onto itself). This fixes one, but I think there's another one
in webplugin_proxy.cc

Antoine

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

[Amanda Walker, 2009-07-22 12:56:13]

Yes--that patch was a snapshot of a work in progress to unblock you :-). I
have a followup patch I've been working on that fixes a bunch of those, as
well as some geometry problems so that events work better.  I'm hoping to
land it very shortly.
--Amanda

On Wed, Jul 22, 2009 at 12:52 AM, Antoine Labour <piman@google.com> wrote:

> On Wed, Jul 22, 2009 at 2:26 AM, Antoine Labour<piman@google.com> wrote:
> > On Wed, Jul 22, 2009 at 2:21 AM, Amanda Walker<amanda@chromium.org>
> wrote:
> >> We could probably share a bunch of this code among all three platforms
> if we
> >> added a few convenience functions (notably "copy a rectangle of bits
> from
> >> one gfx::NativeDrawingContext to another")...
> >> --Amanda
> >
> > Yeah, I thought I'd do that as a second pass. Of course every single
> > one of these blits is different ever so slightly so I know I'll break
> > some platforms I can't test as I try to refactor. Starting with a
> > clean state will help.
> >
> > Antoine
>
> http://codereview.chromium.org/159190
>
> Not ready for review yet (I need to check that it compiles and run on
> mac & win - and I don't have a mac), but fyi.
> While going through the code to refactor things, I noticed what seems
> to be some bugs on mac (e.g. reverting the blits, or blitting a
> surface onto itself). This fixes one, but I think there's another one
> in webplugin_proxy.cc
>
> Antoine
>
> >>> http://codereview.chromium.org/159128
>



-- 
"Portability is generally the result of advance planning rather than trench
warfare involving #ifdef" -- Henry Spencer (1992)