Bug 119235 – EGLNativeWindowType/uint64_t compile error in GLContext

• Component: WebGL
• Status: RESOLVED FIXED

[Alex Christensen]

r117612 introduced using uint64_t as a native pointer type in Source/WebCore/platform/graphics/cairo/GLContext.h which is now Source/WebCore/platform/graphics/GLContext.h.  This causes a compiler error on Windows because it can't convert from uint64_t to a 32-bit EGLNativeWindowType (which is a HWND which is a void*) without a reinterpret_cast.  To avoid the evil that is reinterpret_cast, I propose a new typedef, GLNativeWindowType, which will be the same as EGLNativeWindowType if we're using EGL and it will be a uint64_t to not change any current behaviour.

[Alex Christensen]

Created attachment 207694 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 207694 [details]
Patch

Attachment 207694 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1279011

[Zan Dobersek]

I've struck the same problem while working on making GLContextEGL usable under the Wayland EGL platform.

I have an RFC patch uploaded in bug #118944 that shows the approach, basically having platform-specific classes that extend GLContextEGL, provide platform-specific shared display and also define the native window type.

I've only tested that patch on Wayland, X11 and the default EGL platforms (i.e. what's currently provided in that patch) but I believe a Windows-specific GLContextEGL could be introduced and used the same way.

I've talked to Martin about the patch and he gave the go-ahead to split the patch up into reviewable pieces and land the changes. Can we coordinate on the work being done here? I'd like to start uploading patches up for review this week and can commit some time to that right away, but would like to reduce the clashing with your work. I can either wait for you to set up GLContextEGL functionality on the Windows port and incorporate these changes into the refactoring that's proposed in bug #118944, or get the refactoring into the tree and you can continue after that. What would work best for you?

[Alex Christensen]

It looks like you're doing a lot more changes to this code than I am, Zan.  Everything that was there before worked fine except it assumed the compiler would convert between a pointer and a uint64_t.  Do you think your GLContextEGLDefault will work with the WinCairo port?

I won't have a way to test your changes with the state of the JavaScriptCore on Windows right now.  Go ahead and maybe you could fix this with your changes.

[Alex Christensen]

By the way, I'm trying to get WebGL working using EGL on the AppleWin port, too.  Do you think your changes will break that?

[Zan Dobersek]

The changes I'll be pushing through are meant to preserve the current behavior, that is keeping the X11 and the default EGL platforms functioning while also adding support for the Wayland EGL platform.

I think the AppleWin should use its own GLContextEGL subclass, just so it could provide an appropriate typedef for the native window type, but it should probably mimic the GLContextEGLDefault class in other aspects.

Anyway, while I still have to split my changes into smaller, reviewable patches, I'm still dependent on Martin who'll do all the reviewing. He's travelling at the moment, so I can't really guarantee that all the changes will get into trunk by e.g. tomorrow or the end of the week, even though I've been optimistic this would happen.

So I don't want to block you - I can easily wait another week or two if you think you can make good progress in the meantime, and I can include the AppleWin-specific bits in my refactoring.

[Alex Christensen]

This patch is not AppleWin specific.  EGL already has a native window type, EGLNativeWindowType.  I'm just using that for AppleWin and WinCairo.  I'll try to get this in soon so you can incorporate my small change into yours.

[Alex Christensen]

Created attachment 207748 [details]
Patch

[Zan Dobersek]

(In reply to comment #7)
> This patch is not AppleWin specific.  EGL already has a native window type, EGLNativeWindowType.  I'm just using that for AppleWin and WinCairo.  I'll try to get this in soon so you can incorporate my small change into yours.

I see. The problem is that this requires an inclusion of eglplatform.h. This causes problems for the GTK port which in future wants to support both the X11 and Wayland EGL platforms in the same build, but strictly setting the native window type makes this impossible. This was also the reason behind the refactoring of GLContextEGL I'm proposing, and I managed to solve that by defining the platform-specific native window type in the different GLContextEGL* headers.

Maybe that specific approach should be scaled over the GLContext class as well.

[Alex Christensen]

Do you think it would be better to put #if USE(EGL) && !PLATFORM(GTK) around my inclusion of eglplatform.h, teach GTK how to find eglplatform.h (which is in the ANGLE headers included in WebKit), or just wait for your changes?

[Zan Dobersek]

I think this patch is OK. I'd rather rebase and rework my changes with this already in the tree. So feel free to get this reviewed and landed.

[Alex Christensen]

Created attachment 207751 [details]
Patch

[Zan Dobersek]

Comment on attachment 207751 [details]
Patch

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

> Source/WebCore/platform/graphics/GLContext.h:33
> +#if USE(EGL) && !PLATFORM(GTK)
> +#include "eglplatform.h"
> +typedef EGLNativeWindowType GLNativeWindowType;
> +#else
> +typedef uint64_t GLNativeWindowType;
> +#endif

Works as well. Thanks.

[WebKit Commit Bot]

Comment on attachment 207751 [details]
Patch

Clearing flags on attachment: 207751

Committed r153492: <http://trac.webkit.org/changeset/153492>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.