Bug 151992

Summary: [UNIX] Add support for windowless NPAPI plugins with no UI in non X11 platforms
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, mcatanzaro, mrobinson
Priority: P2 Keywords: Gtk
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 153878    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Updated patch
none
Try to fix EFL build
none
Try to fix EFL build
none
Try to fix EFL build
none
Updated patch
darin: review+
Patch for landing none

Description Carlos Garcia Campos 2015-12-08 08:11:12 PST
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.
Comment 1 Carlos Garcia Campos 2015-12-08 08:25:28 PST
Created attachment 266886 [details]
Patch
Comment 2 Carlos Garcia Campos 2015-12-08 08:33:59 PST
Created attachment 266887 [details]
Updated patch

I forgot to add the new files to the EFL makefile.
Comment 3 Carlos Garcia Campos 2015-12-08 08:45:37 PST
Created attachment 266889 [details]
Try to fix EFL build
Comment 4 Carlos Garcia Campos 2015-12-08 09:01:21 PST
Created attachment 266893 [details]
Try to fix EFL build
Comment 5 Carlos Garcia Campos 2015-12-08 09:30:43 PST
Created attachment 266898 [details]
Try to fix EFL build
Comment 6 Carlos Garcia Campos 2016-01-04 08:13:40 PST
Ping reviewers.
Comment 7 Gustavo Noronha (kov) 2016-01-29 09:48:08 PST
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?
Comment 8 Carlos Garcia Campos 2016-02-01 23:10:45 PST
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!
Comment 9 Carlos Garcia Campos 2016-02-02 03:14:42 PST
Created attachment 270480 [details]
Updated patch
Comment 10 Darin Adler 2016-02-02 16:42:06 PST
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.
Comment 11 Carlos Garcia Campos 2016-02-02 23:34:45 PST
Created attachment 270557 [details]
Patch for landing
Comment 12 Carlos Garcia Campos 2016-02-02 23:36:53 PST
(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.
Comment 13 Carlos Garcia Campos 2016-02-03 00:18:37 PST
Committed r196053: <http://trac.webkit.org/changeset/196053>