[GTK] Implement NetscapePlugin::platformPaint for GTK platform

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

There's common code for X11, but the GTK specific part is still missing.

[Carlos Garcia Campos]

Created attachment 92577 [details]
Patch

[Martin Robinson]

Comment on attachment 92577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92577&action=review

Looking good. Just a couple comments.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:224
> +#elif PLATFORM(GTK)
> +    RefPtr<cairo_surface_t> drawableSurface = adoptRef(cairo_xlib_surface_create(x11Display(),
> +                                                                                 m_drawable,
> +                                                                                 static_cast<NPSetWindowCallbackStruct*>(m_npWindow.ws_info)->visual,
> +                                                                                 m_frameRect.width(),
> +                                                                                 m_frameRect.height()));

Wouldn't it be better to wrap this surface in a Cairo surface after the calls to NPP_HandleEvent and XSync, like Qt?

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:267
> +    cairo_rectangle(cr,
> +                    m_frameRect.x() + exposedRect.x(), m_frameRect.y() + exposedRect.y(),
> +                    exposedRect.width(), exposedRect.height());

Do you mind just putting this one on one line? 120 character is generally okay. Sorry for the nit. :)

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 92577 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92577&action=review
> 
> Looking good. Just a couple comments.
> 
> > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:224
> > +#elif PLATFORM(GTK)
> > +    RefPtr<cairo_surface_t> drawableSurface = adoptRef(cairo_xlib_surface_create(x11Display(),
> > +                                                                                 m_drawable,
> > +                                                                                 static_cast<NPSetWindowCallbackStruct*>(m_npWindow.ws_info)->visual,
> > +                                                                                 m_frameRect.width(),
> > +                                                                                 m_frameRect.height()));
> 
> Wouldn't it be better to wrap this surface in a Cairo surface after the calls to NPP_HandleEvent and XSync, like Qt?

I'm not sure I understand what you mean by "wrap this surface in a Cairo surface". I tried to follow the current webkit1 approach.

> > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:267
> > +    cairo_rectangle(cr,
> > +                    m_frameRect.x() + exposedRect.x(), m_frameRect.y() + exposedRect.y(),
> > +                    exposedRect.width(), exposedRect.height());
> 
> Do you mind just putting this one on one line? 120 character is generally okay. Sorry for the nit. :)

Sure, no problem

[Martin Robinson]

(In reply to comment #3)

> > Wouldn't it be better to wrap this surface in a Cairo surface after the calls to NPP_HandleEvent and XSync, like Qt?
> I'm not sure I understand what you mean by "wrap this surface in a Cairo surface". I tried to follow the current webkit1 approach.

Sorry. I should have said wrap m_drawable in a Cairo surface. In the Qt port they wait until right before blitting to do this. Perhaps it isn't possible with the XLib backend, but couldn't the surface be out of sync according to the API. In any case, I think it's better to mirror the Qt port for clarity, unless it's specifically an error in this case. You could use m_pluginDisplay instead of x11Display() if you did.

[Carlos Garcia Campos]

Created attachment 92591 [details]
Updated patch

[Martin Robinson]

Comment on attachment 92591 [details]
Updated patch

Thanks!

[Carlos Garcia Campos]

Committed r85953: <http://trac.webkit.org/changeset/85953>