Bug 197755

Summary: Add preliminary ANGLE backend to WebCore
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Kenneth Russell <kbr>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, cgarcia, changseok, dino, don.olmstead, ews-watchlist, Hironori.Fujii, krollin, mcatanzaro, rniwa, ryanhaddad, webkit-bug-importer, yang.gu, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 197676, 197787    
Bug Blocks: 198948, 198982, 199212    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Patch
none
1. Add preliminary ANGLE backend to WebCore
none
1. Add preliminary ANGLE backend to WebCore
none
1. Add preliminary ANGLE backend to WebCore
none
1. Add preliminary ANGLE backend to WebCore
none
Patch
none
1. Add preliminary ANGLE backend to WebCore
none
Patch
none
Patch dino: review+

Description Kenneth Russell 2019-05-09 15:13:25 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-05-09 15:57:37 PDT
<rdar://problem/50642205>
Comment 2 Alex Christensen 2019-05-09 17:45:15 PDT
Created attachment 369534 [details]
Patch
Comment 3 Alex Christensen 2019-05-09 17:47:29 PDT
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.
Comment 4 Alex Christensen 2019-05-10 10:49:34 PDT
I'm upstreaming the build system part of this in https://bugs.webkit.org/show_bug.cgi?id=197787
Comment 5 Alex Christensen 2019-05-13 14:10:44 PDT
Created attachment 369780 [details]
Patch
Comment 6 Alex Christensen 2019-05-13 14:25:40 PDT
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 7 EWS Watchlist 2019-05-13 15:15:07 PDT
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.
Comment 8 EWS Watchlist 2019-05-13 15:15:09 PDT
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 9 EWS Watchlist 2019-05-13 15:16:18 PDT
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.
Comment 10 EWS Watchlist 2019-05-13 15:16:20 PDT
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 11 EWS Watchlist 2019-05-13 15:42:12 PDT
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.
Comment 12 EWS Watchlist 2019-05-13 15:42:14 PDT
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
Comment 13 Kenneth Russell 2019-05-14 10:05:45 PDT
Linking to most recent ANGLE roll, which was needed for this work.
Comment 14 Konstantin Tokarev 2019-05-14 12:50:03 PDT
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?
Comment 15 Dean Jackson 2019-05-14 17:45:18 PDT
(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.
Comment 16 Don Olmstead 2019-05-16 09:39:10 PDT
(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.
Comment 17 Dean Jackson 2019-05-16 10:00:53 PDT
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.
Comment 18 Alex Christensen 2019-05-16 10:39:49 PDT
(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.
Comment 19 Kenneth Russell 2019-06-03 14:28:07 PDT
Created attachment 371212 [details]
Patch
Comment 20 Kenneth Russell 2019-06-07 02:43:40 PDT
Created attachment 371576 [details]
1. Add preliminary ANGLE backend to WebCore
Comment 21 Don Olmstead 2019-06-07 11:16:41 PDT
(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.
Comment 22 Kenneth Russell 2019-06-10 10:30:59 PDT
(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.
Comment 23 Kenneth Russell 2019-06-10 11:29:16 PDT
Created attachment 371764 [details]
1. Add preliminary ANGLE backend to WebCore
Comment 24 Kenneth Russell 2019-06-10 15:34:39 PDT
Created attachment 371784 [details]
1. Add preliminary ANGLE backend to WebCore
Comment 25 Kenneth Russell 2019-06-10 17:21:48 PDT
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.
Comment 26 Kenneth Russell 2019-06-10 17:22:41 PDT
Created attachment 371798 [details]
1. Add preliminary ANGLE backend to WebCore
Comment 27 EWS Watchlist 2019-06-10 20:50:32 PDT
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 28 Dean Jackson 2019-06-12 03:10:50 PDT
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 :(
Comment 29 Dean Jackson 2019-06-12 03:11:54 PDT
The test failure is unrelated. Ken, if you upload a new patch I'll land it for you. Thanks again!
Comment 30 Kenneth Russell 2019-06-13 18:42:38 PDT
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.
Comment 31 Kenneth Russell 2019-06-13 18:43:36 PDT
Created attachment 372093 [details]
Patch
Comment 32 Kenneth Russell 2019-06-13 18:45:00 PDT
(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!
Comment 33 Kenneth Russell 2019-06-13 20:35:11 PDT
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.
Comment 34 Michael Catanzaro 2019-06-14 06:43:59 PDT
(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 35 Don Olmstead 2019-06-14 10:28:20 PDT
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.
Comment 36 Kenneth Russell 2019-06-14 11:16:22 PDT
(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.
Comment 37 Kenneth Russell 2019-06-14 11:18:03 PDT
(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. :(
Comment 38 Kenneth Russell 2019-06-14 11:21:25 PDT
Created attachment 372129 [details]
1. Add preliminary ANGLE backend to WebCore
Comment 39 Kenneth Russell 2019-06-14 11:23:10 PDT
(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.
Comment 40 Don Olmstead 2019-06-14 11:30:01 PDT
(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.
Comment 41 Kenneth Russell 2019-06-14 14:55:55 PDT
(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 42 Dean Jackson 2019-06-17 11:21:51 PDT
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)
Comment 43 Dean Jackson 2019-06-17 11:25:34 PDT
Committed r246501: <https://trac.webkit.org/changeset/246501>
Comment 44 Kenneth Russell 2019-06-17 11:27:57 PDT
May I reopen this? I'd been hoping to land a couple of patches under this bug.
Comment 45 Don Olmstead 2019-06-17 12:16:12 PDT
(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.
Comment 46 Ryan Haddad 2019-06-17 13:23:52 PDT
Reverted r246501 for reason:

Breaks Apple internal builds.

Committed r246512: <https://trac.webkit.org/changeset/246512>
Comment 47 Ryan Haddad 2019-06-17 13:24:38 PDT
(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.
Comment 48 Keith Rollin 2019-06-17 15:29:42 PDT
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.
Comment 49 Kenneth Russell 2019-06-17 18:03:00 PDT
(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.
Comment 50 Kenneth Russell 2019-06-17 18:06:16 PDT
(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.
Comment 51 Kenneth Russell 2019-06-17 18:10:29 PDT
Created attachment 372309 [details]
Patch
Comment 52 Kenneth Russell 2019-06-17 18:12:53 PDT
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.
Comment 53 Keith Rollin 2019-06-17 20:47:59 PDT
(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.
Comment 54 Keith Rollin 2019-06-17 23:37:47 PDT
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
Comment 55 Kenneth Russell 2019-06-18 10:43:23 PDT
Created attachment 372349 [details]
Patch
Comment 56 Kenneth Russell 2019-06-18 10:46:21 PDT
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.
Comment 57 Dean Jackson 2019-06-18 11:53:36 PDT
I'm testing on normal and production/deployment builds, and then I'll land.
Comment 58 Dean Jackson 2019-06-18 12:00:19 PDT
It built fine!
Comment 59 Dean Jackson 2019-06-18 12:02:06 PDT
Committed r246554: <https://trac.webkit.org/changeset/246554>
Comment 60 Kenneth Russell 2019-06-18 13:01:54 PDT
(In reply to Dean Jackson from comment #58)
> It built fine!

Thank you Dean for your help shepherding this fix through!
Comment 61 Zan Dobersek 2019-06-18 23:35:07 PDT
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.