Summary: | [Gtk] disable plugins for gtk/directfb target | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jan Alonzo <jmalonzo> | ||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | Keywords: | Gtk | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Jan Alonzo
2008-08-05 04:17:40 PDT
Created attachment 23620 [details] Patch to fix this issue - includes directfb build fixes too. This patch should fix this issue. For Debian users, a patch[1] is needed to libcairo-directfb2-dev for it to link properly. [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=499662 Comment on attachment 23620 [details]
Patch to fix this issue - includes directfb build fixes too.
Substance:
- Is GDK_WINDOWING_X11 really defined when building for Gtk+/X11? Is that due a broken pc file?
- Your patch is changing a lot more than advertised. Change the summary or split it up.
- We added cairo-ft for a specific reason, we have to figure out if that reason is gone
- Adding a random directory to cppflags is quite "dangerous", people with broken packages could just do CPPFLAGS=-I/right/dir ./configure. Specially when cross compiling we might pick up the wrong headers.
Style:
- #define WTF_USE_BLA 1 is supposed to be used in a #if USE(BLA) way.
And I agree that plugins should/can currently only be build on Gtk+/X11.
(In reply to comment #2) > (From update of attachment 23620 [details] [edit]) > Substance: > - Is GDK_WINDOWING_X11 really defined when building for Gtk+/X11? Is that due > a broken pc file? It is defined in gdkconfig.h, which you get when you install the "gtk-dev" package. > > - Your patch is changing a lot more than advertised. Change the summary or > split it up. Ok. I'll update the summary. > > - We added cairo-ft for a specific reason, we have to figure out if that > reason is gone It was ok to keep it there but looking into it more, I don't think we need it anymore for two reasons: 1. It assumes an X11-based cairo, which means we don't get enough value out of it when we build with directfb 2. cairo.pc already contains what cairo-ft can provide us. > > - Adding a random directory to cppflags is quite "dangerous", people with > broken packages could just do CPPFLAGS=-I/right/dir ./configure. Specially when > cross compiling we might pick up the wrong headers. Thanks. I don't think we need this line anymore. I added it because I thought I'm going to need GDK_WINDOWING_DIRECTB. But this should come from the .pc file (which is not there atm). > > Style: > - #define WTF_USE_BLA 1 is supposed to be used in a #if USE(BLA) way. Ok. I changed this and add WTF_PLATFORM_X11 instead and add the check along with GDK_WINDOWING_X11 (e.g. GDK_WINDOWING_X11 && PLATFORM(X11)). The reason being is that some systems (e.g. Debian) allow gtk X11 packages and directfb packages installed side-by-side. Therefore checking for GDK_WINDOWING_X11 is not enough to determine an X11 (or non-X11) build. I hope that answers your questions. Thanks and appreciate your feedback. Created attachment 23702 [details]
updated per holger's feedback, updated .pc checks and introduce WTF_PLATFORM_X11
I addressed Holger's feedback and updated the patch based on my response from the previous post.
Thanks.
Comment on attachment 23702 [details]
updated per holger's feedback, updated .pc checks and introduce WTF_PLATFORM_X11
Alp is your man...
Comment on attachment 23702 [details]
updated per holger's feedback, updated .pc checks and introduce WTF_PLATFORM_X11
I think PLATFORM(X11) is a good direction since we may want to share plugin code between more ports some day.
Will your checks still work using only #if PLATFORM(X11) without checking GDK_WINDOWING_X11 at all? That would be cleaner.
This check is wrong:
+/* Windowing Systems */
+#if PLATFORM(GTK) && !PLATFORM(WIN) && !PLATFORM(DIRECTFB)
+#define WTF_PLATFORM_X11 1
+#endif
+
It should be PLATFORM(WIN_OS). But moreover, I'd prefer if we define this only in the build system without touching Platform.h at all. This sounds like an opportunity to start making use of AC_DEFINE in configure.ac
+ PKG_CHECK_MODULES(CAIRO, cairo-directfb >= $CAIRO_REQUIRED_VERSION)
+ PKG_CHECK_MODULES(GTK, gtk+-directfb-2.0 >= $GTK_REQUIRED_VERSION)
^ I Don't think we actually need to check for either of these packages, since we don't actually use any cairo-directfb or gtk+-directfb-2.0 features directly, even when DirectFB is the target. I don't mind strongly if we add these checks though, your call.
(In reply to comment #6) > (From update of attachment 23702 [details] [edit]) > I think PLATFORM(X11) is a good direction since we may want to share plugin > code between more ports some day. > > Will your checks still work using only #if PLATFORM(X11) without checking > GDK_WINDOWING_X11 at all? That would be cleaner. Yup. It does. I don't think we need GDK_WINDOWING_X11 anymore as we're checking for gtk-x11 package anyway. > This check is wrong: > > +/* Windowing Systems */ > +#if PLATFORM(GTK) && !PLATFORM(WIN) && !PLATFORM(DIRECTFB) > +#define WTF_PLATFORM_X11 1 > +#endif > + > > It should be PLATFORM(WIN_OS). But moreover, I'd prefer if we define this only > in the build system without touching Platform.h at all. This sounds like an > opportunity to start making use of AC_DEFINE in configure.ac Ok. > > + PKG_CHECK_MODULES(CAIRO, cairo-directfb >= $CAIRO_REQUIRED_VERSION) > + PKG_CHECK_MODULES(GTK, gtk+-directfb-2.0 >= $GTK_REQUIRED_VERSION) > > ^ I Don't think we actually need to check for either of these packages, since > we don't actually use any cairo-directfb or gtk+-directfb-2.0 features > directly, even when DirectFB is the target. I don't mind strongly if we add > these checks though, your call. I added the checks since the .pc files nicely sets up the dependencies for us as well as location of directfb or x11 libraries. Patch coming. Created attachment 23873 [details]
Updated to address Alp's feedback
Created attachment 23882 [details]
only check for cairo-ft when doing non-directfb builds
Comment on attachment 23882 [details]
only check for cairo-ft when doing non-directfb builds
Looks sane. I will compile and land it.
Landed. I want to develop plug-in for swfdec on DirectFB/Webkit. X11/Webkit plug-in functionality callas xEvent,Visual,Disaplay functionality. Can I do the same way for DirectFB ? Anybody has tried DFB/WebKit plug-in support ? Manoj |