Basic windowless plugins, try 2.

Basic windowless plugins, try 2.

In response to Dean's comment on http://codereview.chromium.org/39105,
I redid that patch to put the X munging into the plugin implementation.  This
won't work for multiproc, but it's near the correct place and many things will
need to be changed for multiproc.

[Evan Martin, 2009-03-11 01:07:16]

CC jam FYI, it's almost identical in implementation to the previous code but
some stuff has been shuffled around

[Dean McNamee, 2009-03-11 11:34:28]

This looks super good.  Does it all seem to work well?

http://codereview.chromium.org/42056/diff/1002/25
File skia/ext/bitmap_platform_device_linux.h (right):

http://codereview.chromium.org/42056/diff/1002/25#newcode88
Line 88: virtual cairo_surface_t* beginPlatformPaint() { return surface(); }
This gives us a cairo surface that's mapped to the same underlying bitmap as the
Skia right?

http://codereview.chromium.org/42056/diff/1002/32
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/42056/diff/1002/32#newcode189
Line 189: void EnsurePixmapForSize(int width, int height);
EnsurePixmapAtleastSize ?

http://codereview.chromium.org/42056/diff/1002/33
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/42056/diff/1002/33#newcode169
Line 169: NOTIMPLEMENTED();
Heh, I DEMAND YOU IMPLEMENT THIS NOW ;)

http://codereview.chromium.org/42056/diff/1002/33#newcode436
Line 436: gdk_drawable_unref(pixmap_);
maybe pixmap_ = NULL here, just to be paranoid and help catch a bug if anyone
modifies this code in the future

http://codereview.chromium.org/42056/diff/1002/33#newcode439
Line 439: // sys_visual is owned by gdk; we shouldn't free it.
|sys_visual| ?

http://codereview.chromium.org/42056/diff/1002/33#newcode443
Line 443: // XXX we leak the colormap.
We only need to create the colormap once right, even if we are creating multiple
pixmaps_?  Maybe you just need colormap_ and you can destroy it when the
instance goes down along with pixmap_ ?

http://codereview.chromium.org/42056/diff/1002/33#newcode452
Line 452: //
http://mxr.mozilla.org/firefox/source/layout/generic/nsObjectFrame.cpp#4222
Maybe name the function, because the line number is going to go stale as the
Mozilla code changes.

http://codereview.chromium.org/42056/diff/1002/33#newcode472
Line 472: // Copy the current image into the pixmap.
This comment seems to say exactly what the code does, but I am still a bit
confused.  We're copying the rectangle from the surface that webkit has given
us, onto the pixmap we are going to give the plugin, so that it can see / draw
alpha / etc over top of the background ?

http://codereview.chromium.org/42056/diff/1002/33#newcode489
Line 489: static StatsRate plugin_paint("Plugin.Paint");
Do you really want these StatsRates, I think Windows was planning to drop them
out

http://codereview.chromium.org/42056/diff/1002/33#newcode492
Line 492: DCHECK(err == NPERR_NO_ERROR);
DCHECK_EQ (for a better failure message) ?

http://codereview.chromium.org/42056/diff/1002/33#newcode495
Line 495: cairo = cairo_create(context);
Could you not have reused the cairo surface across the HandleEvent call?

http://codereview.chromium.org/42056/diff/1002/34
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/42056/diff/1002/34#newcode713
Line 713: gfx::NativeDrawingContext context = gc->platformContext();
Hm, can we not get this code to work the same across mac?

[Amanda Walker, 2009-03-11 13:22:16]

http://codereview.chromium.org/42056/diff/1002/34
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/42056/diff/1002/34#newcode713
Line 713: gfx::NativeDrawingContext context = gc->platformContext();
Yes, pending some cleanup of the mac graphicscontext code--this seems OK for
now.

[Evan Martin, 2009-03-12 23:41:19]

new upload

http://codereview.chromium.org/42056/diff/1002/25
File skia/ext/bitmap_platform_device_linux.h (right):

http://codereview.chromium.org/42056/diff/1002/25#newcode88
Line 88: virtual cairo_surface_t* beginPlatformPaint() { return surface(); }
On 2009/03/11 11:34:28, Dean McNamee wrote:
> This gives us a cairo surface that's mapped to the same underlying bitmap as
the
> Skia right?

Yes.  Added a comment.

http://codereview.chromium.org/42056/diff/1002/33
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/42056/diff/1002/33#newcode169
Line 169: NOTIMPLEMENTED();
On 2009/03/11 11:34:28, Dean McNamee wrote:
> Heh, I DEMAND YOU IMPLEMENT THIS NOW ;)

It's actually not so bad with Cairo, actually -- it supports rendering to
PDF/PS.  We just would need a Skia->Cairo conversion layer.  :~(

http://codereview.chromium.org/42056/diff/1002/33#newcode472
Line 472: // Copy the current image into the pixmap.
On 2009/03/11 11:34:28, Dean McNamee wrote:
> This comment seems to say exactly what the code does, but I am still a bit
> confused.  We're copying the rectangle from the surface that webkit has given
> us, onto the pixmap we are going to give the plugin, so that it can see / draw
> alpha / etc over top of the background ?

Yes.  Extended the comment.

http://codereview.chromium.org/42056/diff/1002/33#newcode489
Line 489: static StatsRate plugin_paint("Plugin.Paint");
On 2009/03/11 11:34:28, Dean McNamee wrote:
> Do you really want these StatsRates, I think Windows was planning to drop them
> out

Just trying to stay close to windows.

http://codereview.chromium.org/42056/diff/1002/33#newcode495
Line 495: cairo = cairo_create(context);
On 2009/03/11 11:34:28, Dean McNamee wrote:
> Could you not have reused the cairo surface across the HandleEvent call?

One has the pixmap as the source and draws to webkit; the other goes the other
way.  I'd have to reinitialize it.  I can look at performance more once it all
works.

[Dean McNamee, 2009-03-13 13:42:08]

LG!

http://codereview.chromium.org/42056/diff/60/3009
File skia/ext/platform_canvas_linux.h (right):

http://codereview.chromium.org/42056/diff/60/3009#newcode39
Line 39: // endPlatformPaint is a no-op; it is used for symmetry with Windows.
I don't see the point of mentioning plugins here, windows uses
beginPlatformPaint for other stuff, and I could see us doing it too if we wanted
to drop to cairo/pango or something.  Right now it makes it sound plugin
specific.

http://codereview.chromium.org/42056/diff/60/3015
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/42056/diff/60/3015#newcode532
Line 532: // TODO(evanm): we leak the entire "extra" struct, including the
colormap.
Isn't SetWindow called a lot (ie won't this leak be bad?)