Bug 197755 – Add preliminary ANGLE backend to WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

Currently the WebGL implementation delegates to GraphicsContext3D and its OpenGL-based backends (GraphicsContext3DOpenGL.cpp, GraphicsContextOpenGLES.cpp, GraphicsContext3DOpenGLCommon.cpp).

ANGLE was just updated to top-of-tree here:
https://bugs.webkit.org/show_bug.cgi?id=197676

and now would be an excellent time to try running the WebGL implementation on top of it.

Some previous work has been done in these issues:

Progress towards using ANGLE to do WebGL rendering
https://bugs.webkit.org/show_bug.cgi?id=165864

[WebGL] ANGLEWebKitBridge should support ESSL platforms
https://bugs.webkit.org/show_bug.cgi?id=92295

One idea to avoid breaking anything would be to add a new build flag to govern whether ANGLE is used, and to make a new GraphicsContext3DANGLE.cpp, conditionally compiled, which delegates all of the calls to ANGLE. When using ANGLE to create the underlying context, the WebGL compatibility modes can and should be used in order to do exactly the validation which WebGL needs.

[Radar WebKit Bug Importer]

<rdar://problem/50642205>

[Alex Christensen]

Created attachment 369534 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

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

[Alex Christensen]

Created attachment 369780 [details]
Patch

[Alex Christensen]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Kenneth Russell]

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

[Konstantin Tokarev]

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?

[Dean Jackson]

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

[Don Olmstead]

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

[Dean Jackson]

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.

[Alex Christensen]

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

[Kenneth Russell]

Created attachment 371212 [details]
Patch

[Kenneth Russell]

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

[Don Olmstead]

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

[Kenneth Russell]

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

[Kenneth Russell]

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

[Kenneth Russell]

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

[Kenneth Russell]

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.

[Kenneth Russell]

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

[EWS Watchlist]

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

[Dean Jackson]

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

[Dean Jackson]

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

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 372093 [details]
Patch

[Kenneth Russell]

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

[Kenneth Russell]

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.

[Michael Catanzaro]

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

[Don Olmstead]

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.

[Kenneth Russell]

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

[Kenneth Russell]

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

[Kenneth Russell]

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

[Kenneth Russell]

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

[Don Olmstead]

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

[Kenneth Russell]

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

[Dean Jackson]

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)

[Dean Jackson]

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

[Kenneth Russell]

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

[Don Olmstead]

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

[Ryan Haddad]

Reverted r246501 for reason:

Breaks Apple internal builds.

Committed r246512: <https://trac.webkit.org/changeset/246512>

[Ryan Haddad]

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

[Keith Rollin]

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.

[Kenneth Russell]

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

[Kenneth Russell]

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

[Kenneth Russell]

Created attachment 372309 [details]
Patch

[Kenneth Russell]

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.

[Keith Rollin]

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

[Keith Rollin]

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

[Kenneth Russell]

Created attachment 372349 [details]
Patch

[Kenneth Russell]

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.

[Dean Jackson]

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

[Dean Jackson]

It built fine!

[Dean Jackson]

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

[Kenneth Russell]

(In reply to Dean Jackson from comment #58)
> It built fine!

Thank you Dean for your help shepherding this fix through!

[Zan Dobersek]

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.