Bug 15669 – Build with -DXP_UNIX and -lXt for GTK+/X11 port

• Component: Plug-ins
• Status: RESOLVED FIXED

[Rodney Dawes]

The GTK+ port needs -DXP_UNIX and WTF_USE_NPOBJECT for implementation of NPAPI plug-in support. The attached patch enables this.

[Rodney Dawes]

Created attachment 16842 [details]
Build with -DXP_UNIX and WTF_USE_NPOBJECT

[Mark Rowe (bdash)]

Comment on attachment 16842 [details]
Build with -DXP_UNIX and WTF_USE_NPOBJECT

I really think the #undef's need to be isolated to a single location with a comment explaining why they are needed.

You also need to include ChangeLog entries with your patch.

[Rodney Dawes]

Created attachment 17315 [details]
Updated patch with most of the undefs in npapi.h

[Mark Rowe (bdash)]

Comment on attachment 17315 [details]
Updated patch with most of the undefs in npapi.h

It turns out that npapi.h is a public API header file so we can't do the #undef's there.  What we'll need to do is add a new header file to JavaScriptCore that includes npapi.h and Xresources.h and then #undef's the relevant values.  Places inside JavaScriptCore/WebCore/WebKit that include npapi.h should then be updated to include that wrapper header file instead.

One issue is that npruntime.h, another public API header file, includes npapi.h and we clearly can't include our wrapper header in that.  It may be required to wrap npruntime.h instead of npapi.h and move anyone that includes npapi.h or npruntime.h directly to the wrapper.

[Rodney Dawes]

Created attachment 17323 [details]
Updated xpunix build patch

Is this an acceptable way to implement this fix? It's much cleaner and much less invasive than trying to find all the places where npapi.h ends up getting included, and trying to fix each file to use Proxy_npruntime.h instead. I was still ending up with compile errors, with no obvious link to npruntime.h or npapi.h. If this is a sufficient method of fixing it, I'll update the comment in the new file, and create a patch with ChangeLog entries too. Thanks.

[Rodney Dawes]

Created attachment 17400 [details]
Updated patch

[Alp Toker]

Comment on attachment 17400 [details]
Updated patch

Looks like this patch will break non-X11 builds like DirectFB. There needs to be a configure switch to mark that X11 is available or something.

Also, linking to Xt doesn't sound like the right thing to do in standard WebKit/GTK+ builds. There should be an optional switch to enable this kind of feature.

[Rodney Dawes]

Created attachment 17403 [details]
Put a license block in the new header and update the description about npapi/npruntime

[Alp Toker]

Comment on attachment 17403 [details]
Put a license block in the new header and update the description about npapi/npruntime

