Bug 57796 - [Qt] [WK2] Add method to disable plugins under WK2
Summary: [Qt] [WK2] Add method to disable plugins under WK2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P3 Enhancement
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-04-04 15:33 PDT by Keith Kyzivat
Modified: 2011-05-05 15:04 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.53 KB, patch)
2011-04-04 16:27 PDT, Keith Kyzivat
no flags Details | Formatted Diff | Diff
Patch (5.04 KB, patch)
2011-04-06 12:35 PDT, Keith Kyzivat
no flags Details | Formatted Diff | Diff
Patch (1.25 KB, patch)
2011-04-28 17:28 PDT, Keith Kyzivat
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Kyzivat 2011-04-04 15:33:48 PDT
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.
Comment 1 Keith Kyzivat 2011-04-04 16:27:11 PDT
Created attachment 88152 [details]
Patch
Comment 2 Benjamin Poulain 2011-04-05 05:03:23 PDT
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?
Comment 3 Keith Kyzivat 2011-04-05 09:28:16 PDT
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.
Comment 4 Keith Kyzivat 2011-04-06 10:37:50 PDT
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
Comment 5 Keith Kyzivat 2011-04-06 12:35:53 PDT
Created attachment 88487 [details]
Patch
Comment 6 Benjamin Poulain 2011-04-06 14:29:08 PDT
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.
Comment 7 Keith Kyzivat 2011-04-06 15:21:19 PDT
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 (?).
Comment 8 Tor Arne Vestbø 2011-04-26 15:51:36 PDT
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.
Comment 9 Keith Kyzivat 2011-04-28 17:24:00 PDT
(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.
Comment 10 Keith Kyzivat 2011-04-28 17:28:40 PDT
Created attachment 91602 [details]
Patch
Comment 11 Laszlo Gombos 2011-05-05 14:13:33 PDT
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.
Comment 12 WebKit Commit Bot 2011-05-05 15:04:31 PDT
Comment on attachment 91602 [details]
Patch

Clearing flags on attachment: 91602

Committed r85887: <http://trac.webkit.org/changeset/85887>
Comment 13 WebKit Commit Bot 2011-05-05 15:04:37 PDT
All reviewed patches have been landed.  Closing bug.