Bug 145701 - [GTK] [Wayland] Should be possible to build with support for both X11 and Wayland.
Summary: [GTK] [Wayland] Should be possible to build with support for both X11 and Way...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Carlos Alberto Lopez Perez
URL:
Keywords:
Depends on: 145881
Blocks: 81456 146057
  Show dependency treegraph
 
Reported: 2015-06-05 10:29 PDT by Carlos Alberto Lopez Perez
Modified: 2015-06-17 05:10 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.91 KB, patch)
2015-06-05 11:14 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (8.90 KB, patch)
2015-06-05 11:20 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (4.86 KB, patch)
2015-06-10 10:12 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2015-06-16 05:21 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2015-06-05 10:29:04 PDT
It should be possible to build with support for both X11 and Wayland targets.

This was possible before r183491 <http://trac.webkit.org/r183491> that added the conflicting option in CMake.
Comment 1 Carlos Alberto Lopez Perez 2015-06-05 11:14:18 PDT
Created attachment 254366 [details]
Patch
Comment 2 Carlos Alberto Lopez Perez 2015-06-05 11:20:20 PDT
Created attachment 254368 [details]
Patch
Comment 3 Zan Dobersek 2015-06-06 07:11:36 PDT
This is where things start to complicate. The patch removes support for GLContext usage with the Wayland EGL platform.

If we all agree on prioritizing support for both X11 and Wayland display protocols in the same build, then this patch can proceed.
Comment 4 Carlos Alberto Lopez Perez 2015-06-08 05:53:38 PDT
(In reply to comment #3)
> The patch removes support for GLContext usage with the Wayland EGL platform.

But are this removed functions used anywhere? It seems dead code to me.
Comment 5 Zan Dobersek 2015-06-08 11:58:01 PDT
They're used by the pending patches in other bugs blocking bug #81456 and bug #115803.
Comment 6 Carlos Alberto Lopez Perez 2015-06-09 09:55:52 PDT
(In reply to comment #5)
> They're used by the pending patches in other bugs blocking bug #81456 and
> bug #115803.

I see, WebCore::WaylandSurface::createGLContext is used by patch on bug 136832 and WebCore::PlatformDisplayWayland::createSharingGLContext by patch on bug 136831.

So, I will try to enable building both targets without removing the above functions.
Comment 7 Carlos Alberto Lopez Perez 2015-06-10 10:12:54 PDT
Created attachment 254661 [details]
Patch
Comment 8 WebKit Commit Bot 2015-06-11 08:21:42 PDT
Comment on attachment 254661 [details]
Patch

Clearing flags on attachment: 254661

Committed r185453: <http://trac.webkit.org/changeset/185453>
Comment 9 WebKit Commit Bot 2015-06-11 08:21:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 WebKit Commit Bot 2015-06-11 08:56:40 PDT
Re-opened since this is blocked by bug 145881
Comment 11 Carlos Alberto Lopez Perez 2015-06-11 09:01:11 PDT
Fails to build on 32-bit :\

https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/53334/steps/compile-webkit/logs/stdio
../../Source/WebCore/platform/graphics/GLContext.cpp: In static member function ‘static std::unique_ptr<WebCore::GLContext> WebCore::GLContext::createContextForWindow(GLNativeWindowType, WebCore::GLContext*)’:
../../Source/WebCore/platform/graphics/GLContext.cpp:126:89: error: invalid cast from type ‘GLNativeWindowType {aka long long unsigned int}’ to type ‘XID {aka long unsigned int}’
     if (auto glxContext = GLContextGLX::createContext(reinterpret_cast<XID>(windowHandle), sharingContext))


I'm reverting it now, and will try later to find a solution that works also for 32-bits.
Comment 12 Carlos Alberto Lopez Perez 2015-06-16 05:21:34 PDT
Created attachment 254948 [details]
Patch
Comment 13 Darin Adler 2015-06-16 10:45:56 PDT
Comment on attachment 254948 [details]
Patch

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

> Source/WebCore/platform/graphics/GLContext.cpp:130
> +#if PLATFORM(WAYLAND) // Building both X11 and Wayland targets
> +    XID GLXWindowHandle = reinterpret_cast<XID>(windowHandle);
> +#else
> +    XID GLXWindowHandle = static_cast<XID>(windowHandle);
> +#endif

Would it be a problem to always do reinterpret_cast?
Comment 14 Carlos Alberto Lopez Perez 2015-06-16 11:15:15 PDT
(In reply to comment #13)
> Comment on attachment 254948 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254948&action=review
> 
> > Source/WebCore/platform/graphics/GLContext.cpp:130
> > +#if PLATFORM(WAYLAND) // Building both X11 and Wayland targets
> > +    XID GLXWindowHandle = reinterpret_cast<XID>(windowHandle);
> > +#else
> > +    XID GLXWindowHandle = static_cast<XID>(windowHandle);
> > +#endif
> 
> Would it be a problem to always do reinterpret_cast?

Yes. 

That was I tried on r185453 <http://trac.webkit.org/changeset/185453>. But it caused a build failure on 32-bits.

As far as I understand this, the issue is that on the GTK port, when PLATFORM(Wayland) is not enabled we define GLNativeWindowType as uint64_t, and XID is a uintptr_t. And the compiler is not happy doing a reinterpret_cast from a 64bit uint to a 32bit one. However, doing a static_cast is ok (or an implicit cast by assignment like it was done before this patch) because we simply truncate the value if it don't fits on a 32bit uint. That shouldn't be a problem because the value of GLNativeWindowType should never be larger than a 32bit uint on a 32-bit platform.
Comment 15 WebKit Commit Bot 2015-06-16 15:19:20 PDT
Comment on attachment 254948 [details]
Patch

Clearing flags on attachment: 254948

Committed r185620: <http://trac.webkit.org/changeset/185620>
Comment 16 WebKit Commit Bot 2015-06-16 15:19:26 PDT
All reviewed patches have been landed.  Closing bug.