[GTK] Guard uses of RedirectedXCompositeWindow in WebKitWebViewBase with PLATFORM(X11)
Created attachment 233065 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 233065 [details] Patch I think we always need compile-time support for X11.
Comment on attachment 233065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233065&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:435 > +#if PLATFORM(X11) && defined(GDK_WINDOWING_X11) As you mentioned in commit log, PLATFORM(X11) should imply GDK_WINDOWING_X11. So I think it is better to remove defined(GDK_WINDOWING_X11) from this patch.
(In reply to comment #3) > (From update of attachment 233065 [details]) > I think we always need compile-time support for X11. We do now, but this patch guards RedirectedXCompositeWindow that won't be used in builds for the Wayland target.
(In reply to comment #4) > (From update of attachment 233065 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233065&action=review > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:435 > > +#if PLATFORM(X11) && defined(GDK_WINDOWING_X11) > > As you mentioned in commit log, PLATFORM(X11) should imply GDK_WINDOWING_X11. > So I think it is better to remove defined(GDK_WINDOWING_X11) from this patch. OK, I'll enhance the CMake system to properly check for dependencies that would make 'PLATFORM(X11)' equivalent to 'PLATFORM(X11) && defined(GDK_WINDOWING_X11)', and then use PLATFORM(X11) only.
(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 233065 [details] [details]) > > I think we always need compile-time support for X11. > > We do now, but this patch guards RedirectedXCompositeWindow that won't be used in builds for the Wayland target. Can you talk more about builds that do not support X11, but do support Wayland?
Created attachment 234551 [details] Patch
(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #3) > > > (From update of attachment 233065 [details] [details] [details]) > > > I think we always need compile-time support for X11. > > > > We do now, but this patch guards RedirectedXCompositeWindow that won't be used in builds for the Wayland target. > > Can you talk more about builds that do not support X11, but do support Wayland? In Autotools we supported building for either X11 windowing target, Wayland windowing target, or both -- much like GTK+ can support different windowing targets. Of course we'll be checking that the GTK+ installation that's used was actually configured to enable the desired windowing target. The idea is to achieve the same with CMake. I can't really remember though why all these guards that are being added in this patch weren't necessary ~6 months ago.
Comment on attachment 234551 [details] Patch Clearing flags on attachment: 234551 Committed r170887: <http://trac.webkit.org/changeset/170887>
All reviewed patches have been landed. Closing bug.
(In reply to comment #9) > In Autotools we supported building for either X11 windowing target, Wayland windowing target, or both -- much like GTK+ can support different windowing targets. Of course we'll be checking that the GTK+ installation that's used was actually configured to enable the desired windowing target. I'm not sure of the benefit of making this configurable. My proposal is that we just target the same platforms that the GTK+ we are compiling against supports.
(In reply to comment #12) > (In reply to comment #9) > > > In Autotools we supported building for either X11 windowing target, Wayland windowing target, or both -- much like GTK+ can support different windowing targets. Of course we'll be checking that the GTK+ installation that's used was actually configured to enable the desired windowing target. > > I'm not sure of the benefit of making this configurable. My proposal is that we just target the same platforms that the GTK+ we are compiling against supports. That makes sense. Bug 134736.