Bug 197755

<rdar://problem/50642205>

Created attachment 369534 [details] Patch

This is a resurrection of a several-year-old prototype of this that at one point worked except drawing to the screen from what I could tell. It currently compiles, runs, and asserts when you do anything with webgl. This would be a good starting point for further prototyping.

I'm upstreaming the build system part of this in https://bugs.webkit.org/show_bug.cgi?id=197787

Created attachment 369780 [details] Patch

Comment on attachment 369780 [details] Patch There we go. It builds on Mac. That's as far as I plan to take this right now. Happy prototyping.

Comment on attachment 369780 [details] Patch Attachment 369780 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12181249 Number of test failures exceeded the failure limit.

Created attachment 369786 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 369780 [details] Patch Attachment 369780 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12181307 Number of test failures exceeded the failure limit.

Created attachment 369787 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 369780 [details] Patch Attachment 369780 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12181352 Number of test failures exceeded the failure limit.

Created attachment 369793 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6

Linking to most recent ANGLE roll, which was needed for this work.

Could you explain, what's the point of having dedicated ANGLE backend when we already have OpenGL ES backend which can work with ANGLE in the same way as with other OpenGL ES implementations, and will have to stay because embedded Linux systems use OpenGL ES?

(In reply to Konstantin Tokarev from comment #14) > Could you explain, what's the point of having dedicated ANGLE backend when > we already have OpenGL ES backend which can work with ANGLE in the same way > as with other OpenGL ES implementations, and will have to stay because > embedded Linux systems use OpenGL ES? Because ANGLE will do the WebGL validation for us. As long as it isn't a significant performance hit to go from WebGL -> OpenGLES to WebGL -> ANGLE -> OpenGLES then we'll be able to remove code in WebKit. Also, ANGLE will allow us to implement WebGL 2 with much less effort.

(In reply to Dean Jackson from comment #15) > (In reply to Konstantin Tokarev from comment #14) > > Could you explain, what's the point of having dedicated ANGLE backend when > > we already have OpenGL ES backend which can work with ANGLE in the same way > > as with other OpenGL ES implementations, and will have to stay because > > embedded Linux systems use OpenGL ES? > > Because ANGLE will do the WebGL validation for us. As long as it isn't a > significant performance hit to go from WebGL -> OpenGLES to WebGL -> ANGLE > -> OpenGLES then we'll be able to remove code in WebKit. > > Also, ANGLE will allow us to implement WebGL 2 with much less effort. It would be nice to collapse the GL stuff so its just targeting ES. We could really cut down the class hierarchy and general complexity there. For linux there's vulkan support so that would potentially be nice for GTK or WPE assuming the hardware support is there. With WinCairo using ANGLE already that's not a big concern for us there. With PlayStation we have an ES implementation and don't support WebGL outside of some legacy system apps so if this work moves forward I'd want a way to just use that rather than wrapping ES in ANGLE. Maybe this is a similar concern for WPE? In the long term though how might this play with WebGPU? We're very interested in adding support for that outside of Apple ports once we're fully in trunk and moving to using that for rendering.

How to implement WebGPU on multiple platforms is still an unknown. We've made as much as possible platform independent, but there is still a lot of Metal stuff in the GPU* classes.

(In reply to Don Olmstead from comment #16) > With WinCairo using ANGLE already that's not a big concern for us there. > With PlayStation we have an ES implementation and don't support WebGL > outside of some legacy system apps so if this work moves forward I'd want a > way to just use that rather than wrapping ES in ANGLE. Maybe this is a > similar concern for WPE? In the long term we hope to remove the security checks in WebCore because they will be redundant with security checks in ANGLE. WebGL implementations in WebKit will be just thin wrappers around ANGLE, which can wrap GLES. Either way, there will need to be something wrapping the OpenGL(ES) implementation doing security checks, we are just planning to move those checks to ANGLE and simplify WebCore. (In reply to Dean Jackson from comment #17) > How to implement WebGPU on multiple platforms is still an unknown. We've > made as much as possible platform independent, but there is still a lot of > Metal stuff in the GPU* classes. ...but WebGPU is designed to be implementable on other platforms so it should be possible.

Created attachment 371212 [details] Patch

Created attachment 371576 [details] 1. Add preliminary ANGLE backend to WebCore

(In reply to Kenneth Russell from comment #20) > Created attachment 371576 [details] > 1. Add preliminary ANGLE backend to WebCore I would have thought we would just have a GraphicsContext3D backed by only OpenGL ES and have ANGLE used as the ES implementations for all ports.

(In reply to Don Olmstead from comment #21) > (In reply to Kenneth Russell from comment #20) > > Created attachment 371576 [details] > > 1. Add preliminary ANGLE backend to WebCore > > I would have thought we would just have a GraphicsContext3D backed by only > OpenGL ES and have ANGLE used as the ES implementations for all ports. Trying to call ANGLE's publicly exposed OpenGL ES API entry points will probably cause symbol collision problems with the system's OpenGL ES libraries on iOS. I'm not sure, but this is probably the reason the WebKit team started this prototype by calling into ANGLE's internal namespaced entry points. This seems like a reasonable solution, and in conversations with Dean, he and I both preferred to bring up a separate ANGLE backend using this approach and then switch the ports over one by one. Does this sound OK to you? Fixing the iOS build issue in the above patch now.

Created attachment 371764 [details] 1. Add preliminary ANGLE backend to WebCore

Created attachment 371784 [details] 1. Add preliminary ANGLE backend to WebCore

Sorry for the EWS churn. I've now followed the instructions to set up the iOS Simulator build and the next patch will fix the compilation errors on that platform.

Created attachment 371798 [details] 1. Add preliminary ANGLE backend to WebCore

Comment on attachment 371798 [details] 1. Add preliminary ANGLE backend to WebCore Attachment 371798 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12439740 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla apiTests

Comment on attachment 371798 [details] 1. Add preliminary ANGLE backend to WebCore View in context: https://bugs.webkit.org/attachment.cgi?id=371798&action=review This is great. Thanks Ken. I'm happy to land this now and start incrementally getting things working. > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:2615 > + shellScript = "# Type a script or drag a script file from your workspace to insert its path.\n/bin/sh $SRCROOT/adjust-angle-include-paths.sh\n"; Could you remove the comment line and just have the part that calls the shell script? > Source/ThirdParty/ANGLE/adjust-angle-include-paths.sh:38 > +' $i > $i.new > + mv $i.new $i You could do sed -i '' to edit the file in place and avoid the mv and .new steps. > Source/WebCore/SourcesCocoa.txt:225 > +platform/graphics/angle/Extensions3DANGLE.cpp @no-unify > +platform/graphics/angle/GraphicsContext3DANGLE.cpp @no-unify > +platform/graphics/angle/TemporaryANGLESetting.cpp @no-unify It's probably fine to unify these sources, but that's not important for this patch. I've tried to unify the few remaining WebGL files a few times but keep making mistakes and getting rolled out, so I can just add these to that job. Edit: Oh I see. You can't unify them in case they get merged with files that use the system OpenGL. So we'll address this later. > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:89 > + // Enable support in ANGLE (if not enabled already) Nit: End comment with . (same with other comments in this function) Actually, maybe we don't even need these comments? > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:94 > + compiler.setResources(ANGLEResources); Is getting/setting the resources expensive? If not, it might read easier if this function just does something like: ANGLEWebKitBridge& compiler = m_context->m_compiler; ShBuiltInResources ANGLEResources = compiler.getResources(); if (name == "GL_OES_standard_derivatives") ANGLEResources.OES_standard_derivatives = 1; else if .... compiler.setResources(ANGLEResources); > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:128 > + ANGLEWebKitBridge& compiler = m_context->m_compiler; > + return compiler.getResources().OES_standard_derivatives; I wouldn't bother with the local variable here. > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:154 > + return ""; // Invalid shader type. Use emptyString(); > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:159 > + return ""; Use emptyString(); > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:182 > + return ""; emptyString(); > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:69 > + // EXT Robustness - uses getGraphicsResetStatusARB() > + void readnPixelsEXT(int x, int y, GC3Dsizei width, GC3Dsizei height, GC3Denum format, GC3Denum type, GC3Dsizei bufSize, void *data) override; > + void getnUniformfvEXT(GC3Duint program, int location, GC3Dsizei bufSize, float *params) override; > + void getnUniformivEXT(GC3Duint program, int location, GC3Dsizei bufSize, int *params) override; Should we just leave these out for now since they aren't implemented? > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:88 > + // Weak pointer back to GraphicsContext3D Nit: End with . > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:33 > +#include "GraphicsContext3DIOS.h" I really need to get rid of this horrible hack. Adopting ANGLE will definitely help! > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:277 > + // clamp the maximum to 8192, which is big enough for a 5K display. Oops. We just announced a 6K display :(

The test failure is unrelated. Ken, if you upload a new patch I'll land it for you. Thanks again!

Comment on attachment 371798 [details] 1. Add preliminary ANGLE backend to WebCore View in context: https://bugs.webkit.org/attachment.cgi?id=371798&action=review >> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:2615 >> + shellScript = "# Type a script or drag a script file from your workspace to insert its path.\n/bin/sh $SRCROOT/adjust-angle-include-paths.sh\n"; > > Could you remove the comment line and just have the part that calls the shell script? Yes. Done. >> Source/ThirdParty/ANGLE/adjust-angle-include-paths.sh:38 >> + mv $i.new $i > > You could do sed -i '' to edit the file in place and avoid the mv and .new steps. Thanks, done. >> Source/WebCore/SourcesCocoa.txt:225 >> +platform/graphics/angle/TemporaryANGLESetting.cpp @no-unify > > It's probably fine to unify these sources, but that's not important for this patch. I've tried to unify the few remaining WebGL files a few times but keep making mistakes and getting rolled out, so I can just add these to that job. > > Edit: Oh I see. You can't unify them in case they get merged with files that use the system OpenGL. So we'll address this later. Right. I've added a comment indicating the reason they can't be unified. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:89 >> + // Enable support in ANGLE (if not enabled already) > > Nit: End comment with . (same with other comments in this function) > Actually, maybe we don't even need these comments? Updated these comments - this file is essentially a vanilla copy of Extensions3DOpenGLCommon and Extensions3DOpenGL combined, so I'd rather minimize the number of changes in the initial commit. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:94 >> + compiler.setResources(ANGLEResources); > > Is getting/setting the resources expensive? If not, it might read easier if this function just does something like: > > ANGLEWebKitBridge& compiler = m_context->m_compiler; > ShBuiltInResources ANGLEResources = compiler.getResources(); > > if (name == "GL_OES_standard_derivatives") > ANGLEResources.OES_standard_derivatives = 1; > else if .... > > compiler.setResources(ANGLEResources); See above comment; this is essentially a copy of existing (known working) sources so I'd rather not change them too much during the initial commit. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:128 >> + return compiler.getResources().OES_standard_derivatives; > > I wouldn't bother with the local variable here. See above comment about copy of sources. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:154 >> + return ""; // Invalid shader type. > > Use emptyString(); Seems simple enough change. Done. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:159 >> + return ""; > > Use emptyString(); Done. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:182 >> + return ""; > > emptyString(); Done. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:69 >> + void getnUniformivEXT(GC3Duint program, int location, GC3Dsizei bufSize, int *params) override; > > Should we just leave these out for now since they aren't implemented? I don't think that's an option; they're pure virtual in the base class. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:88 >> + // Weak pointer back to GraphicsContext3D > > Nit: End with . Done. >> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:33 >> +#include "GraphicsContext3DIOS.h" > > I really need to get rid of this horrible hack. Adopting ANGLE will definitely help! Yes, we can eliminate this easily in forthcoming patches. This is essentially a copy of GraphicsContext3DOpenGLCommon combined with GraphicsContext3DOpenGL. >> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:277 >> + // clamp the maximum to 8192, which is big enough for a 5K display. > > Oops. We just announced a 6K display :( This code should handle that too, but we can fix it in the future if not.

Created attachment 372093 [details] Patch

(In reply to Dean Jackson from comment #29) > The test failure is unrelated. Ken, if you upload a new patch I'll land it > for you. Thanks again! New patch has been uploaded. (Still rebuilding locally, so apologies if it fails the EWS. Will fix if so.) Thank you for reviewing this and accepting these changes!

Is there any way to re-trigger an EWS bot ("win", in this case)? It looks to me like the build failure is unrelated to this patch, and probably already resolved on the bot.

(In reply to Kenneth Russell from comment #33) > Is there any way to re-trigger an EWS bot ("win", in this case)? Reupload the same patch :(

Comment on attachment 372093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372093&action=review > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) Dean can we find a better guard for these methods? I know this is just copy paste on Ken's part but these guards are fundamentally wrong. If we fix it I can just enable WebGL 2 on WinCairo. Its been low priority so I've never gotten around to it.

(In reply to Don Olmstead from comment #35) > Comment on attachment 372093 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372093&action=review > > > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) > > Dean can we find a better guard for these methods? I know this is just copy > paste on Ken's part but these guards are fundamentally wrong. If we fix it I > can just enable WebGL 2 on WinCairo. Its been low priority so I've never > gotten around to it. I can remove those guards in this file. Please note that WebKit's current WebGL 2 implementation is far from conformant and I would strongly recommend not enabling it on further platforms as doing so will only increase developer confusion. This ANGLE backend will be conformant, and should work for all WebKit ports.

(In reply to Michael Catanzaro from comment #34) > (In reply to Kenneth Russell from comment #33) > > Is there any way to re-trigger an EWS bot ("win", in this case)? > > Reupload the same patch :( Thanks for the tip. :(

Created attachment 372129 [details] 1. Add preliminary ANGLE backend to WebCore

(In reply to Kenneth Russell from comment #36) > (In reply to Don Olmstead from comment #35) > > Comment on attachment 372093 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=372093&action=review > > > > > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) > > > > Dean can we find a better guard for these methods? I know this is just copy > > paste on Ken's part but these guards are fundamentally wrong. If we fix it I > > can just enable WebGL 2 on WinCairo. Its been low priority so I've never > > gotten around to it. > > I can remove those guards in this file. Please note that WebKit's current > WebGL 2 implementation is far from conformant and I would strongly recommend > not enabling it on further platforms as doing so will only increase > developer confusion. This ANGLE backend will be conformant, and should work > for all WebKit ports. Removed in the current patch. Taking the opportunity to do another EWS run which will hopefully be green.

(In reply to Kenneth Russell from comment #39) > (In reply to Kenneth Russell from comment #36) > > (In reply to Don Olmstead from comment #35) > > > Comment on attachment 372093 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=372093&action=review > > > > > > > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > > > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) > > > > > > Dean can we find a better guard for these methods? I know this is just copy > > > paste on Ken's part but these guards are fundamentally wrong. If we fix it I > > > can just enable WebGL 2 on WinCairo. Its been low priority so I've never > > > gotten around to it. > > > > I can remove those guards in this file. Please note that WebKit's current > > WebGL 2 implementation is far from conformant and I would strongly recommend > > not enabling it on further platforms as doing so will only increase > > developer confusion. This ANGLE backend will be conformant, and should work > > for all WebKit ports. > > Removed in the current patch. Taking the opportunity to do another EWS run > which will hopefully be green. It was more of a comment for Dean since those are actually in the header. I would think it should take into account whether the OpenGL, or OpenGL ES, has those functions, or whether its the ANGLE backend. The WebGL2 support would just be enabled as experimental so if there are bugs thats fine.

(In reply to Don Olmstead from comment #40) > (In reply to Kenneth Russell from comment #39) > > (In reply to Kenneth Russell from comment #36) > > > (In reply to Don Olmstead from comment #35) > > > > Comment on attachment 372093 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=372093&action=review > > > > > > > > > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > > > > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) > > > > > > > > Dean can we find a better guard for these methods? I know this is just copy > > > > paste on Ken's part but these guards are fundamentally wrong. If we fix it I > > > > can just enable WebGL 2 on WinCairo. Its been low priority so I've never > > > > gotten around to it. > > > > > > I can remove those guards in this file. Please note that WebKit's current > > > WebGL 2 implementation is far from conformant and I would strongly recommend > > > not enabling it on further platforms as doing so will only increase > > > developer confusion. This ANGLE backend will be conformant, and should work > > > for all WebKit ports. > > > > Removed in the current patch. Taking the opportunity to do another EWS run > > which will hopefully be green. > > It was more of a comment for Dean since those are actually in the header. I > would think it should take into account whether the OpenGL, or OpenGL ES, > has those functions, or whether its the ANGLE backend. > > The WebGL2 support would just be enabled as experimental so if there are > bugs thats fine. As you pointed out, I copied that #if from GraphicsContext3DOpenGLCommon.cpp and it isn't necessary in the ANGLE backend. GraphicsContext3D.h doesn't seem to conditionalize the declaration of mapBufferRange, unmapBuffer and these other entry points, so with USE_ANGLE=1 the WinCairo port will get these definitions. Let's work together to make the ANGLE backend happen for WinCairo; I'll have to focus exclusively on the desktop macOS port.

Comment on attachment 372093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372093&action=review >>>>>> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 >>>>>> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) >>>>> >>>>> Dean can we find a better guard for these methods? I know this is just copy paste on Ken's part but these guards are fundamentally wrong. If we fix it I can just enable WebGL 2 on WinCairo. Its been low priority so I've never gotten around to it. >>>> >>>> I can remove those guards in this file. Please note that WebKit's current WebGL 2 implementation is far from conformant and I would strongly recommend not enabling it on further platforms as doing so will only increase developer confusion. This ANGLE backend will be conformant, and should work for all WebKit ports. >>> >>> Removed in the current patch. Taking the opportunity to do another EWS run which will hopefully be green. >> >> It was more of a comment for Dean since those are actually in the header. I would think it should take into account whether the OpenGL, or OpenGL ES, has those functions, or whether its the ANGLE backend. >> >> The WebGL2 support would just be enabled as experimental so if there are bugs thats fine. > > As you pointed out, I copied that #if from GraphicsContext3DOpenGLCommon.cpp and it isn't necessary in the ANGLE backend. GraphicsContext3D.h doesn't seem to conditionalize the declaration of mapBufferRange, unmapBuffer and these other entry points, so with USE_ANGLE=1 the WinCairo port will get these definitions. Let's work together to make the ANGLE backend happen for WinCairo; I'll have to focus exclusively on the desktop macOS port. Yeah, I can't remember why they are here. It should be a USE definition in Platform.h. Let's fix this after the patch lands (both here and GC3DOpenGLCommon)

Committed r246501: <https://trac.webkit.org/changeset/246501>

May I reopen this? I'd been hoping to land a couple of patches under this bug.

(In reply to Kenneth Russell from comment #44) > May I reopen this? I'd been hoping to land a couple of patches under this > bug. This probably should've been a meta bug then. Think its preferable to open separate bugs for each change.

Reverted r246501 for reason: Breaks Apple internal builds. Committed r246512: <https://trac.webkit.org/changeset/246512>

(In reply to Ryan Haddad from comment #46) > Reverted r246501 for reason: > > Breaks Apple internal builds. > > Committed r246512: <https://trac.webkit.org/changeset/246512> I shared the failed build log with Dean, hoping that he will take a look soon.

Instead of using: output_dir=$BUILT_PRODUCTS_DIR/usr/local/include/ANGLE Maybe make use of the following variable: PUBLIC_HEADERS_FOLDER_PATH = $(ANGLE_INSTALL_PATH_PREFIX)/usr/local/include/ANGLE; I haven't tried that change, but since that's where the headers are actually copied to, it seems to make sense.

(In reply to Don Olmstead from comment #45) > (In reply to Kenneth Russell from comment #44) > > May I reopen this? I'd been hoping to land a couple of patches under this > > bug. > > This probably should've been a meta bug then. Think its preferable to open > separate bugs for each change. Thanks for the suggestion. Filed a new meta-bug, made it depend on this one, and changed the summary.

(In reply to Keith Rollin from comment #48) > Instead of using: > > output_dir=$BUILT_PRODUCTS_DIR/usr/local/include/ANGLE > > Maybe make use of the following variable: > > PUBLIC_HEADERS_FOLDER_PATH = > $(ANGLE_INSTALL_PATH_PREFIX)/usr/local/include/ANGLE; > > I haven't tried that change, but since that's where the headers are actually > copied to, it seems to make sense. Thanks for the suggestion. It looks like ANGLE_INSTALL_PATH_PREFIX isn't set for external WebKit builds, so PUBLIC_HEADERS_FOLDER_PATH isn't set up in that way - is it for Apple-internal builds? Uploading a new patch now which attempts to make both cases work. Appreciate your or anyone else's help testing and/or modifying it.

Created attachment 372309 [details] Patch

Current patch changes how Source/ThirdParty/ANGLE/adjust-angle-include-paths.sh attempts to find the output directory. Also adjusted the ChangeLog messages to reflect the new bug description. Those are the only changes compared to the earlier version which was rolled out. Could someone from Apple please try this in an Apple-internal build setting and see whether it works? I'd greatly appreciate any help. Thanks.

(In reply to Kenneth Russell from comment #50) > (In reply to Keith Rollin from comment #48) > > Instead of using: > > > > output_dir=$BUILT_PRODUCTS_DIR/usr/local/include/ANGLE > > > > Maybe make use of the following variable: > > > > PUBLIC_HEADERS_FOLDER_PATH = > > $(ANGLE_INSTALL_PATH_PREFIX)/usr/local/include/ANGLE; > > > > I haven't tried that change, but since that's where the headers are actually > > copied to, it seems to make sense. > > Thanks for the suggestion. It looks like ANGLE_INSTALL_PATH_PREFIX isn't set > for external WebKit builds, so PUBLIC_HEADERS_FOLDER_PATH isn't set up in > that way - is it for Apple-internal builds? > > Uploading a new patch now which attempts to make both cases work. Appreciate > your or anyone else's help testing and/or modifying it. I should have thought this through a little bit more in my first posting. I think that what we want here is: output_dir=$BUILT_PRODUCTS_DIR/$PUBLIC_HEADERS_FOLDER_PATH I'll try this out and post back with the results. Go ahead and test for both of those before using them if you like, but otherwise I don't think there's any need to differentiate between "internal" and "external" builds. ANGLE_INSTALL_PATH_PREFIX may be undefined or set to an empty string in some configurations, but that's expected. In those cases, output_dir degenerates to what you originally had.

I ended up with the following. I didn't see a build variable that held the desired path in both contexts. if [ "$DEPLOYMENT_LOCATION" == "YES" ] ; then output_dir=$DSTROOT/$PUBLIC_HEADERS_FOLDER_PATH else output_dir=$BUILT_PRODUCTS_DIR/$PUBLIC_HEADERS_FOLDER_PATH fi

Created attachment 372349 [details] Patch

Thanks Keith, and Dean also for figuring out a similar solution he sent via email. Have incorporated that fix. I also accidentally introduced a bug with my earlier changes to that script, now fixed in the current reland patch.

I'm testing on normal and production/deployment builds, and then I'll land.

It built fine!

Committed r246554: <https://trac.webkit.org/changeset/246554>

(In reply to Dean Jackson from comment #58) > It built fine! Thank you Dean for your help shepherding this fix through!

USE_ANGLE can be left undefined on some configurations, leading to a bunch of compiler warnings when the incompatibility check in Platform.h is done. Uploaded a patch that defines the macro value to 0 in case it's not been previously defined in bug #198991, please take a look.