WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
15669
Build with -DXP_UNIX and -lXt for GTK+/X11 port
https://bugs.webkit.org/show_bug.cgi?id=15669
Summary
Build with -DXP_UNIX and -lXt for GTK+/X11 port
Rodney Dawes
Reported
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.
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
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Rodney Dawes
Comment 1
2007-10-24 12:38:13 PDT
Created
attachment 16842
[details]
Build with -DXP_UNIX and WTF_USE_NPOBJECT
Mark Rowe (bdash)
Comment 2
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.
Rodney Dawes
Comment 3
2007-11-16 09:47:05 PST
Created
attachment 17315
[details]
Updated patch with most of the undefs in npapi.h
Mark Rowe (bdash)
Comment 4
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.
Rodney Dawes
Comment 5
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.
Rodney Dawes
Comment 6
2007-11-19 10:05:27 PST
Created
attachment 17400
[details]
Updated patch
Alp Toker
Comment 7
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.
Rodney Dawes
Comment 8
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
Alp Toker
Comment 9
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.
Rodney Dawes
Comment 10
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.
Rodney Dawes
Comment 11
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.
Rodney Dawes
Comment 12
2007-11-27 15:38:11 PST
Updated summary.
Alp Toker
Comment 13
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?
Rodney Dawes
Comment 14
2007-11-29 09:12:08 PST
Created
attachment 17593
[details]
Updated patch to fix alp's issues with previous patch
Alp Toker
Comment 15
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).
Rodney Dawes
Comment 16
2007-12-03 07:41:18 PST
Created
attachment 17675
[details]
patch without -lxt Fine. Here is a patch without the -lXt part.
Rodney Dawes
Comment 17
2007-12-03 08:37:14 PST
Created
attachment 17676
[details]
remove WebKitTools changes and fix changelog for -lXt removal
Alp Toker
Comment 18
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?
Darin Adler
Comment 19
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
Rodney Dawes
Comment 20
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?
Rodney Dawes
Comment 21
2007-12-20 13:57:33 PST
Created
attachment 18016
[details]
Update to use internal for the name, and set the Reviewer to Darin
Mark Rowe (bdash)
Comment 22
2007-12-20 14:17:58 PST
Landed in
r28911
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug