Summary: | [v2.1.2] GTK2 build fails for undefined GDK_IS_X11_DISPLAY | ||
---|---|---|---|
Product: | WebKit | Reporter: | Dominique Leuenberger <dimstar> |
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | berto, cgarcia, commit-queue, eflews.bot, gyuyoung.kim, kalevlember, mrobinson, zan |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | Linux | ||
Bug Depends on: | |||
Bug Blocks: | 118980 | ||
Attachments: |
Description
Dominique Leuenberger
2013-06-21 14:44:26 PDT
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. |