[webkit-reviews] review granted: [Bug 224888] [CMake] Support USE_ANGLE_EGL on additional platforms : [Attachment 428411] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Don Olmstead
<don.olmstead at sony.com>'s request for review:
Bug 224888: [CMake] Support USE_ANGLE_EGL on additional platforms
https://bugs.webkit.org/show_bug.cgi?id=224888

Attachment 428411: Patch

https://bugs.webkit.org/attachment.cgi?id=428411&action=review




--- Comment #7 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 428411
  --> https://bugs.webkit.org/attachment.cgi?id=428411
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.