Bug 18831 – [GTK] support windowless plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Benjamin Otte]

Firefox 3 NPAPI implements windowless plugin support. This is required to support transparent plugins and (at least for Mozilla) respect stacking order in a web page. Webkit should support it, too.
See https://bugzilla.mozilla.org/show_bug.cgi?id=137189 for the corresponding Mozilla bug. The only plugin supporting it so far is the Swfdec-Mozilla plugin in it's development version.

[Chris Toshok]

Moonlight also supports it, although at the moment it's rather firefox dependent (that should change in the next couple of days, though).

[Mark Renouf]

Flash 10 for Linux supports this (since July 2008, see: http://blogs.adobe.com/penguin.swf/2008/07/turkish_localization_also_wmod.html).

Any progress on this? I'm kind of surprised this isn't supported. Is it a Gtk thing?

[Bastien Nocera]

*** Bug 29297 has been marked as a duplicate of this bug. ***

[Bastien Nocera]

Breaks layering as seen in bug 29297

[Brian Tarricone]

Created attachment 42175 [details]
First mostly-broken patch

Ok, so I "implemented" windowless mode by merging in much of the Qt code to support this, and modifying it for gtk/gdk.  It doesn't really work so well.  This is my first foray into WebKit's code, so to be honest I don't really know what I'm doing.

For windowless plugins, it creates an offscreen X Pixmap and passes that to the plugin to draw on.  There are currently a few problems:

1.  The positioning of the plugin's contents (the XCopyArea() in PluginView::paint()) is wrong when the page is scrolled to something other than the origin.

2.  There are repaint/invalidation problems: sometimes areas don't get repainted, some areas seem to not get painted over when they should.

3.  Input handling is pretty messed up, probably at least partly related to #1.

4.  I'm not sure how to implement the difference between the transparent and opaque windowless modes (an XClearArea() before the XCopyArea() doesn't seem to work, but that's probably because of my other painting issues, and is likely sub-optimal anyway).

Any hints or guidance would be greatly appreciated.

Note:  I'm using this page:

http://www.communitymx.com/content/source/E5141/wmodenone.htm

as a nice test case.  The URL above is for windowed mode, and there are links to the right to pages for wmode=transparent and wmode=opaque.  To see the placement problem when the view is scrolled, the stock price chart view on finance.yahoo.com works well.

[Antonio Gomes]

cc'ing girish, who implement this support for the qt port.

[Mark Renouf]

Much appreciated! I will poke at this tomorrow and give you some feedback. You have a better handle on it than I do.

One thing to think about is performance, and whether this can be accelerated in any way (compositing?). I know windowless mode on FF/Linux is really resource intensive.

[Bastien Nocera]

Using client-side windows in the latest GTK+ should allow you to do this without doing the double-buffering yourself.

[Brian Tarricone]

Ok, I figured out (one of) the problems with my first patch: a stupid typo
exchanging an x coordinate for a y coordinate.

I've also switched to using cairo instead of gdk to do the drawing.  There are
still some problems (wmode=transparent has some drawing issues, though =opaque works fine), but it's pretty decent now.

I'll clean up and attach the patch in a bit.

Not sure about the approach of using client-side windows, especially since we don't know what toolkit the plugin is using (and we need to pass an X Drawable for the plugin to draw on).  Now that I look at them after the fact, the NPAPI docs on developer.mozilla.org actually specifically say to use an X pixmap for the plugin's drawing surface.  Not sure if that's normative or just informative, though.

Mark, not sure about performance... "unfortunately" I have a pretty speedy dual-core machine, so I wouldn't notice a slowdown without some benchmarking.  It looks *really* nice right now to me, anyway... no noticeable difference from windowed mode.

[Brian Tarricone]

Created attachment 42247 [details]
Second mostly-working patch

Ok, this one's a bit better.

wmode=opaque appears to work perfectly.

wmode=transparent works, but the background doesn't appear to get painted, and I'm not sure what to do about that.

The input handling code works, but could probably stand to be cleaned up a bit.

On a semi-related note, PluginView.cpp defaults m_isTransparent to false.  Mozilla's NPAPI docs say that the default for windowless plugins should be transparent.  Maybe this should be changed?

[Simon Hausmann]

Comment on attachment 42247 [details]
Second mostly-working patch


> -#if defined(Q_WS_X11) && ENABLE(NETSCAPE_PLUGIN_API)
> +#if (defined(XP_UNIX) || defined(Q_WS_X11)) && ENABLE(NETSCAPE_PLUGIN_API)

You can simplify this condition by replacing the use of Q_WS_X11 with XP_UNIX.

Given the existance of platforms like Qt for Embedded Linux or Android that are Unixy without X-Windows, XP_UNIX becomes a confusing term
as it really means a Unix platform that uses X-Windows. But for now XP_UNIX is the right one to use I would say.

[Girish Ramakrishnan]

The XSync's are not required. They are in the Qt code only because Flash and Qt use different connections. I guess this is not the case with the GTK/WebKit?

Also, you might want to test the testcases in WebCore/manual-tests/plugins/windowless.html.

[Brian Tarricone]

(In reply to comment #12)
> The XSync's are not required. They are in the Qt code only because Flash and Qt
> use different connections. I guess this is not the case with the GTK/WebKit?

Hmm, I don't think we can guarantee that in general.  Plugins need not use gtk, meaning they'd have their own X server connection.  I'm not sure if it's possible to actually find out if they're using the same connection, though.

> Also, you might want to test the testcases in
> WebCore/manual-tests/plugins/windowless.html.

Ah, great, I'll do that.

[Brian Tarricone]

Created attachment 42350 [details]
Take 3, mostly working

Ok, here's v3.  Changes from v2:

* Replace preprocessor checks for (XP_UNIX || Q_WS_X11) with just XP_UNIX, and ifdef out a few more sections that should be XP_UNIX only.
* Copy from gdk's backing store to fix transparent windowless mode for plugins that use a visual that doesn't include an alpha channel.
* Use the qt backend's method of figuring out the colormap/visual to use for the plugin's backing pixmap.

I left the XSync() stuff in for now, though as it stands it'll never get called during paint (since m_pluginDisplay == GDK_DISPLAY() always).  It might be useful later, or possibly it could get put into common code with the qt port.  In that vein, there are probably some things that could be made more generic and be shared with the qt port (like the visual/cmap selection code).

I ran the windowless plugin test page included in the webkit source, and it sorta works.  At least, it works just as well as the qt backend does: move is broken (old plugin location doesn't get invalidated, new location doesn't get repainted unless the plugin's contents change), animate is broken (plugin area picks up some text from outside the plugin area, other drawing issues).  But these problems are common to both the qt backend and my new gtk backend code.  I'm not yet sure if they're issues with the plugin backends, or need to be fixed in common code -- maybe both.

Anyhow, it's working decently well now.  I think I might be able to at least work on fixing the animation stuff, but I'm not sure how much more time I'll have to dedicate to fixing other issues.

[Benjamin Otte]

(In reply to comment #14)
> * Copy from gdk's backing store to fix transparent windowless mode for plugins
> that use a visual that doesn't include an alpha channel.
> 
This sentence reminded me of https://bugzilla.mozilla.org/show_bug.cgi?id=445250 - that bug includes some testcases that you might wanna check if you haven't yet.

[Brian Tarricone]

Unless I'm missing something, the moonlight guys only provide an XPI for Firefox.  I tried extracting the files and putting the .so files in /usr/lib/mozilla/plugins, but it doesn't appear to work.  I just get errors on console about it being unable to probe for browser type (despite the fact that my user agent string has "AppleWebKit" in it).  It also complains about failign to load libmoonplugin-webkitbridge.so (so I guess it *is* figuring out my platform?), which isn't included in the XPI anyway.

I didn't see references in that bug to any other test cases, aside from the communitymx.com one I've been using.

[Benjamin Otte]

I think the difference is that https://bugzilla.mozilla.org/attachment.cgi?id=329591 uses "style { opacity: 0.5 }" on the div containing the flash, and the default communitymx page doesn't do that.

[Brian Tarricone]

(In reply to comment #17)
> I think the difference is that
> https://bugzilla.mozilla.org/attachment.cgi?id=329591 uses "style { opacity:
> 0.5 }" on the div containing the flash, and the default communitymx page
> doesn't do that.

Ah, I see.  Well, from that it looks like Firefox is broken there: I can only see the bouncing ball when it's in the top left corner of the plugin window.  WebKit with my patch works great: I can see the purple star, and the ball moves around on top of it, but semi-transparent such that you can see the star underneath it as it moves.

[Gustavo Noronha (kov)]

Hey, awesome to see work on this! To draw official reviewers atention you should set the review flag to ? when submitting a patch, since we check the review queue from time to time! =)

[Brian Tarricone]

Created attachment 42620 [details]
gtk windowless v4

Changes from last patch:

* If needed, set the GdkColormap on the plugin's GdkPixmap wrapper (necessary if the plugin does not use gtk).
* Don't bail on visual/colormap selection if xrender>=0.5 isn't found unless depth is also 32.
* Add ChangeLog entry.

I'm feeling decently good about this.  It's not perfect, but it seems to work at least as well as the windowless support for Qt.  I'd like to get at least this much checked in if possible and then handle fixes/improvements as they become necessary.

There's also some work that can be done to avoid some code duplication between the Qt and gtk backends for this, which is probably best done in a future patch.

[Kenneth Rohde Christiansen]

(In reply to comment #20)
> Created an attachment (id=42620) [details]
> gtk windowless v4
> 
> Changes from last patch:
> 
> * If needed, set the GdkColormap on the plugin's GdkPixmap wrapper (necessary
> if the plugin does not use gtk).
> * Don't bail on visual/colormap selection if xrender>=0.5 isn't found unless
> depth is also 32.
> * Add ChangeLog entry.
> 
> I'm feeling decently good about this.  It's not perfect, but it seems to work
> at least as well as the windowless support for Qt.  I'd like to get at least
> this much checked in if possible and then handle fixes/improvements as they
> become necessary.
> 
> There's also some work that can be done to avoid some code duplication between
> the Qt and gtk backends for this, which is probably best done in a future
> patch.

The patch looks good to me, knowing that you will work further on it. It has some coding style violations thought:

+        GtkWidget *widget = m_parentFrame->view()->hostWindow()->platformPageClient();
+        GdkDrawable *gdkBackingStore = NULL;


please run the check-webkit-style script.

[Brian Tarricone]

Created attachment 42765 [details]
gtk windowless v5

Same as v4 patch, but  with coding style fixes.

Odd, check-webkit-style --git-commit=... --verbose=5 didn't pick anything up.

[Girish Ramakrishnan]

Looks good to me.

You might also be interested in subscribing to 31232

[Gustavo Noronha (kov)]

Comment on attachment 42765 [details]
gtk windowless v5

> -#if defined(XP_UNIX) || defined(Q_WS_X11)
> +#if defined(XP_UNIX)
>          bool m_needsXEmbed;
>  #endif

I am wondering about these instances of this change in which we used to have both XP_UNIX, and Q_WS_X11. I wonder if the Qt people had a reason for this? Kenneth? Girish? =)
  
>  #include <gdk/gdkx.h>
> +#define Bool int
> +#define Status int
> +#include <X11/extensions/Xrender.h>

What are these for?

> +
> +    //if (m_windowRect == oldWindowRect && m_clipRect == oldClipRect)
> +    //    return;
> +

We don't usually accept commented code into the tree, unless we plan to enable it soon, but then it would need a comment with a TODO mark.

> +#if defined(XP_UNIX)
> +    if (!m_isWindowed && m_windowRect.size() != oldWindowRect.size()) {
> +        GtkWidget* parentWindow = m_parentFrame->view()->hostWindow()->platformPageClient();

We usually only initialize variables when they are first used, so I suggest moving this to just before XCreatePixmap, or you may consider losing the variable. It will make the line a bit longer, but not too long for WebKit's standards.

> +        if (m_drawable)
> +            XFreePixmap(GDK_DISPLAY(), m_drawable);
> +
> +        m_drawable = XCreatePixmap(GDK_DISPLAY(), GDK_DRAWABLE_XID(parentWindow->window),
> +                                   m_windowRect.width(), m_windowRect.height(),
> +                                   ((NPSetWindowCallbackStruct*)m_npWindow.ws_info)->depth);
> +        XSync(GDK_DISPLAY(), False); // make sure that the server knows about the Drawable
> +    }

Excuse my ignorance of X programming, but why do we need a sync here?

> +    // in order to move/resize the plugin window at the same time as the

A nit - use a capital I there =).

> +        if (gdkBackingStore) {
> +            XCopyArea(GDK_DISPLAY(), GDK_DRAWABLE_XID(gdkBackingStore), m_drawable, gc,
> +                      m_windowRect.x() + exposedRect.x() - xoff,
> +                      m_windowRect.y() + exposedRect.y() - yoff,
> +                      exposedRect.width(), exposedRect.height(),
> +                      exposedRect.x(), exposedRect.y());
> +        } else {
> +            // no valid backing store; clear to the background color
> +            XFillRectangle(GDK_DISPLAY(), m_drawable, gc,
> +                           exposedRect.x(), exposedRect.y(),
> +                           exposedRect.width(), exposedRect.height());
> +        }

Both are single line calls, no need for the brackets.

> +    exposeEvent.width = exposedRect.x() + exposedRect.width(); // flash bug? it thinks width is the right in transparent mode
> +    exposeEvent.height = exposedRect.y() + exposedRect.height(); // flash bug? it thinks height is the bottom in transparent mode

I don't understand these comments. You mean it thinks height is the absolute y position? Will this break other plugins?

> +    // passed Drawable without first doing gdk_pixmap_lookup().  If we create
> +    // the GdkPixmap here before Adobe does, then Adobe's _foreign_new() will
> +    // trigger a gdk warning.  (Ideally, gdk would first do a lookup before
> +    // creating a new one, like it does for Windows, but oh well, nothing we
> +    // can do about that.)

I recommend taking the parens out here, they do not add anything.

> +    //IntPoint p = static_cast<FrameView*>(parent())->contentsToWindow(IntPoint(event->pageX(), event->pageY()));

Please drop.

> +    // On Unix, only call plugin if it's full-page or windowed
> +    if (m_mode != NP_FULL && m_mode != NP_EMBED)
> +        return;

What does 'call plugin' mean? Calling setNPWindow?

>      IntRect r(rect->left, rect->top, rect->right - rect->left, rect->bottom - rect->top);
> -    invalidateRect(r);
> +    //invalidateRect(r);

Drop commented line.

> +    int nvi;
> +    XVisualInfo templ;
> +    templ.screen  = gdk_screen_get_number(gdk_screen_get_default());
> +    templ.depth   = depth;
> +    templ.c_class = TrueColor;
> +    XVisualInfo* xvi = XGetVisualInfo(GDK_DISPLAY(), VisualScreenMask | VisualDepthMask | VisualClassMask, &templ, &nvi);

Can we get better names for these variables? visualInfo, for instance, would be more in line with our coding style.

Thanks for your work on this!

[Kenneth Rohde Christiansen]

(In reply to comment #24)
> (From update of attachment 42765 [details])
> > -#if defined(XP_UNIX) || defined(Q_WS_X11)
> > +#if defined(XP_UNIX)
> >          bool m_needsXEmbed;
> >  #endif
> 
> I am wondering about these instances of this change in which we used to have
> both XP_UNIX, and Q_WS_X11. I wonder if the Qt people had a reason for this?
> Kenneth? Girish? =)

Simon had a comment about that, that if XP_UNIX is true, Q_WS_X11 is as well. 

> >  #include <gdk/gdkx.h>
> > +#define Bool int
> > +#define Status int
> > +#include <X11/extensions/Xrender.h>
> 
> What are these for?

The Bool and Status seems like redefines of some X typedefs. Girish are you using XRender for the windowless support?

> We don't usually accept commented code into the tree, unless we plan to enable
> it soon, but then it would need a comment with a TODO mark.

I agree

> > +        if (gdkBackingStore) {
> > +            XCopyArea(GDK_DISPLAY(), GDK_DRAWABLE_XID(gdkBackingStore), m_drawable, gc,
> > +                      m_windowRect.x() + exposedRect.x() - xoff,
> > +                      m_windowRect.y() + exposedRect.y() - yoff,
> > +                      exposedRect.width(), exposedRect.height(),
> > +                      exposedRect.x(), exposedRect.y());
> > +        } else {
> > +            // no valid backing store; clear to the background color
> > +            XFillRectangle(GDK_DISPLAY(), m_drawable, gc,
> > +                           exposedRect.x(), exposedRect.y(),
> > +                           exposedRect.width(), exposedRect.height());
> > +        }
> 
> Both are single line calls, no need for the brackets.

It spans multiply lines, thus the coding style guide says to use braces.

> > +    // On Unix, only call plugin if it's full-page or windowed
> > +    if (m_mode != NP_FULL && m_mode != NP_EMBED)
> > +        return;
> 
> What does 'call plugin' mean? Calling setNPWindow?

the setNPWindow is supposed to be called on all changes to geometry (size or position) according to the spec.

[Benjamin Otte]

(In reply to comment #24)
> > +        if (m_drawable)
> > +            XFreePixmap(GDK_DISPLAY(), m_drawable);
> > +
> > +        m_drawable = XCreatePixmap(GDK_DISPLAY(), GDK_DRAWABLE_XID(parentWindow->window),
> > +                                   m_windowRect.width(), m_windowRect.height(),
> > +                                   ((NPSetWindowCallbackStruct*)m_npWindow.ws_info)->depth);
> > +        XSync(GDK_DISPLAY(), False); // make sure that the server knows about the Drawable
> > +    }
> 
> Excuse my ignorance of X programming, but why do we need a sync here?
>
It ensures that all previous operations have completed on the server. In this particular case, as the comment says, it ensures that the server has created the pixmap.
This is important when there are multiple X connections (aka displays) in use that can access this resource. In that case the other display could try to do an operation on that resource while the server doesn't know about it. That would cause an error, and due to the gorgeous error handling of X, would cause an abort of the application.

Now that's the theory. In practice, I suspect it's copied from the Qt port which does indeed use two different displays and Brian forgot to delete it when porting to gtk.
 
> > +    exposeEvent.width = exposedRect.x() + exposedRect.width(); // flash bug? it thinks width is the right in transparent mode
> > +    exposeEvent.height = exposedRect.y() + exposedRect.height(); // flash bug? it thinks height is the bottom in transparent mode
> 
> I don't understand these comments. You mean it thinks height is the absolute y
> position? Will this break other plugins?
>
I'd like a proof that Mozilla does this the same way. Otherwise I suspect it shadows a different bug.
Also, I know Swfdec doesn't treat it this way.

> I recommend taking the parens out here, they do not add anything.
> 
I like it when people add () to function names as that makes it very clear it's talking about a function (in code comments as well as email or irc). YMMV of course.

[Gustavo Noronha (kov)]

(In reply to comment #26)
> (In reply to comment #24)
> Now that's the theory. In practice, I suspect it's copied from the Qt port
> which does indeed use two different displays and Brian forgot to delete it when
> porting to gtk.

Right, thanks.
 
> > I recommend taking the parens out here, they do not add anything.
> > 
> I like it when people add () to function names as that makes it very clear it's
> talking about a function (in code comments as well as email or irc). YMMV of
> course.

Oh, sorry for the confusion. I do agree with you there, I am talking about the parens surrounding the last sentence. I think they are not necessary.

[Brian Tarricone]

>>  #include <gdk/gdkx.h>
>> +#define Bool int
>> +#define Status int
>> +#include <X11/extensions/Xrender.h>

> What are these for?

See getVisualAndColormap().  It's mostly copied from the Qt backend.  When a 32-bit visual is requested, we use XRender to find the most appropriate visual (that is, a visual with an alpha channel).  I'm not sure what the deal is with the defines: apparently *somewhere* a lot of the defines in the Xlib headers get undefined, including Bool and Status.  Xrender.h is not compilable without those defines.  (You'll also note elsewhere in the code that numbers are used instead of the constants for FocusIn, FocusOut, ButtonPress, etc., as it appears someone undefs those as well for some reason.  This behavior is present in the Qt-related file as well.)

>> +    exposeEvent.width = exposedRect.x() + exposedRect.width(); // flash bug? it thinks width is the right in transparent mode
>> +    exposeEvent.height = exposedRect.y() + exposedRect.height(); // flash bug? it thinks height is the bottom in transparent mode
>
> I don't understand these comments. You mean it thinks height is the absolute y
> position? Will this break other plugins?

This was also copied over from Qt's windowless plugin support; I have no evidence for this behavior besides that.  It shouldn't (in theory) break any other plugins; it can only make the width and height of the exposed region larger than it needs to be (which is sub-optimal, of course, but not broken).  IMHO any plugin that isn't properly clipping the expose event's rect to its available drawing area is fundamentally broken anyway.  I'll test Adobe Flash 10 (x86_64 beta) without the hack and see, but I'd recommend leaving it in regardless of my findings, unless Girish knows differently (since I assume he's the one who put it in the Qt windowless support).

>> +    // On Unix, only call plugin if it's full-page or windowed
>> +    if (m_mode != NP_FULL && m_mode != NP_EMBED)
>> +        return;
>
> What does 'call plugin' mean? Calling setNPWindow?

I don't know; copied from the Qt backend.

>> > +        if (m_drawable)
>> > +            XFreePixmap(GDK_DISPLAY(), m_drawable);
>> > +
>> > +        m_drawable = XCreatePixmap(GDK_DISPLAY(), GDK_DRAWABLE_XID(parentWindow->window),
>> > +                                   m_windowRect.width(), m_windowRect.height(),
>> > +                                   ((NPSetWindowCallbackStruct*)m_npWindow.ws_info)->depth);
>> > +        XSync(GDK_DISPLAY(), False); // make sure that the server knows about the Drawable
>> > +    }
>> 
>> Excuse my ignorance of X programming, but why do we need a sync here?
>>
> It ensures that all previous operations have completed on the server. In this
> particular case, as the comment says, it ensures that the server has created
> the pixmap.
> This is important when there are multiple X connections (aka displays) in use
> that can access this resource. In that case the other display could try to do
> an operation on that resource while the server doesn't know about it. That
> would cause an error, and due to the gorgeous error handling of X, would cause
> an abort of the application.
>
> Now that's the theory. In practice, I suspect it's copied from the Qt port
>which does indeed use two different displays and Brian forgot to delete it when
>porting to gtk.

Yes and no.  It is indeed copied from the Qt backend, and is necessary for the reason you mention.  It's not any less necessary for the gtk backend because we can't know that a windowless plugin is also using gdk/gtk and is sharing the same X connection.  If it's using a different X connection (which would be the case if the plugin is using raw xlib, xt, qt, or... well, anything but gtk), then the XSync() is required.

Now what *is* questionable is the m_pluginDisplay and getPluginDisplay() stuff (and the conditional XSync() in ::paint()).  In the Qt backend, we do know that some plugins use gtk, and so they will use a different X connection, and we can "guess" that they're using gtk by dlopen()ing libgtk and trying to fetch its default display.  If that succeeds (in the Qt backend), then we know at least one plugin is using gtk and has a different X connection open, so keeping an extra Display* around (m_pluginDisplay) and using getPluginDisplay() is necessary.  For the gtk backend, this may or may not be necessary, but, regardless, the current implementation of it is somewhat useless (but does no harm).  So I'd be willing to take it out in the hopes of later either replacing it with code that does something useful, or just doing nothing (because we can't always figure out the Display* that the plugin is using).  Regardless, though, the commented-upon use of the XSync() after the XCreatePixmap() is correct.

The other requested changes sound fine to me; I'll work up a new patch tomorrow (5.30am is already far too late for me to be awake!).

[Kenneth Rohde Christiansen]

(In reply to comment #28)
> >>  #include <gdk/gdkx.h>
> >> +#define Bool int
> >> +#define Status int
> >> +#include <X11/extensions/Xrender.h>
> 
> > What are these for?
> 
> See getVisualAndColormap().  It's mostly copied from the Qt backend.  When a
> 32-bit visual is requested, we use XRender to find the most appropriate visual
> (that is, a visual with an alpha channel).  I'm not sure what the deal is with
> the defines: apparently *somewhere* a lot of the defines in the Xlib headers
> get undefined, including Bool and Status.  Xrender.h is not compilable without
> those defines.  (You'll also note elsewhere in the code that numbers are used
> instead of the constants for FocusIn, FocusOut, ButtonPress, etc., as it
> appears someone undefs those as well for some reason.  This behavior is present
> in the Qt-related file as well.)

Seems like a good idea to add a comment then
> 
> >> +    // On Unix, only call plugin if it's full-page or windowed
> >> +    if (m_mode != NP_FULL && m_mode != NP_EMBED)
> >> +        return;
> >
> > What does 'call plugin' mean? Calling setNPWindow?

Qt has issues with Flash as it is not being able to ref the GtkPlug, so I guess Gtk doesn't need this particular piece of code.

[Brian Tarricone]

(In reply to comment #23)
> Looks good to me.
> 
> You might also be interested in subscribing to 31232

Hmm, I probably won't look into it.  Adobe Flash 10 is windowless and that's good enough for me.  Composited windowed mode sounds like a bit of an unnecessary hack to me.  Maybe I'm wrong about that, but either way I don't think I care to do the work.

[Girish Ramakrishnan]

(In reply to comment #28)
> >>  #include <gdk/gdkx.h>
> >> +#define Bool int
> >> +#define Status int
> >> +#include <X11/extensions/Xrender.h>
> 
> > What are these for?
> 
> See getVisualAndColormap().  It's mostly copied from the Qt backend.  When a
> 32-bit visual is requested, we use XRender to find the most appropriate visual
> (that is, a visual with an alpha channel).  I'm not sure what the deal is with
> the defines: apparently *somewhere* a lot of the defines in the Xlib headers
> get undefined, including Bool and Status. 

Qt undefines them since they conflict with Qt has a type named Status and event enums named FocusIn, FocusOut and all. Also, iirc, JSC also has a Status enum.

> 
> >> +    exposeEvent.width = exposedRect.x() + exposedRect.width(); // flash bug? it thinks width is the right in transparent mode
> >> +    exposeEvent.height = exposedRect.y() + exposedRect.height(); // flash bug? it thinks height is the bottom in transparent mode
> >
> > I don't understand these comments. You mean it thinks height is the absolute y
> > position? Will this break other plugins?
> 

That's right. Flash thinks that height parameter is the bottom and width parameter is the right. As Brian said, this can cause no harm to other plugin except that we end up painting a little more than necessary.

> 
> >> +    // On Unix, only call plugin if it's full-page or windowed
> >> +    if (m_mode != NP_FULL && m_mode != NP_EMBED)
> >> +        return;
> >
> > What does 'call plugin' mean? Calling setNPWindow?
> 
> I don't know; copied from the Qt backend.
> 

We should change this to "call plugin's setwindow"

[Mark Renouf]

@ Comment #30

> Hmm, I probably won't look into it.  Adobe Flash 10 is windowless and that's
> good enough for me.  Composited windowed mode sounds like a bit of an
> unnecessary hack to me.  Maybe I'm wrong about that, but either way I don't
> think I care to do the work.

I've done some experiments trying to get flash to render into an ARGB visual but I was probably doing it naively. This has gone past my knowledge level, but am I correct in assuming proper compositing isn't possible unless flash (or any other plugin) makes an alpha channel available in it's output?

And, just to be sure, does flashplayer's wmode="trans" setting work as expected?

[Brian Tarricone]

(In reply to comment #32)

> And, just to be sure, does flashplayer's wmode="trans" setting work as
> expected?

Yep.

[Mark Renouf]

I just tested this applied to trunk and get a crash on the communitymx test page. Loads of details below, let me know if there's anything else I can provide. The system is Ubuntu 9.10, x86, without any major customizations. Desktop effects are enabled.

$ svn info
Path: .
URL: http://svn.webkit.org/repository/webkit/trunk
Repository Root: http://svn.webkit.org/repository/webkit
Repository UUID: 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Revision: 50747
Node Kind: directory
Schedule: normal
Last Changed Author: [e-mail]
Last Changed Rev: 50747
Last Changed Date: 2009-11-10 13:57:34 -0500 (Tue, 10 Nov 2009)

Load URL:
    www.communitymx.com/content/source/E5141/wmodetrans.htm

Shortly after the page begins to load, I get a XError and Segfault.


****** Console Output ******

$ Programs/GtkLauncher 
** (GtkLauncher:15359): DEBUG: NP_Initialize
** (GtkLauncher:15359): DEBUG: NP_Initialize succeeded
** (GtkLauncher:15359): DEBUG: NP_Initialize
** (GtkLauncher:15359): DEBUG: NP_Initialize succeeded
** (GtkLauncher:15359): DEBUG: NP_Initialize
** (GtkLauncher:15359): DEBUG: NP_Initialize succeeded
** (GtkLauncher:15359): DEBUG: NP_Initialize
** (GtkLauncher:15359): DEBUG: NP_Initialize succeeded

(GtkLauncher:15359): Gtk-CRITICAL **: gtk_widget_size_allocate: assertion `GTK_IS_WIDGET (widget)' failed

(GtkLauncher:15359): Gtk-CRITICAL **: gtk_widget_size_allocate: assertion `GTK_IS_WIDGET (widget)' failed

(GtkLauncher:15359): Gtk-CRITICAL **: gtk_widget_size_allocate: assertion `GTK_IS_WIDGET (widget)' failed
The program 'GtkLauncher' received an X Window System error.
This probably reflects a bug in the program.

The error was 'BadValue (integer parameter out of range for operation)'.
  (Details: serial 32806 error_code 2 request_code 139 minor_code 3)
  (Note to programmers: normally, X errors are reported asynchronously;
   that is, you will receive the error a while after causing it.
   To debug your program, run it with the --sync command line
   option to change this behavior. You can then get a meaningful
   backtrace from your debugger if you break on the gdk_x_error() function.)

******* Backtrace from GDB ********

Program received signal SIGSEGV, Segmentation fault.
0xb6a56d1d in pthread_mutex_lock () from /lib/tls/i686/cmov/libpthread.so.0
(gdb) bt
#0  0xb6a56d1d in pthread_mutex_lock () from /lib/tls/i686/cmov/libpthread.so.0
#1  0xb1730d42 in ?? ()
   from /usr/lib/firefox/plugins/flashplugin-alternative.so
#2  0xb177ef92 in ?? ()
   from /usr/lib/firefox/plugins/flashplugin-alternative.so
#3  0xb15ffa4d in ?? ()
   from /usr/lib/firefox/plugins/flashplugin-alternative.so
#4  0xb15e90d0 in ?? ()
   from /usr/lib/firefox/plugins/flashplugin-alternative.so
#5  0xb693a05f in ?? () from /lib/tls/i686/cmov/libc.so.6
#6  0xb693a0cf in exit () from /lib/tls/i686/cmov/libc.so.6
#7  0xb6e73ec1 in gdk_x_error (display=0x8068138, error=0xbfffd62c)
    at /build/buildd/gtk+2.0-2.18.3/gdk/x11/gdkmain-x11.c:470
#8  0xb541a839 in _XError (dpy=0x8068138, rep=0x8469b28)
    at ../../src/XlibInt.c:2924
#9  0xb5420e9f in process_responses (dpy=0x8068138, 
    wait_for_first_event=<value optimized out>, current_error=0xbfffd75c, 
    current_request=32807) at ../../src/xcb_io.c:207
#10 0xb5421526 in _XReply (dpy=0x8068138, rep=0xbfffd790, extra=0, discard=1)
    at ../../src/xcb_io.c:457
#11 0xb54151a7 in XSync (dpy=0x8068138, discard=0) at ../../src/Sync.c:48
#12 0xb5415335 in _XSyncFunction (dpy=0x8068138) at ../../src/Synchro.c:37
#13 0xb52b1730 in XShmPutImage () from /usr/lib/libXext.so.6
#14 0xb6e67f9e in gdk_x11_draw_image (drawable=0x80eeec0, gc=0x84798c0, 
    image=0x8488068, xsrc=11, ysrc=46, xdest=0, ydest=0, width=200, height=200)
    at /build/buildd/gtk+2.0-2.18.3/gdk/x11/gdkdrawable-x11.c:847
#15 0xb6e31888 in IA__gdk_draw_image (drawable=0x80eeec0, gc=0x84798c0, 
    image=0x8488068, xsrc=11, ysrc=46, xdest=0, ydest=0, width=200, height=200)
    at /build/buildd/gtk+2.0-2.18.3/gdk/gdkdraw.c:726
#16 0xb6e31888 in IA__gdk_draw_image (drawable=0x848e098, gc=0x84798c0, 
    image=0x8488068, xsrc=11, ysrc=46, xdest=0, ydest=0, width=200, height=200)
    at /build/buildd/gtk+2.0-2.18.3/gdk/gdkdraw.c:726
#17 0xb15fef0f in ?? ()
   from /usr/lib/firefox/plugins/flashplugin-alternative.so
#18 0xb15f2b18 in ?? ()
   from /usr/lib/firefox/plugins/flashplugin-alternative.so
#19 0xb15e9110 in ?? ()
   from /usr/lib/firefox/plugins/flashplugin-alternative.so
#20 0xb15edbd4 in ?? ()
   from /usr/lib/firefox/plugins/flashplugin-alternative.so
#21 0xb7b827a0 in WebCore::PluginView::dispatchNPEvent(_XEvent&) ()
   from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#22 0xb7b837d8 in WebCore::PluginView::paint(WebCore::GraphicsContext*, WebCore::IntRect const&) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#23 0xb79bada8 in WebCore::RenderWidget::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#24 0xb79130bd in WebCore::InlineBox::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#25 0xb791430a in WebCore::InlineFlowBox::paint(WebCore::RenderObject::PaintInfo---Type <return> to continue, or q <return> to quit---
&, int, int) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#26 0xb79bd9a0 in WebCore::RootInlineBox::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#27 0xb797685f in WebCore::RenderLineBoxList::paint(WebCore::RenderBoxModelObject*, WebCore::RenderObject::PaintInfo&, int, int) const ()
   from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#28 0xb7927d93 in WebCore::RenderBlock::paintContents(WebCore::RenderObject::PaintInfo&, int, int) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#29 0xb792d0b5 in WebCore::RenderBlock::paintObject(WebCore::RenderObject::PaintInfo&, int, int) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#30 0xb7926457 in WebCore::RenderBlock::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#31 0xb79741ff in WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, WebCore::PaintRestriction, WebCore::RenderObject*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int)
    () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#32 0xb7973a39 in WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, WebCore::PaintRestriction, WebCore::RenderObject*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int)
    () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#33 0xb7973a39 in WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, WebCore::PaintRestriction, WebCore::RenderObject*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int)
    () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#34 0xb79742e1 in WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::IntRect const&, WebCore::PaintRestriction, WebCore::RenderObject*) ()
   from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#35 0xb786d8e8 in WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&) ()
   from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#36 0xb78af696 in WebCore::ScrollView::paint(WebCore::GraphicsContext*, WebCore::IntRect const&) () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#37 0xb73bc08a in webkit_web_view_expose_event(_GtkWidget*, _GdkEventExpose*)
    () from /home/mark/Research/WebKit/.libs/libwebkit-1.0.so.2
#38 0xb6fea474 in _gtk_marshal_BOOLEAN__BOXED (closure=0x807f388, 
    return_value=0xbfffec34, n_param_values=2, param_values=0x8469368, 
    invocation_hint=0xbfffec20, marshal_data=0xb73bbf20)
    at /build/buildd/gtk+2.0-2.18.3/gtk/gtkmarshalers.c:84
#39 0xb6b376f9 in g_type_class_meta_marshal (closure=0x807f388, 
    return_value=0xbfffec34, n_param_values=2, param_values=0x8469368, 
    invocation_hint=0xbfffec20, marshal_data=0xc8)
    at /build/buildd/glib2.0-2.22.2/gobject/gclosure.c:878
#40 0xb6b39072 in IA__g_closure_invoke (closure=0x807f388, 
---Type <return> to continue, or q <return> to quit---
    return_value=0xbfffec34, n_param_values=2, param_values=0x8469368, 
    invocation_hint=0xbfffec20)
    at /build/buildd/glib2.0-2.22.2/gobject/gclosure.c:767
#41 0xb6b4e49e in signal_emit_unlocked_R (node=<value optimized out>, 
    detail=<value optimized out>, instance=0x8098000, 
    emission_return=0xbfffed7c, instance_and_params=0x8469368)
    at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:3285
#42 0xb6b4f9b8 in IA__g_signal_emit_valist (instance=0x8098000, signal_id=41, 
    detail=0, 
    var_args=0xbfffede0 "\034\356\377\277\364\257\352\266\b\356\377\277\364\237&\267") at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:2990
#43 0xb6b4ffb6 in IA__g_signal_emit (instance=0x8098000, signal_id=41, 
    detail=0) at /build/buildd/glib2.0-2.22.2/gobject/gsignal.c:3037
#44 0xb710696e in gtk_widget_event_internal (widget=<value optimized out>, 
    event=0xbfffeee8) at /build/buildd/gtk+2.0-2.18.3/gtk/gtkwidget.c:4767
#45 0xb6fe4190 in IA__gtk_main_do_event (event=0xbfffeee8)
    at /build/buildd/gtk+2.0-2.18.3/gtk/gtkmain.c:1571
#46 0xb6e571d4 in _gdk_window_process_updates_recurse (window=0x807bf38, 
    expose_region=0x8496300)
    at /build/buildd/gtk+2.0-2.18.3/gdk/gdkwindow.c:5061
#47 0xb6e7a734 in _gdk_windowing_window_process_updates_recurse (
    window=0x807bf38, region=0x8496300)
    at /build/buildd/gtk+2.0-2.18.3/gdk/x11/gdkwindow-x11.c:5566
#48 0xb6e4e87f in gdk_window_process_updates_internal (window=0x807bf38)
    at /build/buildd/gtk+2.0-2.18.3/gdk/gdkwindow.c:5220
#49 0xb6e5083f in IA__gdk_window_process_all_updates ()
    at /build/buildd/gtk+2.0-2.18.3/gdk/gdkwindow.c:5328
#50 0xb6e508bb in gdk_window_update_idle (data=0x0)
    at /build/buildd/gtk+2.0-2.18.3/gdk/gdkwindow.c:4954
#51 0xb6e2cf78 in gdk_threads_dispatch (data=0x8420930)
    at /build/buildd/gtk+2.0-2.18.3/gdk/gdk.c:506
#52 0xb6a9f0f1 in g_idle_dispatch (source=0x8498af8, callback=0xb040b020, 
    user_data=0x8420930) at /build/buildd/glib2.0-2.22.2/glib/gmain.c:4065
#53 0xb6aa0e78 in g_main_dispatch (context=0x807d7a0)
    at /build/buildd/glib2.0-2.22.2/glib/gmain.c:1960
#54 IA__g_main_context_dispatch (context=0x807d7a0)
    at /build/buildd/glib2.0-2.22.2/glib/gmain.c:2513
#55 0xb6aa4720 in g_main_context_iterate (context=0x807d7a0, 
    block=<value optimized out>, dispatch=1, self=0x8062690)
    at /build/buildd/glib2.0-2.22.2/glib/gmain.c:2591
#56 0xb6aa4b8f in IA__g_main_loop_run (loop=0x8465cc8)
    at /build/buildd/glib2.0-2.22.2/glib/gmain.c:2799
#57 0xb6fe4419 in IA__gtk_main ()
    at /build/buildd/gtk+2.0-2.18.3/gtk/gtkmain.c:1218
#58 0x08049e6d in main ()



Additional system information:


$ uname -a 
Linux crashpad 2.6.31-14-generic-pae #48-Ubuntu SMP Fri Oct 16 15:22:42 UTC 2009 i686 GNU/Linux

$ dpkg-query -l libgtk2.0*
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Cfg-files/Unpacked/Failed-cfg/Half-inst/trig-aWait/Trig-pend
|/ Err?=(noq
ne)/Reinst-required (Status,Err: uppercase=bad)
||/ Name  q         Version        Description
+++-==============-==============-============================================
ii  libgtk2.0-0    2.18.3-1       The GTK+ graphical user interface library
ii  libgtk2.0-0-db 2.18.3-1       The GTK+ libraries and debugging symbols
ii  libgtk2.0-bin  2.18.3-1       The programs for the GTK+ graphical user int
ii  libgtk2.0-cil  2.12.9-1       CLI binding for the GTK+ toolkit 2.12
ii  libgtk2.0-comm 2.18.3-1       Common files for the GTK+ graphical user int
ii  libgtk2.0-dev  2.18.3-1       Development files for the GTK+ library
ii  libgtk2.0-doc  2.18.3-1       Documentation for the GTK+ graphical user in

$ Xorg -version

X.Org X Server 1.6.4
Release Date: 2009-9-27
X Protocol Version 11, Revision 0
Build Operating System: Linux 2.6.24-23-server i686 Ubuntu
Current Operating System: Linux crashpad 2.6.31-14-generic-pae #48-Ubuntu SMP Fri Oct 16 15:22:42 UTC 2009 i686
Kernel command line: root=UUID=0a46f67d-395d-4573-8a8f-49b22f34b718 ro quiet splash 
Build Date: 26 October 2009  05:15:02PM
xorg-server 2:1.6.4-2ubuntu4 (buildd@) 
	Before reporting problems, check http://wiki.x.org
	to make sure that you have the latest version.

$ xdpyinfo
name of display:    :0.0
version number:    11.0
vendor string:    The X.Org Foundation
vendor release number:    10604000
X.Org version: 1.6.4
maximum request size:  16777212 bytes
motion buffer size:  256
bitmap unit, bit order, padding:    32, LSBFirst, 32
image byte order:    LSBFirst
number of supported pixmap formats:    7
supported pixmap formats:
    depth 1, bits_per_pixel 1, scanline_pad 32
    depth 4, bits_per_pixel 8, scanline_pad 32
    depth 8, bits_per_pixel 8, scanline_pad 32
    depth 15, bits_per_pixel 16, scanline_pad 32
    depth 16, bits_per_pixel 16, scanline_pad 32
    depth 24, bits_per_pixel 32, scanline_pad 32
    depth 32, bits_per_pixel 32, scanline_pad 32
keycode range:    minimum 8, maximum 255
focus:  window 0x4e00005, revert to Parent
number of extensions:    27
    BIG-REQUESTS
    Composite
    DAMAGE
    DOUBLE-BUFFER
    DPMS
    DRI2
    GLX
    Generic Event Extension
    MIT-SCREEN-SAVER
    MIT-SHM
    RANDR
    RECORD
    RENDER
    SECURITY
    SGI-GLX
    SHAPE
    SYNC
    X-Resource
    XC-MISC
    XFIXES
    XFree86-DGA
    XFree86-VidModeExtension
    XINERAMA
    XInputExtension
    XKEYBOARD
    XTEST
    XVideo
default screen number:    0
number of screens:    1

screen #0:
  dimensions:    1920x1200 pixels (508x317 millimeters)
  resolution:    96x96 dots per inch
  depths (7):    24, 1, 4, 8, 15, 16, 32
  root window id:    0x122
  depth of root window:    24 planes
  number of colormaps:    minimum 1, maximum 1
  default colormap:    0x20
  default number of colormap cells:    256
  preallocated pixels:    black 0, white 16777215
  options:    backing-store NO, save-unders NO
  largest cursor:    64x64
  current input event mask:    0x7a803f
    KeyPressMask             KeyReleaseMask           ButtonPressMask          
    ButtonReleaseMask        EnterWindowMask          LeaveWindowMask          
    ExposureMask             StructureNotifyMask      SubstructureNotifyMask   
    SubstructureRedirectMask FocusChangeMask          PropertyChangeMask       
  number of visuals:    72
  default visual id:  0x21
  visual:
    visual id:    0x21
    class:    TrueColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff0000, 0xff00, 0xff
    significant bits in color specification:    8 bits

[Brian Tarricone]

What's flashplugin-alternative?

[Gustavo Noronha (kov)]

(In reply to comment #35)
> What's flashplugin-alternative?

That's Debian's alternatives system. It allows you to have several packages that do the same thing, and choose the system's default. It uses symlinks to do that. So you have stuff such as a /usr/bin/editor, which is an alternative symlink to one of the installed editors:

root@goiaba ~# update-alternatives --display editor
editor - manual mode
 link currently points to /usr/bin/emacs23
/bin/ed - priority -100
 slave editor.1.gz: /usr/share/man/man1/ed.1.gz
/bin/nano - priority 40
 slave editor.1.gz: /usr/share/man/man1/nano.1.gz
[...]

This means that may be adobe flash, swfdec, or gnash. I'm guessing it's adobe's flash plugin.

[Simon Hausmann]

(In reply to comment #24)
> (From update of attachment 42765 [details])
> > -#if defined(XP_UNIX) || defined(Q_WS_X11)
> > +#if defined(XP_UNIX)
> >          bool m_needsXEmbed;
> >  #endif
> 
> I am wondering about these instances of this change in which we used to have
> both XP_UNIX, and Q_WS_X11. I wonder if the Qt people had a reason for this?
> Kenneth? Girish? =)

The above #ifdef was indeed bogus, but there were other cases where we used #if defined(XP_UNIX) && defined(Q_WS_X11) to guard code relating to windowless plugins that should only be enabled on Qt/X11 to avoid breaking the Gtk code.

With this effort to add windowless plugin support to the Gtk port some of these #ifdefs collapse to a single #if defined(XP_UNIX). Brian's change is correct AFAICS.

[Mark Renouf]

Any chance this patch can get updated? I'm getting some merge conflicts against the latest tree. And is there a plan to accept this? I need to know how long I might need to maintain a custom build, since this feature is needed for a product I am building.

Thanks!

[Brian Tarricone]

Sorry, I've been completely swamped lately (work+vacation... the latter of which just makes me more swamped now).  I'll try to update the patch and cover the review items within the next couple weeks.

[Gustavo Noronha (kov)]

(In reply to comment #38)
> Any chance this patch can get updated? I'm getting some merge conflicts against
> the latest tree. And is there a plan to accept this? I need to know how long I
> might need to maintain a custom build, since this feature is needed for a
> product I am building.

It will be accepted as long as the patch is as close to being correct as we can get it to, there are no style issues, and it passes the currently available tests. If it is important for you, maybe you would like to contribute helping fix the issues that were raised? =)

[Mark Renouf]

Yes, I certainly would! I attempted to apply the patch to the current tree but I do not feel confident enough in my understanding to merge the conflicts myself. If a clean patch can be produced I'd be happy to help in tidying up whatever changes remain, as well as further testing (I experienced some crashes the last time I tried it).

[Alex Alexander]

Created attachment 44682 [details]
v5 patch modified to apply on git-svn-id: http://svn.webkit.org/repository/webkit/trunk@51980 268f45cc-cd09-0410-ab3c-d52691b4dbfc (git commit 98b137e9b87d583cb67d8fb83a80960774d0bd61)

Trasparency works.

for example in:
http://www.contra.gr/Soccer/Hellas/Superleague/AEK/257844.html

there's a flash ad under the photo at the left of the main text.

without the patch, a gray box covers half the text, with the patch you can view the text.

however, even with the patch the flash box is there, meaning you can't select the text underneath it (flash gets all the events there).

also note that the communitymx test page doesn't crash here.

[Mark Renouf]

Excellent! I've applied and tested the new merged patch and I can confirm it resolves the previous crash I was experiencing on the communitymx[1] site. I've got it on a local git branch now and if I have time over next few days I will address the remaining issues to get it accepted.

[1] http://www.communitymx.com/content/source/E5141/wmodetrans.htm

[Alex Alexander]

Note that I removed the Changelog part from the patch I attached (there was no need to have that just for a testing patch).

Whoever provides the final patch please make sure you add that info into it :)

[Brian Tarricone]

Created attachment 47162 [details]
gtk windowless v6

Finally found some time to get back to this.

Changes from v5:

* Applies cleanly to git master as of today.
* I think I've addressed all the style issues and questions in the comments; if not, please let me know.
* Don't use GDK when painting the Pixmap to the window after the plugin has painted to it.  The API for creating a foreign GdkPixmap is kinda broken IMO, and it's just as easy to create a cairo_surface_t from the X Pixmap.
* When in transparent mode AND the Pixmap passed to the plugin has a 32bpp depth, clear the expose region of the Pixmap to fully transparent before passing it to the plugin.  Without this, you end up with junk in the Pixmap anywhere where the plugin paints anything with transparency.
* Use the root window when creating the Pixmap instead of GDK_WINDOW_XID() on the parent widget.  XCreatePixmap() just uses the window to figure out which screen it's on, and GDK_WINDOW_XID() will force the GdkWindow to create a backing native Window on GTK+ 2.18 and above.

I left in the hack regarding the expose rect in based on Girish's agreement that it can do no harm and is possibly necessary for Adobe Flash.

[Brian Tarricone]

(In reply to comment #45)
> Created an attachment (id=47162) [details]

> * When in transparent mode AND the Pixmap passed to the plugin has a 32bpp
> depth, clear the expose region of the Pixmap to fully transparent before
> passing it to the plugin.  Without this, you end up with junk in the Pixmap
> anywhere where the plugin paints anything with transparency.

BTW, Girish, I suspect it is a bug in the Qt windowless backend that it doesn't do this.  I imagine it's possible that no one has tested with a windowless, transparent plugin that can deal with a 32bit visual before.

[WebKit Review Bot]

Attachment 47162 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/plugins/gtk/PluginViewGtk.cpp:68:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/plugins/gtk/PluginViewGtk.cpp:69:  One space before end of line comments  [whitespace/comments] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:70:  One space before end of line comments  [whitespace/comments] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/plugins/gtk/PluginViewGtk.cpp:212:  dummy_w is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/gtk/PluginViewGtk.cpp:213:  dummy_i is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/gtk/PluginViewGtk.cpp:214:  dummy_ui is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/gtk/PluginViewGtk.cpp:232:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:250:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:304:  One space before end of line comments  [whitespace/comments] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:311:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:321:  One space before end of line comments  [whitespace/comments] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:325:  One space before end of line comments  [whitespace/comments] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:468:  One space before end of line comments  [whitespace/comments] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:481:  One space before end of line comments  [whitespace/comments] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:777:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:794:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:844:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:847:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 19


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Tarricone]

Sigh, ok...

(In reply to comment #47)
> WebCore/plugins/gtk/PluginViewGtk.cpp:794:  Tests for true/false,
> null/non-null, and zero/non-zero should all be done without equality
> comparisons.  [readability/comparison_to_zero] [5]

I'm not fixing this one, because the test is while checking a version number.  IMO (rmaj == 0 && rmin < 5) makes it a lot clearer that you're testing for a version < 0.5.

New patch in a sec with the rest of the fixes, I hope...

[Brian Tarricone]

Created attachment 47166 [details]
gtk windowless v7

Changes from v6:

* Fix style issues reported by style bot.

[WebKit Review Bot]

Attachment 47166 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/plugins/gtk/PluginViewGtk.cpp:794:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/plugins/gtk/PluginViewGtk.cpp:844:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

> I'm not fixing this one, because the test is while checking a version number. 
> IMO (rmaj == 0 && rmin < 5) makes it a lot clearer that you're testing for a
> version < 0.5.

Thanks.  This is one of our biggest remaining sources of false positives.  We'll need to think about how to improve the style checker here.

[Gustavo Noronha (kov)]

Comment on attachment 47166 [details]
gtk windowless v7

> +        No new tests; there is already a windowless test in the tree.

What test is that? We should unskip it in this patch, I believe.

[Brian Tarricone]

(In reply to comment #52)
> (From update of attachment 47166 [details])
> > +        No new tests; there is already a windowless test in the tree.
> 
> What test is that? We should unskip it in this patch, I believe.

Gah, I can't find it now.  Someone pointed me to one back when I was starting on this, coulda sworn it was in LayoutTests/plugins/, but I don't see it.

[Antonio Gomes]

(In reply to comment #53)
> (In reply to comment #52)
> > (From update of attachment 47166 [details] [details])
> > > +        No new tests; there is already a windowless test in the tree.
> > 
> > What test is that? We should unskip it in this patch, I believe.
> 
> Gah, I can't find it now.  Someone pointed me to one back when I was starting
> on this, coulda sworn it was in LayoutTests/plugins/, but I don't see it.

i might had been a manual test:

"Also, you might want to test the testcases in
WebCore/manual-tests/plugins/windowless.html."

[Brian Tarricone]

Aha, thanks, there it is:  WebCore/manual-tests/plugins/windowless.html

[Gustavo Noronha (kov)]

(In reply to comment #55)
> Aha, thanks, there it is:  WebCore/manual-tests/plugins/windowless.html

OK! I tried it, as well as some other pages, and it seems to work nicely.

[Gustavo Noronha (kov)]

Comment on attachment 47166 [details]
gtk windowless v7

OK, the patch looks good to me, and my testing says it's quite ready to debut. Let's find the remaining bugs by using it =). Thanks!

[Brian Tarricone]

Yeah, it's a little messed up for the move/resize operations, I think just the proper rects aren't invalidating properly.  But the Qt backend has the same problems.  I'd be happy to address that in a later patch (along with some cleanups and common code refactoring), but I'd really like to get this into mainline as is so I can stop maintaining it out of tree.

[Brian Tarricone]

(In reply to comment #57)
> (From update of attachment 47166 [details])
> OK, the patch looks good to me, and my testing says it's quite ready to debut.
> Let's find the remaining bugs by using it =). Thanks!

Great to hear!  I must say thanks for the experience -- you guys have been very helpful and the patch submission process is mostly painless.

[WebKit Commit Bot]

Comment on attachment 47166 [details]
gtk windowless v7

Clearing flags on attachment: 47166

Committed r53955: <http://trac.webkit.org/changeset/53955>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Siddharth Mathur]

Brian, 

I take it that the edit from OS(SYMBIAN) to PLATFORM(SYMBIAN) in the diff snippet below was an inadvertent error? It seems to be causing a build bust for QtWebkit on Symbian. 

-#if defined(XP_UNIX) || defined(Q_WS_X11) || OS(SYMBIAN)
+#if defined(XP_UNIX) || PLATFORM(SYMBIAN)

Thanks,
Siddharth

[Brian Tarricone]

(In reply to comment #62)

> I take it that the edit from OS(SYMBIAN) to PLATFORM(SYMBIAN) in the diff
> snippet below was an inadvertent error? It seems to be causing a build bust for
> QtWebkit on Symbian. 
> 
> -#if defined(XP_UNIX) || defined(Q_WS_X11) || OS(SYMBIAN)
> +#if defined(XP_UNIX) || PLATFORM(SYMBIAN)

Crap, sorry about that.  All I can think is that I messed that one up when resyncing my patch with git master after it had bitrotted.  It should of course be

#if defined(XP_UNIX) || OS(SYMBIAN)