Bug 151992 – [UNIX] Add support for windowless NPAPI plugins with no UI in non X11 platforms

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We are currently disabling all plugins when running under wayland, for example. There are some plugins, like the one used by extensions.gnome.org, that don't do any rendering, so there's not reason not to support those plugins under wayland or any other unix non-x11 platform.

[Carlos Garcia Campos]

Created attachment 266886 [details]
Patch

[Carlos Garcia Campos]

Created attachment 266887 [details]
Updated patch

I forgot to add the new files to the EFL makefile.

[Carlos Garcia Campos]

Created attachment 266889 [details]
Try to fix EFL build

[Carlos Garcia Campos]

Created attachment 266893 [details]
Try to fix EFL build

[Carlos Garcia Campos]

Created attachment 266898 [details]
Try to fix EFL build

[Carlos Garcia Campos]

Ping reviewers.

[Gustavo Noronha (kov)]

Comment on attachment 266898 [details]
Try to fix EFL build

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

> Source/WebKit2/ChangeLog:15
> +        that will ue the X11 implementatin only when needed and

s/ue/use/

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:275
> +    ASSERT(m_plugin.isWindowed());

Won't this be called with windowless? It looks like the only criteria for platformPaint() to call this one is whether it's running under X, and I'd expect windowed plugins to do their own painting?

[Carlos Garcia Campos]

Comment on attachment 266898 [details]
Try to fix EFL build

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

>> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:275
>> +    ASSERT(m_plugin.isWindowed());
> 
> Won't this be called with windowless? It looks like the only criteria for platformPaint() to call this one is whether it's running under X, and I'd expect windowed plugins to do their own painting?

You are right, I forgot to return early in NetscapePlugin::platformPaint() like I do in other windowless only methods. Good catch!

[Carlos Garcia Campos]

Created attachment 270480 [details]
Updated patch

[Darin Adler]

Comment on attachment 270480 [details]
Updated patch

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

While I’m not a platform expert I think I can review.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:534
> +            const auto& display = WebCore::PlatformDisplay::sharedDisplay();

I don’t think the const here is needed or helpful.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:537
> +            *reinterpret_cast<Display**>(value) = downcast<PlatformDisplayX11>(display).native();

Strange that the WebCore prefix is not needed here but is needed on the line above.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:56
>  class NetscapePluginStream;
> -    
> +#if PLUGIN_ARCHITECTURE(X11)
> +class NetscapePluginUnix;
> +#endif
>  class NetscapePlugin : public Plugin {

Please don’t delete the blank line here that separates the forward declarations from the class definition. Not clear that we need to put #if around a forward declaration. If we do, then please put it in a separate paragraph with spaces above and below the #if/#endif

> Source/WebKit2/WebProcess/Plugins/Netscape/unix/NetscapePluginUnix.h:31
> +#include <WebCore/GraphicsContext.h>

I think we could use forward declarations for GraphicsContext and IntRect in this file rather than including the header, which is typically better.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.h:31
> +#if PLUGIN_ARCHITECTURE(X11)
> +#include "NetscapePluginUnix.h"

We normally put a blank line here between the #if and the #include.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.h:38
> +class NetscapePluginX11 final: public NetscapePluginUnix {

We normally put a space after final before ":".

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.h:46
> +private:

We normally put a blank line here before private.

[Carlos Garcia Campos]

Created attachment 270557 [details]
Patch for landing

[Carlos Garcia Campos]

(In reply to comment #10)
> Comment on attachment 270480 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270480&action=review
> 
> While I’m not a platform expert I think I can review.

Thanks!

> > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:534
> > +            const auto& display = WebCore::PlatformDisplay::sharedDisplay();
> 
> I don’t think the const here is needed or helpful.

Ok, removed.

> > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:537
> > +            *reinterpret_cast<Display**>(value) = downcast<PlatformDisplayX11>(display).native();
> 
> Strange that the WebCore prefix is not needed here but is needed on the line
> above.

It's not needed, there's a using namespace in this file.

> > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:56
> >  class NetscapePluginStream;
> > -    
> > +#if PLUGIN_ARCHITECTURE(X11)
> > +class NetscapePluginUnix;
> > +#endif
> >  class NetscapePlugin : public Plugin {
> 
> Please don’t delete the blank line here that separates the forward
> declarations from the class definition. Not clear that we need to put #if
> around a forward declaration. If we do, then please put it in a separate
> paragraph with spaces above and below the #if/#endif

I don't think we need ifdefs.

> > Source/WebKit2/WebProcess/Plugins/Netscape/unix/NetscapePluginUnix.h:31
> > +#include <WebCore/GraphicsContext.h>
> 
> I think we could use forward declarations for GraphicsContext and IntRect in
> this file rather than including the header, which is typically better.

Ok.

> > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.h:31
> > +#if PLUGIN_ARCHITECTURE(X11)
> > +#include "NetscapePluginUnix.h"
> 
> We normally put a blank line here between the #if and the #include.

Ok.

> > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.h:38
> > +class NetscapePluginX11 final: public NetscapePluginUnix {
> 
> We normally put a space after final before ":".

OK.

> > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.h:46
> > +private:
> 
> We normally put a blank line here before private.

Ok.

[Carlos Garcia Campos]

Committed r196053: <http://trac.webkit.org/changeset/196053>