[webkit-reviews] review denied: [Bug 18831] [GTK] support windowless plugins : [Attachment 42765] gtk windowless v5

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Brian Tarricone
<bjt23 at cornell.edu>'s request for review:
Bug 18831: [GTK] support windowless plugins
https://bugs.webkit.org/show_bug.cgi?id=18831

Attachment 42765: gtk windowless v5
https://bugs.webkit.org/attachment.cgi?id=42765&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> -#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!