Bug 120321 - [GTK][WK2] Only set up a RedirectedXCompositeWindow if running under an X11 display
Summary: [GTK][WK2] Only set up a RedirectedXCompositeWindow if running under an X11 d...
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: 2013-08-26 11:27 PDT by Zan Dobersek
Modified: 2013-09-02 05:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.10 KB, patch)
2013-08-26 11:34 PDT, Zan Dobersek
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2013-08-26 11:27:43 PDT
[GTK][WK2] Only set up a RedirectedXCompositeWindow if running under an X11 display
Comment 1 Zan Dobersek 2013-08-26 11:34:50 PDT
Created attachment 209667 [details]
Patch
Comment 2 WebKit Commit Bot 2013-08-26 11:37:09 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 Gustavo Noronha (kov) 2013-08-26 12:41:10 PDT
Comment on attachment 209667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209667&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:60
> +#ifdef GDK_WINDOWING_X11

I've been pondering that we should probably have a macro that tests that both PLATFORM(X11) and GDK_WINDOWING_X11 are present for these, what do you think?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:387
> +    GdkDisplay* display = gdk_display_manager_get_default_display(gdk_display_manager_get());
> +    if (GDK_IS_X11_DISPLAY(display)) {
> +        priv->redirectedWindow = RedirectedXCompositeWindow::create(IntSize(1, 1), RedirectedXCompositeWindow::DoNotCreateGLContext);
> +        if (priv->redirectedWindow)
> +            priv->redirectedWindow->setDamageNotifyCallback(redirectedWindowDamagedCallback, object);
> +    }

Need the preprocessor check here as well.
Comment 4 Zan Dobersek 2013-08-27 01:38:47 PDT
Comment on attachment 209667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209667&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:60
>> +#ifdef GDK_WINDOWING_X11
> 
> I've been pondering that we should probably have a macro that tests that both PLATFORM(X11) and GDK_WINDOWING_X11 are present for these, what do you think?

I think we could test for PLATFORM(X11) instead of GDK_WINDOWING_X11 if we explicitly test for gdk-x11-3.0.pc when building for the X11 target. I believe that finding the gdk-x11 package indirectly assures that GDK_WINDOWING_X11 is defined, though we can add additional checks for this in the configuration step.

I'd rather be using PLATFORM(X11) than GDK_WINDOWING_X11 simply because it's more WebKit-like. I know that PLATFORM(X11) is also applicable to other ports, but the `#ifdef GDK_WINDOWING_X11` checks are still GTK-specific, so using PLATFORM(X11) in their place shouldn't break anything.

Similar would apply for GDK_WINDOWING_WAYLAND and PLATFORM(WAYLAND).
Comment 5 Zan Dobersek 2013-08-27 02:17:04 PDT
This patch enables running UIProcesses under Wayland even when building with accelerated compositing enabled - before, the process would assert due to an invalid cast from GdkWaylandDisplay to GdkX11Display.

The accelerated compositing is enabled by default in WK2 though, so page rendering under Wayland goes wrong whenever it switches to the accelerated compositing mode.

To avoid that, I'm at the moment disabling accelerated compositing under Wayland displays in webkitWebViewGroupAttachSettingsToPageGroup. Is that feasible/sane? It works, and it's needed, but it's kind of a dirty hack. Bug #120347 has the patch.
Comment 6 Zan Dobersek 2013-08-28 02:44:08 PDT
Committed r154729: <http://trac.webkit.org/changeset/154729>
Comment 7 Gustavo Noronha (kov) 2013-09-02 05:18:10 PDT
(In reply to comment #4)
> I think we could test for PLATFORM(X11) instead of GDK_WINDOWING_X11 if we explicitly test for gdk-x11-3.0.pc when building for the X11 target. I believe that finding the gdk-x11 package indirectly assures that GDK_WINDOWING_X11 is defined, though we can add additional checks for this in the configuration step.
> 

I think that would be better, I'm thinking about a flying-cars future where X is disabled and only wayland is enabled, though. I guess we can cross that bridge when we get to it, anyway.