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.
Created attachment 205313 [details] Patch
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.
Created attachment 205319 [details] Reformatted patch (replaced tabs with spaces)
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.
Created attachment 206333 [details] New version of patch - Reworked ChangeLog - added inclusion of autotoolsconfig.h (some defines moved there)
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.
Why do you need to include autotoolsconfig.h ? You shouldn't include it directly anyway, use config.h instead.
(In reply to comment #6) > This should probably go into GtkVersioning.h. You're right, I overlooked that file.
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.
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> ^
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.
(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
Created attachment 206350 [details] Updated patch - addresses the style issues reported Sorry for the many iterations.
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
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 :)
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?
(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)
(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.
(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
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?
Created attachment 206413 [details] Next iteration of patch after review by mrobinson...
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>
All reviewed patches have been landed. Closing bug.
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
(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.