Bug 133871 - [GTK] Guard uses of RedirectedXCompositeWindow in WebKitWebViewBase with PLATFORM(X11)
Summary: [GTK] Guard uses of RedirectedXCompositeWindow in WebKitWebViewBase with PLAT...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-13 11:38 PDT by Zan Dobersek
Modified: 2014-07-08 11:57 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.21 KB, patch)
2014-06-13 11:54 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (5.02 KB, patch)
2014-07-08 00:12 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-06-13 11:38:05 PDT
[GTK] Guard uses of RedirectedXCompositeWindow in WebKitWebViewBase with PLATFORM(X11)
Comment 1 Zan Dobersek 2014-06-13 11:54:39 PDT
Created attachment 233065 [details]
Patch
Comment 2 WebKit Commit Bot 2014-06-13 11:56:56 PDT
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 3 Martin Robinson 2014-06-13 13:19:54 PDT
Comment on attachment 233065 [details]
Patch

I think we always need compile-time support for X11.
Comment 4 Gwang Yoon Hwang 2014-06-13 22:40:10 PDT
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.
Comment 5 Zan Dobersek 2014-06-17 01:45:03 PDT
(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.
Comment 6 Zan Dobersek 2014-06-17 01:46:54 PDT
(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.
Comment 7 Martin Robinson 2014-06-17 09:31:12 PDT
(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?
Comment 8 Zan Dobersek 2014-07-08 00:12:22 PDT
Created attachment 234551 [details]
Patch
Comment 9 Zan Dobersek 2014-07-08 09:26:44 PDT
(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 10 Zan Dobersek 2014-07-08 09:35:08 PDT
Comment on attachment 234551 [details]
Patch

Clearing flags on attachment: 234551

Committed r170887: <http://trac.webkit.org/changeset/170887>
Comment 11 Zan Dobersek 2014-07-08 09:35:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Martin Robinson 2014-07-08 09:56:46 PDT
(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.
Comment 13 Zan Dobersek 2014-07-08 11:57:17 PDT
(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.