Bug 15669 - Build with -DXP_UNIX and -lXt for GTK+/X11 port
Summary: Build with -DXP_UNIX and -lXt for GTK+/X11 port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-24 12:37 PDT by Rodney Dawes
Modified: 2007-12-20 14:17 PST (History)
1 user (show)

See Also:


Attachments
Build with -DXP_UNIX and WTF_USE_NPOBJECT (2.87 KB, patch)
2007-10-24 12:38 PDT, Rodney Dawes
mrowe: review-
Details | Formatted Diff | Diff
Updated patch with most of the undefs in npapi.h (2.72 KB, patch)
2007-11-16 09:47 PST, Rodney Dawes
mrowe: review-
Details | Formatted Diff | Diff
Updated xpunix build patch (1.91 KB, patch)
2007-11-16 16:42 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch (6.98 KB, patch)
2007-11-19 10:05 PST, Rodney Dawes
alp: review-
Details | Formatted Diff | Diff
Put a license block in the new header and update the description about npapi/npruntime (7.08 KB, patch)
2007-11-19 10:54 PST, Rodney Dawes
alp: review-
Details | Formatted Diff | Diff
Updated patch to use !directfb in WebCore.pro (5.41 KB, patch)
2007-11-23 12:31 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to use x11 instead of !mac:!win32-*:!directfb (6.63 KB, patch)
2007-11-27 15:37 PST, Rodney Dawes
alp: review-
Details | Formatted Diff | Diff
Updated patch to fix alp's issues with previous patch (8.92 KB, patch)
2007-11-29 09:12 PST, Rodney Dawes
alp: review-
Details | Formatted Diff | Diff
patch without -lxt (8.90 KB, patch)
2007-12-03 07:41 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
remove WebKitTools changes and fix changelog for -lXt removal (7.41 KB, patch)
2007-12-03 08:37 PST, Rodney Dawes
darin: review+
Details | Formatted Diff | Diff
Update to use internal for the name, and set the Reviewer to Darin (7.45 KB, patch)
2007-12-20 13:57 PST, Rodney Dawes
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rodney Dawes 2007-10-24 12:37:36 PDT
The GTK+ port needs -DXP_UNIX and WTF_USE_NPOBJECT for implementation of NPAPI plug-in support. The attached patch enables this.
Comment 1 Rodney Dawes 2007-10-24 12:38:13 PDT
Created attachment 16842 [details]
Build with -DXP_UNIX and WTF_USE_NPOBJECT
Comment 2 Mark Rowe (bdash) 2007-10-24 13:56:54 PDT
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.
Comment 3 Rodney Dawes 2007-11-16 09:47:05 PST
Created attachment 17315 [details]
Updated patch with most of the undefs in npapi.h
Comment 4 Mark Rowe (bdash) 2007-11-16 11:05:25 PST
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.
Comment 5 Rodney Dawes 2007-11-16 16:42:14 PST
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.
Comment 6 Rodney Dawes 2007-11-19 10:05:27 PST
Created attachment 17400 [details]
Updated patch
Comment 7 Alp Toker 2007-11-19 10:45:07 PST
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.
Comment 8 Rodney Dawes 2007-11-19 10:54:34 PST
Created attachment 17403 [details]
Put a license block in the new header and update the description about npapi/npruntime
Comment 9 Alp Toker 2007-11-19 12:05:06 PST
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.
Comment 10 Rodney Dawes 2007-11-23 12:31:47 PST
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.
Comment 11 Rodney Dawes 2007-11-27 15:37:39 PST
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.
Comment 12 Rodney Dawes 2007-11-27 15:38:11 PST
Updated summary.
Comment 13 Alp Toker 2007-11-28 09:07:38 PST
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?
Comment 14 Rodney Dawes 2007-11-29 09:12:08 PST
Created attachment 17593 [details]
Updated patch to fix alp's issues with previous patch
Comment 15 Alp Toker 2007-12-01 00:19:02 PST
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).
Comment 16 Rodney Dawes 2007-12-03 07:41:18 PST
Created attachment 17675 [details]
patch without -lxt

Fine. Here is a patch without the -lXt part.
Comment 17 Rodney Dawes 2007-12-03 08:37:14 PST
Created attachment 17676 [details]
remove WebKitTools changes and fix changelog for -lXt removal
Comment 18 Alp Toker 2007-12-06 13:37:04 PST
This is OK on the GTK+ port side. Can someone with plugin experience review the changes to shared headers?
Comment 19 Darin Adler 2007-12-06 14:51:38 PST
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
Comment 20 Rodney Dawes 2007-12-20 09:39:59 PST
(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?
Comment 21 Rodney Dawes 2007-12-20 13:57:33 PST
Created attachment 18016 [details]
Update to use internal for the name, and set the Reviewer to Darin
Comment 22 Mark Rowe (bdash) 2007-12-20 14:17:58 PST
Landed in r28911.