Bug 151992 - [UNIX] Add support for windowless NPAPI plugins with no UI in non X11 platforms
Summary: [UNIX] Add support for windowless NPAPI plugins with no UI in non X11 platforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 153878
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-08 08:11 PST by Carlos Garcia Campos
Modified: 2016-02-04 11:32 PST (History)
3 users (show)

See Also:


Attachments
Patch (41.86 KB, patch)
2015-12-08 08:25 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (42.82 KB, patch)
2015-12-08 08:33 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL build (42.82 KB, patch)
2015-12-08 08:45 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL build (43.11 KB, patch)
2015-12-08 09:01 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL build (43.17 KB, patch)
2015-12-08 09:30 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (43.22 KB, patch)
2016-02-02 03:14 PST, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch for landing (43.19 KB, patch)
2016-02-02 23:34 PST, Carlos Garcia Campos
no flags 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 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>