Bug 20287 – [Gtk] disable plugins for gtk/directfb target

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jan Alonzo]

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.

[Jan Alonzo]

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

[Holger Freyther]

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.

[Jan Alonzo]

(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.

[Jan Alonzo]

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.

[Eric Seidel (no email)]

Comment on attachment 23702 [details]
updated per holger's feedback, updated .pc checks and introduce WTF_PLATFORM_X11

Alp is your man...

[Alp Toker]

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.

[Jan Alonzo]

(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.

[Jan Alonzo]

Created attachment 23873 [details]
Updated to address Alp's feedback

[Jan Alonzo]

Created attachment 23882 [details]
only check for cairo-ft when doing non-directfb builds

[Holger Freyther]

Comment on attachment 23882 [details]
only check for cairo-ft when doing non-directfb builds

Looks sane. I will compile and land it.

[Holger Freyther]

Landed.

[Manoj]

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