Bug 20287 - [Gtk] disable plugins for gtk/directfb target
Summary: [Gtk] disable plugins for gtk/directfb target
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2008-08-05 04:17 PDT by Jan Alonzo
Modified: 2009-02-16 10:27 PST (History)
0 users

See Also:


Attachments
Patch to fix this issue - includes directfb build fixes too. (8.56 KB, patch)
2008-09-20 23:04 PDT, Jan Alonzo
zecke: review-
Details | Formatted Diff | Diff
updated per holger's feedback, updated .pc checks and introduce WTF_PLATFORM_X11 (9.68 KB, patch)
2008-09-23 05:09 PDT, Jan Alonzo
alp: review-
Details | Formatted Diff | Diff
Updated to address Alp's feedback (8.25 KB, patch)
2008-09-27 00:34 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
only check for cairo-ft when doing non-directfb builds (8.30 KB, patch)
2008-09-27 16:18 PDT, Jan Alonzo
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Alonzo 2008-08-05 04:17:40 PDT
Speaking to barisione and marcoil, it would be better to disable plugin support for the gtk/directfb port as this is not supported at the moment.
Comment 1 Jan Alonzo 2008-09-20 23:04:56 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 2 Holger Freyther 2008-09-21 19:21:35 PDT
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.
Comment 3 Jan Alonzo 2008-09-22 06:08:51 PDT
(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.
Comment 4 Jan Alonzo 2008-09-23 05:09:31 PDT
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 5 Eric Seidel (no email) 2008-09-24 16:00:42 PDT
Comment on attachment 23702 [details]
updated per holger's feedback, updated .pc checks and introduce WTF_PLATFORM_X11

Alp is your man...
Comment 6 Alp Toker 2008-09-24 17:33:39 PDT
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.
Comment 7 Jan Alonzo 2008-09-27 00:33:49 PDT
(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.
Comment 8 Jan Alonzo 2008-09-27 00:34:45 PDT
Created attachment 23873 [details]
Updated to address Alp's feedback
Comment 9 Jan Alonzo 2008-09-27 16:18:05 PDT
Created attachment 23882 [details]
only check for cairo-ft when doing non-directfb builds
Comment 10 Holger Freyther 2008-09-27 18:05:51 PDT
Comment on attachment 23882 [details]
only check for cairo-ft when doing non-directfb builds

Looks sane. I will compile and land it.
Comment 11 Holger Freyther 2008-09-27 18:55:19 PDT
Landed.
Comment 12 Manoj 2009-02-16 10:27:47 PST
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