Bug 145701 – [GTK] [Wayland] Should be possible to build with support for both X11 and Wayland.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

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.

[Carlos Alberto Lopez Perez]

Created attachment 254366 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 254368 [details]
Patch

[Zan Dobersek]

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.

[Carlos Alberto Lopez Perez]

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

[Zan Dobersek]

They're used by the pending patches in other bugs blocking bug #81456 and bug #115803.

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

Created attachment 254661 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 254661 [details]
Patch

Clearing flags on attachment: 254661

Committed r185453: <http://trac.webkit.org/changeset/185453>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145881

[Carlos Alberto Lopez Perez]

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.

[Carlos Alberto Lopez Perez]

Created attachment 254948 [details]
Patch

[Darin Adler]

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?

[Carlos Alberto Lopez Perez]

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

[WebKit Commit Bot]

Comment on attachment 254948 [details]
Patch

Clearing flags on attachment: 254948

Committed r185620: <http://trac.webkit.org/changeset/185620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.