Summary: | [UNIX] Add support for windowless NPAPI plugins with no UI in non X11 platforms | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | gustavo, mcatanzaro, mrobinson | ||||||||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 153878 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2015-12-08 08:11:12 PST
Created attachment 266886 [details]
Patch
Created attachment 266887 [details]
Updated patch
I forgot to add the new files to the EFL makefile.
Created attachment 266889 [details]
Try to fix EFL build
Created attachment 266893 [details]
Try to fix EFL build
Created attachment 266898 [details]
Try to fix EFL build
Ping reviewers. 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 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! Created attachment 270480 [details]
Updated patch
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. Created attachment 270557 [details]
Patch for landing
(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. Committed r196053: <http://trac.webkit.org/changeset/196053> |