Bug 133871 – [GTK] Guard uses of RedirectedXCompositeWindow in WebKitWebViewBase with PLATFORM(X11)

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GTK] Guard uses of RedirectedXCompositeWindow in WebKitWebViewBase with PLATFORM(X11)

[Zan Dobersek]

Created attachment 233065 [details]
Patch

[WebKit Commit Bot]

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

[Martin Robinson]

Comment on attachment 233065 [details]
Patch

I think we always need compile-time support for X11.

[Gwang Yoon Hwang]

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.

[Zan Dobersek]

(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.

[Zan Dobersek]

(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.

[Martin Robinson]

(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?

[Zan Dobersek]

Created attachment 234551 [details]
Patch

[Zan Dobersek]

(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.

[Zan Dobersek]

Comment on attachment 234551 [details]
Patch

Clearing flags on attachment: 234551

Committed r170887: <http://trac.webkit.org/changeset/170887>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

(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.

[Zan Dobersek]

(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.