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

[bugzilla-daemon at webkit.org]

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


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42765|review?                     |review-
               Flag|                            |




--- Comment #24 from Gustavo Noronha (kov) <gns at gnome.org>  2009-11-10 03:02:33 PDT ---
(From update of attachment 42765)
> -#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!

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