>Index: WebCore/WebCore.pro
>===================================================================
>--- WebCore/WebCore.pro	(revision 27906)
>+++ WebCore/WebCore.pro	(working copy)
>@@ -91,6 +91,11 @@ DEPENDPATH += editing/qt history/qt load
> }
> 
> gtk-port {
>+!mac:!win32-* {
>+    DEFINES += XP_UNIX
>+    LIBS += -lXt
>+}

The assumption that !mac:!win32-* implies that X11 is available is incorrect. The core of the GTK+ port is windowing-system neutral, so features specific to X11 need to be added with care.

I'm also not convinced that we want to link to Xt by default even when X11 is available. Can we make plugin support an optional feature?

Thanks.

[Rodney Dawes]

Created attachment 17468 [details]
Updated patch to use !directfb in WebCore.pro

Alp, does this version suit you better? It uses !directfb to avoid enabling the X11 stuff when using directfb. I don't think it makes sense to have plug-ins be something you are required to enable at compile time, when you're building for a windowing system that NPAPI supports. We should by default allow that feature to work. And using -lXt is a requirement for that to work.

[Rodney Dawes]

Created attachment 17561 [details]
Updated patch to use x11 instead of !mac:!win32-*:!directfb

Here's an updated patch. This patch requires the patch in bug 16146 to land, in order to work correctly. Without it, the code will build, but XP_UNIX will not be defined, and -lXt will not be added to LIBS.

I've also removed the NP_OBJECT enabling part from this patch. There's apparently another piece that needs to be enabled as well, so I will enable that, and submit a separate patch for it.

[Rodney Dawes]

Updated summary.

[Alp Toker]

Comment on attachment 17561 [details]
Updated patch to use x11 instead of !mac:!win32-*:!directfb

>Index: JavaScriptCore/bindings/npapi.h
>===================================================================
>--- JavaScriptCore/bindings/npapi.h	(revision 27989)
>+++ JavaScriptCore/bindings/npapi.h	(working copy)
>@@ -89,6 +89,7 @@
> #ifdef XP_UNIX
>     #include <X11/Xlib.h>
>     #include <X11/Xutil.h>
>+    #include <stdio.h>
> #endif

Can you explain what this change is for in the ChangeLog entry?

>Index: WebCore/WebCore.pro
>===================================================================
>--- WebCore/WebCore.pro	(revision 27989)
>+++ WebCore/WebCore.pro	(working copy)
>@@ -130,6 +130,11 @@ DEPENDPATH += editing/qt history/qt load
> }
> 
> gtk-port {
>+x11 {
>+    DEFINES += XP_UNIX
>+    LIBS += -lXt
>+}
>+

I'm still not convinced that pulling in a dependency on another toolkit makes much sense for the GTK+ port on X11, though this can definitely be an optional switch for users to enable if they want the functionality.

>Index: JavaScriptCore/bindings/npruntime_proxy.h
>===================================================================
>--- JavaScriptCore/bindings/npruntime_proxy.h	(revision 0)
>+++ JavaScriptCore/bindings/npruntime_proxy.h	(revision 0)
>@@ -0,0 +1,26 @@
>+/*
>+ * Copyright (C) 2007 Collabora, Ltd.  All rights reserved.
>+ * 
>+ */

All rights reserved and no license?

[Rodney Dawes]

Created attachment 17593 [details]
Updated patch to fix alp's issues with previous patch

[Alp Toker]

Comment on attachment 17593 [details]
Updated patch to fix alp's issues with previous patch

>+sub enableGtkPlugins()
>+{
>+    determineEnableGtkPlugins();
>+    return $enableGtkPlugins;
>+}
>+
>+sub determineEnableGtkPlugins()
>+{
>+    return if defined($enableGtkPlugins);
>+
>+    if (checkArgv("--disable-gtk-plugins")) {
>+        $enableGtkPlugins = 0;
>+    } else {
>+        $enableGtkPlugins = 1;
>+    }
>+}

My comment from the previous review was:

  Also, linking to Xt doesn't sound like the right thing to do in standard
  WebKit/GTK+ builds. There should be an optional switch to enable this kind of
  feature.

If you really want to contribute this as a standalone patch independent of the actual code, can you be sure to change this to disabled by default? There's no indication that linking to Xt is a good default, especially when nothing in the codebase uses it yet.

Usually small build system changes like this are included in the same patch as the actual implementation. It's difficult to review one or the other by itself (there is no indication why X intrinsics is being linked at all here).

[Rodney Dawes]

Created attachment 17675 [details]
patch without -lxt

Fine. Here is a patch without the -lXt part.

[Rodney Dawes]

Created attachment 17676 [details]
remove WebKitTools changes and fix changelog for -lXt removal

[Alp Toker]

This is OK on the GTK+ port side. Can someone with plugin experience review the changes to shared headers?

[Darin Adler]

Comment on attachment 17676 [details]
remove WebKitTools changes and fix changelog for -lXt removal

This seems good to me, except that I do not like the name of the new header file.

I don't think the use of the term "proxy" in the title makes sense, and since it's an internal file for WebCore, we don't have to use the "np" style naming.

 gtk-port {
+x11:plugins {
+    DEFINES += XP_UNIX
+}
+

Can this be indented so the nesting looks logical? I don't know the syntax of these files, so I'm not sure.

r=me as-is, though -- those are really quibbles and this code is already pretty messy

[Rodney Dawes]

(In reply to comment #19)
> (From update of attachment 17676 [details] [edit])
> This seems good to me, except that I do not like the name of the new header
> file.
> 
> I don't think the use of the term "proxy" in the title makes sense, and since
> it's an internal file for WebCore, we don't have to use the "np" style naming.

Proxy is the only term I can think of that accurately describes the function of the header. It proxies the including of npruntime.h/npapi.h. Do you have any better suggestions?

[Rodney Dawes]

Created attachment 18016 [details]
Update to use internal for the name, and set the Reviewer to Darin

[Mark Rowe (bdash)]

Landed in r28911.