Bug 60368 - [GTK] Implement NetscapePlugin::platformPaint for GTK platform
Summary: [GTK] Implement NetscapePlugin::platformPaint for GTK platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-05-06 06:21 PDT by Carlos Garcia Campos
Modified: 2011-05-06 09:56 PDT (History)
1 user (show)

See Also:


Attachments
Patch (2.81 KB, patch)
2011-05-06 06:23 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (2.77 KB, patch)
2011-05-06 09:42 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-05-06 06:21:52 PDT
There's common code for X11, but the GTK specific part is still missing.
Comment 1 Carlos Garcia Campos 2011-05-06 06:23:46 PDT
Created attachment 92577 [details]
Patch
Comment 2 Martin Robinson 2011-05-06 08:11:55 PDT
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. :)
Comment 3 Carlos Garcia Campos 2011-05-06 08:54:58 PDT
(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
Comment 4 Martin Robinson 2011-05-06 09:00:45 PDT
(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.
Comment 5 Carlos Garcia Campos 2011-05-06 09:42:51 PDT
Created attachment 92591 [details]
Updated patch
Comment 6 Martin Robinson 2011-05-06 09:46:27 PDT
Comment on attachment 92591 [details]
Updated patch

Thanks!
Comment 7 Carlos Garcia Campos 2011-05-06 09:56:04 PDT
Committed r85953: <http://trac.webkit.org/changeset/85953>