Bug 57796 – [Qt] [WK2] Add method to disable plugins under WK2

• Component: WebKit2
• Status: RESOLVED FIXED

[Keith Kyzivat]

When compiling WK2 in non-X11 unix environments, the compilation of the Netscape plugin support fails due to X11 dependencies.
Furthermore, if one wishes to just disable this functionality with ENABLE_NETSCAPE_PLUGIN_API=0 , it doesn't work, as it still tries to link with the Netscape Plugin code, which depends on X11.

[Keith Kyzivat]

Created attachment 88152 [details]
Patch

[Benjamin Poulain]

Comment on attachment 88152 [details]
Patch

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

Why the guard #elif (PLATFORM(QT) || (PLATFORM(GTK))) && (OS(UNIX) && !OS(MAC_OS_X)) is not enough for your issue?
Your fix looks weird to me, the plugin architecture seems more general than just NPAPI.

> Tools/Tools.pro:14
> +        exists($$PWD/DumpRenderTree/qt/TestNetscapePlugin/TestNetscapePlugin.pro): SUBDIRS += DumpRenderTree/qt/TestNetscapePlugin/TestNetscapePlugin.pro

Why do you change the indentation?

[Keith Kyzivat]

Comment on attachment 88152 [details]
Patch

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

What I've come across is - I have a Linux install that contains Qt (Lighthouse), but no X11.  The WK2 NPAPI code currently has dependencies on X11, so I'd like to be able to disable it. 
It appears that WK2 does not honor ENABLE_NETSCAPE_PLUGIN_API=0, so I put this in here as an easy way to make this particular case work.

Right now, the only files that use PLUGIN_ARCHITECTURE_XXX are all relating to NPAPI (that I can see).
I do see what you're saying though - that "PLUGIN_ARCHITECTURE_" indicates this applies to more than just NPAPI.

Maybe the answer is an ENABLE_PLUGIN_ARCHITECTURE=0 define?

>> Tools/Tools.pro:14
>> +        exists($$PWD/DumpRenderTree/qt/TestNetscapePlugin/TestNetscapePlugin.pro): SUBDIRS += DumpRenderTree/qt/TestNetscapePlugin/TestNetscapePlugin.pro
> 
> Why do you change the indentation?

This is leftover from a prior iteration, and I missed cleaning it up.  Will fix in an updated patch.

[Keith Kyzivat]

Change title from:
[Qt] [WK2] Allow WK2 ENABLE_NETSCAPE_PLUGIN_API=0 to work in non-X11 unix environments
to:
[Qt] [WK2] Add feature to disable all plugins under WK2

[Keith Kyzivat]

Created attachment 88487 [details]
Patch

[Benjamin Poulain]

Comment on attachment 88487 [details]
Patch

This looks reasonable but there is something I don't get.
Why add a new define for plugins, instead of adding a new define for X11 (if there is no such thing already)?

I have not checked, but I would think there are other cases of Unix but !X11. Or is X11 really used by WebCore only for plugins? I would think window surface sharing is another area where such cases happen.

I would think (Linux && !X11) has other applications we could share with other ports like Android.

[Keith Kyzivat]

I had originally thought to check against X11, however I was unsure if implicitly turning off Netscape Plugin support for !X11 Unix environments was the right thing to do.

Right now the Netscape Plugin API relies on X11 constructs, however, we may eventually want to add support for this.

I tried yesterday to do just that, but I was unable to figure out a good way to determine if there is X11 available at Platform.h time without including qglobal.h (to get access to Q_WS_X11) - and that's a bad idea - If done there, it means qglobal.h will be included for all things including Platform.h -- most of Webkit I think (?).

[Tor Arne Vestbø]

Comment on attachment 88487 [details]
Patch

I agree with Benjamin. If there's a dependency on X11, we should check for that using stuff like the HAVE() macro, not another global ENABLE() macro that just maps to the NPAPI enable.

[Keith Kyzivat]

(In reply to comment #8)
> (From update of attachment 88487 [details])
> I agree with Benjamin. If there's a dependency on X11, we should check for that using stuff like the HAVE() macro, not another global ENABLE() macro that just maps to the NPAPI enable.

Unfortunately, I have not found a way to add a HAVE(X11) without including qglobal.h in platform.h -- and that would mask a lot of errors if we did that.

I have a solution forthcoming.

[Keith Kyzivat]

Created attachment 91602 [details]
Patch

[Laszlo Gombos]

Comment on attachment 91602 [details]
Patch

I find it useful to have an option to disable plugins for WK2 independently of X11 support. We might still need an independent HAVE(X11) switch later on for reasons discussed above.

[WebKit Commit Bot]

Comment on attachment 91602 [details]
Patch

Clearing flags on attachment: 91602

Committed r85887: <http://trac.webkit.org/changeset/85887>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.