[Webkit-unassigned] [Bug 18831] [GTK] support windowless plugins

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=18831





--- Comment #28 from Brian Tarricone <bjt23 at cornell.edu>  2009-11-10 05:27:33 PDT ---
>>  #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!).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.