Bug 117895 – [v2.1.2] GTK2 build fails for undefined GDK_IS_X11_DISPLAY

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Dominique Leuenberger]

Trying to build webkitgtk 2.1.2 for GTK2 fails with

[ 1578s] Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp: In function 'WTF::OwnPtr<WebCore::WidgetBackingStore> WebKit::createBackingStore(GtkWidget*, const WebCore::IntSize&)':
[ 1578s] Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:111:35: error: 'GDK_IS_X11_DISPLAY' was not declared in this scope
[ 1580s] make[1]: *** [Source/WebKit/gtk/WebCoreSupport/libwebkitgtk_1_0_la-ChromeClientGtk.lo] Error 1
[ 1580s] make[1]: Leaving directory `/home/abuild/rpmbuild/BUILD/webkitgtk-2.1.2'
[ 1580s] make: *** [all] Error 2
[ 1580s] error: Bad exit status from /var/tmp/rpm-tmp.MOddiR (%build)

(A full build log can be found at https://build.opensuse.org/package/live_build_log?arch=x86_64&package=webkitgtk&project=home%3Adimstar%3Abranches%3AGNOME%3AFactory&repository=openSUSE_Factory )

GDK_IS_X11_DISPLAY is not defined in GTK+ 2.x.

[Dominique Leuenberger]

Created attachment 205313 [details]
Patch

[WebKit Commit Bot]

Attachment 205313 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp']" exit_code: 1
Source/WebKit/gtk/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominique Leuenberger]

Created attachment 205319 [details]
Reformatted patch (replaced tabs with spaces)

[Alberto Garcia]

Thanks for the patch!

Can you please rework a bit the ChangeLog entry? I should be
something like this:

   Bug Title
   Bug URL

   Reviewed by NOBODY (OOPS!).

   Long description.

   * WebCoreSupport/ChromeClientGtk.cpp:

About the change itself, I think it's fine. As a minor thing, you can
also put everything in one line, e.g.

   #if defined(GDK_WINDOWING_X11) && !defined(GDK_IS_X11_DISPLAY)
   ...

I think it would be a good idea to have all the compatibility code
together in the same file, but we don't have that at the moment.

[Dominique Leuenberger]

Created attachment 206333 [details]
New version of patch - Reworked ChangeLog - added inclusion of autotoolsconfig.h (some defines moved there)

[Martin Robinson]

Comment on attachment 206333 [details]
New version of patch - Reworked ChangeLog - added inclusion of autotoolsconfig.h (some defines moved there)

View in context: https://bugs.webkit.org/attachment.cgi?id=206333&action=review

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:106
> +// GTK 2.0 compatibility
> +#ifdef GDK_WINDOWING_X11 && !defined(GDK_IS_X11_DISPLAY)
> +#define GDK_IS_X11_DISPLAY(dpy) 1
> +#endif

This should probably go into GtkVersioning.h.

[Alberto Garcia]

Why do you need to include autotoolsconfig.h ? You shouldn't include
it directly anyway, use config.h instead.

[Alberto Garcia]

(In reply to comment #6)
> This should probably go into GtkVersioning.h.

You're right, I overlooked that file.

[Dominique Leuenberger]

Thanks for your comments guys...

anyway, version 2.1.3 (recently released) has now even more issues... so this patch has been rendered useless already :)

I'll try to fix up the build of 2.1.3... then we can try to fixup 'formatting' for inclusion into the svn tree. I'll work based on your comments.

[Dominique Leuenberger]

Created attachment 206349 [details]
New version, hopefully translated all comments into the right snippets. Patch build-tested with  webkitgtk 2.1.3 / gtk2 2.24.19

Build failures addressed in new patch:

1)
Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:749:58: error: 'gtk_widget_get_preferred_size' was not declared in this scope
     gtk_widget_get_preferred_size(widget, &requisition, 0);


2)
Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp: In function 'WTF::PassOwnPtr<WebCore::WidgetBackingStore> WebKit::createBackingStore(GtkWidget*, const WebCore::IntSize&)':
Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:111:35: error: 'GDK_IS_X11_DISPLAY' was not declared in this scope
     if (GDK_IS_X11_DISPLAY(display))

3)
In file included from Source/WebCore/plugins/gtk/gtk2xtbin.c:47:0:
Source/WebCore/plugins/gtk/gtk2xtbin.h:45:22: fatal error: gtk/gtkx.h: No such file or directory
 #include <gtk/gtkx.h>
                      ^

[WebKit Commit Bot]

Attachment 206349 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/gtk/GtkVersioning.h', u'Source/WebCore/plugins/gtk/gtk2xtbin.h']" exit_code: 1
Source/WebCore/platform/gtk/GtkVersioning.h:44:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/gtk/GtkVersioning.h:46:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/gtk/GtkVersioning.h:46:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/plugins/gtk/gtk2xtbin.h:43:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominique Leuenberger]

(In reply to comment #11)
> Attachment 206349 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/gtk/GtkVersioning.h', u'Source/WebCore/plugins/gtk/gtk2xtbin.h']" exit_code: 1
> Source/WebCore/platform/gtk/GtkVersioning.h:44:  Missing space after ,  [whitespace/comma] [3]
> Source/WebCore/platform/gtk/GtkVersioning.h:46:  Tab found; better to use spaces  [whitespace/tab] [1]
> Source/WebCore/platform/gtk/GtkVersioning.h:46:  Extra space before ( in function call  [whitespace/parens] [4]

Ok, those three I can easily address

> Source/WebCore/plugins/gtk/gtk2xtbin.h:43:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
> Total errors found: 4 in 3 files

??? That contradicts what was said earlier

[Dominique Leuenberger]

Created attachment 206350 [details]
Updated patch - addresses the style issues reported

Sorry for the many iterations.

[EFL EWS Bot]

Comment on attachment 206350 [details]
Updated patch - addresses the style issues reported

Attachment 206350 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/984271

[Dominique Leuenberger]

Created attachment 206394 [details]
Another round - taking mrobinson's comment into account, that code comment should be sentences...

Let's see what else we can fix :)

[Martin Robinson]

Comment on attachment 206394 [details]
Another round - taking mrobinson's comment into account, that code comment should be sentences...

View in context: https://bugs.webkit.org/attachment.cgi?id=206394&action=review

> Source/WebCore/platform/gtk/GtkVersioning.h:53
> +#if defined(GDK_WINDOWING_X11) && !defined(GDK_IS_X11_DISPLAY)
> +#define GDK_IS_X11_DISPLAY(dpy) 1
> +#endif
> +

Perhaps this should be 
#if defined(GDK_WINDOWING_X11) && !defined(GDK_IS_X11_DISPLAY)
#define GDK_IS_X11_DISPLAY(display) 1
#else
#define GDK_IS_X11_DISPLAY(display) 0
#endif

for Windows and Mac?

[Dominique Leuenberger]

(In reply to comment #16)
> (From update of attachment 206394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206394&action=review
> 
> > Source/WebCore/platform/gtk/GtkVersioning.h:53
> > +#if defined(GDK_WINDOWING_X11) && !defined(GDK_IS_X11_DISPLAY)
> > +#define GDK_IS_X11_DISPLAY(dpy) 1
> > +#endif
> > +
> 
> Perhaps this should be 
> #if defined(GDK_WINDOWING_X11) && !defined(GDK_IS_X11_DISPLAY)
> #define GDK_IS_X11_DISPLAY(display) 1
> #else
> #define GDK_IS_X11_DISPLAY(display) 0
> #endif
> 
> for Windows and Mac?

Maybe... I honestly would not know if that's the right behavior or what the consequences are in this case :(

Question would probably be: are there webkit2/gtk2 windows/mac builds (I guess yes.. but then, seeing on how long and how often Gtk2 builds fail, I'm not sure)

[Martin Robinson]

(In reply to comment #17)

> Question would probably be: are there webkit2/gtk2 windows/mac builds (I guess yes.. but then, seeing on how long and how often Gtk2 builds fail, I'm not sure)


Well for sure there's no WebKit2 support for GTK+ 2 at all. WebKit1 works on both Windows and Mac (at various points in time). There is a small group of people who actively fix problems with Windows. I've pointed them here.

[Kalev Lember]

(In reply to comment #17)
> (In reply to comment #16)
> > Perhaps this should be 
> > #if defined(GDK_WINDOWING_X11) && !defined(GDK_IS_X11_DISPLAY)
> > #define GDK_IS_X11_DISPLAY(display) 1
> > #else
> > #define GDK_IS_X11_DISPLAY(display) 0
> > #endif
> > 
> > for Windows and Mac?
> 
> Maybe... I honestly would not know if that's the right behavior or what the consequences are in this case :(

It should be fine to leave it undefined for other platforms.

In all the places that use it [1], GDK_IS_X11_DISPLAY() calls are always guarded with #ifdef GDK_WINDOWING_X11. As GTK+ 2 doesn't support multiple GDK backends, we can be sure that when GDK_WINDOWING_X11 is defined, it's an X11 build.

[1] ChromeClientGtk.cpp and BackingStoreCairo.cpp

[Kalev Lember]

Perhaps it would make sense to surround the GTK+ 2 workarounds with

#ifdef GTK_API_VERSION_2
...
#endif

to make sure it's easy to find them when GTK+ 2 support gets removed in the future?

[Dominique Leuenberger]

Created attachment 206413 [details]
Next iteration of patch after review by mrobinson...

[WebKit Commit Bot]

Comment on attachment 206413 [details]
Next iteration of patch after review by mrobinson...

Clearing flags on attachment: 206413

Committed r152552: <http://trac.webkit.org/changeset/152552>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Comment on attachment 206413 [details]
Next iteration of patch after review by mrobinson...

View in context: https://bugs.webkit.org/attachment.cgi?id=206413&action=review

> Source/WebCore/platform/gtk/GtkVersioning.h:56
> +// Define GDK_IS_X11_DISPLAY dummy for GTK+ 2.0 compatibility.
> +#ifndef GDK_IS_X11_DISPLAY
> +  #ifdef GDK_WINDOWING_X11
> +    #define GDK_IS_X11_DISPLAY(display) 1
> +  #else
> +    #define GDK_IS_X11_DISPLAY(display) 0
> +  #endif
> +#endif

I don't think this is the right fix, this depends on whether GDK_IS_X11_DISPLAY is defined at this point. GDK_IS_X11_DISPLAY is defined in gdk/gdkx.h in GTK+3 and that header is included in GtkVersioning.cpp. I think we should either check for a explicit version here or use GTK_API_VERSION_2 macro. This is causing compile warnings, see:

In file included from /home/cgarcia/src/git/WebKit/WebKitBuild/Dependencies/Root/include/gtk-3.0/gdk/gdkx.h:43:0,
                 from ../../Source/WebCore/platform/gtk/GtkVersioning.c:28:
/home/cgarcia/src/git/WebKit/WebKitBuild/Dependencies/Root/include/gtk-3.0/gdk/x11/gdkx11display.h:49:0: warning: "GDK_IS_X11_DISPLAY" redefined [enabled by default]
In file included from ../../Source/WebCore/platform/gtk/GtkVersioning.c:22:0:
../../Source/WebCore/platform/gtk/GtkVersioning.h:52:0: note: this is the location of the previous definition

[Zan Dobersek]

(In reply to comment #24)
> (From update of attachment 206413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206413&action=review
> 
> > Source/WebCore/platform/gtk/GtkVersioning.h:56
> > +// Define GDK_IS_X11_DISPLAY dummy for GTK+ 2.0 compatibility.
> > +#ifndef GDK_IS_X11_DISPLAY
> > +  #ifdef GDK_WINDOWING_X11
> > +    #define GDK_IS_X11_DISPLAY(display) 1
> > +  #else
> > +    #define GDK_IS_X11_DISPLAY(display) 0
> > +  #endif
> > +#endif
> 
> I don't think this is the right fix, this depends on whether GDK_IS_X11_DISPLAY is defined at this point. GDK_IS_X11_DISPLAY is defined in gdk/gdkx.h in GTK+3 and that header is included in GtkVersioning.cpp. I think we should either check for a explicit version here or use GTK_API_VERSION_2 macro. This is causing compile warnings, see:
> 
> In file included from /home/cgarcia/src/git/WebKit/WebKitBuild/Dependencies/Root/include/gtk-3.0/gdk/gdkx.h:43:0,
>                  from ../../Source/WebCore/platform/gtk/GtkVersioning.c:28:
> /home/cgarcia/src/git/WebKit/WebKitBuild/Dependencies/Root/include/gtk-3.0/gdk/x11/gdkx11display.h:49:0: warning: "GDK_IS_X11_DISPLAY" redefined [enabled by default]
> In file included from ../../Source/WebCore/platform/gtk/GtkVersioning.c:22:0:
> ../../Source/WebCore/platform/gtk/GtkVersioning.h:52:0: note: this is the location of the previous definition

Bug #118980.