Bug 224888

Created attachment 426951 [details] WIP Patch

<rdar://problem/77280211>

Created attachment 428411 [details] Patch

Comment on attachment 428411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428411&action=review Looks good to me overall - a few small comments. The real proof is whether things build with these changes. r+ > Source/ThirdParty/ANGLE/PlatformGTK.cmake:49 > + list(APPEND ANGLE_DEFINITIONS WL_EGL_PLATFORM) Understanding the comment above, still, won't some Wayland definitions be lost with this if/elseif construction if both X11 and Wayland are defined? Should that be fixed? > Source/ThirdParty/ANGLE/PlatformGTK.cmake:50 > + else () Wondering whether this should be something like if (!ENABLE_X11_TARGET && !ENABLE_WAYLAND_TARGET), and detached from this if/else chain. > Source/WebKit/CMakeLists.txt:320 > + # Prepend to make sure the ANGLE headers are found before system headers This kind of search path manipulation seemed fragile, which is why the code elsewhere in WebKit that calls into ANGLE on macOS uses more explicitly-scoped include paths, and processing is/was done when publishing these headers to the outside world. Not 100% sure if this is exactly the same issue, but something to think about. > Tools/Scripts/update-angle:169 > +./gni-to-cmake.py src/libANGLE/renderer/metal/BUILD.gn Metal.cmake --prepend 'src/libANGLE/renderer/metal/' Great to see these updates.

Comment on attachment 428411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428411&action=review >> Source/WebKit/CMakeLists.txt:320 >> + # Prepend to make sure the ANGLE headers are found before system headers > > This kind of search path manipulation seemed fragile, which is why the code elsewhere in WebKit that calls into ANGLE on macOS uses more explicitly-scoped include paths, and processing is/was done when publishing these headers to the outside world. Not 100% sure if this is exactly the same issue, but something to think about. Won't this cause the UI process to link to ANGLE? I think the UI process should not be allowed to link to ANGLE? This could cause weird issues in applications that don't expect to wind up using WebKit's special private OpenGL rather than mesa, right? I wonder what Adrian thinks.

Created attachment 429791 [details] Patch

Comment on attachment 428411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428411&action=review >> Source/ThirdParty/ANGLE/PlatformGTK.cmake:50 >> + else () > > Wondering whether this should be something like if (!ENABLE_X11_TARGET && !ENABLE_WAYLAND_TARGET), and detached from this if/else chain. This should be fixed in the patch for landing >>> Source/WebKit/CMakeLists.txt:320 >>> + # Prepend to make sure the ANGLE headers are found before system headers >> >> This kind of search path manipulation seemed fragile, which is why the code elsewhere in WebKit that calls into ANGLE on macOS uses more explicitly-scoped include paths, and processing is/was done when publishing these headers to the outside world. Not 100% sure if this is exactly the same issue, but something to think about. > > Won't this cause the UI process to link to ANGLE? I think the UI process should not be allowed to link to ANGLE? This could cause weird issues in applications that don't expect to wind up using WebKit's special private OpenGL rather than mesa, right? I wonder what Adrian thinks. This is potentially fragile BUT this shouldn't be a problem for GTK Michael. USE_ANGLE_EGL means use ANGLE as the OpenGL ES implementation for everything. So GTK won't hit that because it compiles with USE_ANGLE_WEBGL which just means use ANGLE for WebGL support and that's it.

Ok so I believe I have addressed all concerns with this patch. The GTK EWS for the last patch ran successfully with USE_ANGLE_WEBGL enabled. It had some local changes that aren't for upstream but were just to get a green build. I'm sure there's probably more changes for GTK but I feel this patch is good to go and anything else with getting GTK working should be done in a follow up.

Created attachment 429792 [details] Patch for Igalia to get started This contains the local changes to get EWS to complete successfully to continue investigation of turning on ANGLE for WebGL 2 on GTK.

Comment on attachment 429791 [details] Patch Latest patch still looks good to me.

(In reply to Don Olmstead from comment #14) > This is potentially fragile BUT this shouldn't be a problem for GTK Michael. > > USE_ANGLE_EGL means use ANGLE as the OpenGL ES implementation for > everything. So GTK won't hit that because it compiles with USE_ANGLE_WEBGL > which just means use ANGLE for WebGL support and that's it. OK, I agree this would only be a problem if USE_ANGLE_EGL were to be turned on.

Committed r278130 (238180@main): <https://commits.webkit.org/238180@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429791 [details].