Bug 216722 – WebGL should use GLES in iOS apps running on Apple Silicon

• Component: WebGL
• Status: RESOLVED FIXED

[Dean Jackson]

A Catalyst app that was built for iOS may have linked against OpenGLES.framework. Meanwhile, any WebGL content running in there will use whatever WebKit links against, which currently is OpenGL.framework. This confuses the system and crashes.

The solution is to have iOS apps running on Apple Silicon use OpenGLES, rather than OpenGL. But we need more than that - we also need to not statically link either framework.

[Dean Jackson]

Created attachment 409187 [details]
WIP

Work in progress patch:

What has been done:
- all the #ifs to have the correct bits compiled on each configuration (the tricky one is Apple Silicon target with the Catalyst variant, which needs both OpenGL and OpenGLES)
- added soft linking of all CGL methods, and removed "-l OpenGL" from the build

What needs to be done (and I'll do later today):
- add soft linking for EAGL methods (will need more of the Soft Linking macros)
- dynamic selection of DisplayCGL v DisplayEAGL at runtime

What I don't yet know how to do:
- how to expose GL_BGRA_EXT in formatutils, which is different on GL and GLES. I might have to do a runtime check there as well.

[Dean Jackson]

Created attachment 409199 [details]
WIP2

Not quite. Obviously WebCore needs to make sure it doesn't link OpenGL. And WebKit does as well.

Uploading a new patch that removes the OpenGL linking up to WebCore.

[Dean Jackson]

<rdar://problem/68976337>

[Dean Jackson]

Created attachment 409231 [details]
WIP3

This latest patch (WIP3) compiles and runs fine on macOS and Intel Catalyst.

All hard-linking to GL has been removed. It won't link on iOS yes because I haven't added the soft-linking glue for GLES.

[Dean Jackson]

I just realised this patch is missing a couple of new files in ANGLE. Here is WIP4, which has a start at the iOS/EAGL linking in ANGLE.

[Dean Jackson]

Created attachment 409232 [details]
WIP4

[Dean Jackson]

Created attachment 409268 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Dean Jackson]

I am amazed that EWS is all green on this patch. I've had one-line patches that I'm sure would work that broke everything, yet this one somehow managed to pass.

[Dean Jackson]

I think it must be a mistake!

[Kenneth Russell]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

Looks fantastic overall - a few questions before approving.

> Source/ThirdParty/ANGLE/src/common/platform.h:120
> +#    define ANGLE_DYLD_PLATFORM_IOS 2

Please add a comment that these are well-known constants which come back from the Apple-specific dyld_get_active_platform(), and must not be changed.

> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
> +    bool isIOSOnMac      = false;

Could you rename this to "isIOSOnAppleSilicon" for clarity?

> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo_apple.cpp:17
> +   extern uint32_t dyld_get_active_platform(void);

Could you add a comment about where this extern is defined? Perhaps a documentation URL?

> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo_apple.cpp:30
> +#endif

Can these prototypes be moved to headers with appropriate #include guards?

> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo_ios.cpp:19
> +#endif

Can this declaration be moved to some header which is common to SystemInfo_apple.cpp and SystemInfo_ios.cpp?

> Source/ThirdParty/ANGLE/src/libANGLE/Caps.cpp:631
> +        // FIXME: I think this needs to be a runtime check when running an iOS app on Mac.

I'm pretty sure - but not 100% sure - that later patches made the removal of this requirement unnecessary, because lower levels emulate the support for this format on top of either 16- or 24-bit formats. See https://bugs.chromium.org/p/angleproject/issues/detail?id=4591#c13 . Let's please plan on revisiting this. I'm not 100% sure how to test it - with Unity content and the conformance suite on iOS hardware?

> Source/ThirdParty/ANGLE/src/libANGLE/Caps.cpp:648
> +        // FIXME: I think this needs to be a runtime check when running an iOS app on Mac.

Same comment here.

> Source/ThirdParty/ANGLE/src/libANGLE/Display.cpp:282
> +                if (info.isIOSOnMac)

This is confusing - why do iOS binaries on Apple Silicon use CGL? Per comments above I thought they were supposed to use EAGL. Is this a bug?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/EAGLFunctions.h:26
> +//SOFT_LINK_FUNCTION_HEADER(OpenGL, CGLChoosePixelFormat, CGLError, (const CGLPixelFormatAttribute *attribs, CGLPixelFormatObj *pix, GLint *npix), (attribs, pix, npix))

Please remove commented-out code.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/EAGLFunctions.mm:23
> +//SOFT_LINK_FUNCTION_SOURCE(OpenGL, CGLChoosePixelFormat, CGLError, (const CGLPixelFormatAttribute *attribs, CGLPixelFormatObj *pix, GLint *npix), (attribs, pix, npix))

Same here.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/WindowSurfaceEAGL.mm:-25
> -// TODO(anglebug.com/4275): It's not clear why this needs to be an EAGLLayer.

Is this clear now? Could you drop a comment on this bug, or reference it in your upstreaming CL to the ANGLE project?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/WindowSurfaceEAGL.mm:32
> +- (BOOL)presentRenderbuffer:(NSUInteger)target;

Is this like a forward declaration of something in a system header? Can it be pulled in via some other mechanism rather than declared inline here?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/renderergl_utils.cpp:1348
> +    if (angle::GetSystemInfo(&info))

Will querying the system info at this point have any side-effects, startup time or otherwise? If it's been queried before, can it be passed in to this point?

[Darin Adler]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>> +    bool isIOSOnMac      = false;
> 
> Could you rename this to "isIOSOnAppleSilicon" for clarity?

Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.

[Kenneth Russell]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>>> +    bool isIOSOnMac      = false;
>> 
>> Could you rename this to "isIOSOnAppleSilicon" for clarity?
> 
> Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.

Fair enough - would "isIOSonAppleSiliconMac" be too wordy?

[Darin Adler]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>>>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>>>> +    bool isIOSOnMac      = false;
>>> 
>>> Could you rename this to "isIOSOnAppleSilicon" for clarity?
>> 
>> Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.
> 
> Fair enough - would "isIOSonAppleSiliconMac" be too wordy?

No, "isIOSOnAppleSiliconMac" seems fine. (Note that the terminology is wrong, because Apple doesn’t call these frameworks running on a Mac "iOS".)

[Darin Adler]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>>>>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>>>>> +    bool isIOSOnMac      = false;
>>>> 
>>>> Could you rename this to "isIOSOnAppleSilicon" for clarity?
>>> 
>>> Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.
>> 
>> Fair enough - would "isIOSonAppleSiliconMac" be too wordy?
> 
> No, "isIOSOnAppleSiliconMac" seems fine. (Note that the terminology is wrong, because Apple doesn’t call these frameworks running on a Mac "iOS".)

Actually "isiOSAppOnMac" would be better.

[Dean Jackson]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>>>>>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>>>>>> +    bool isIOSOnMac      = false;
>>>>> 
>>>>> Could you rename this to "isIOSOnAppleSilicon" for clarity?
>>>> 
>>>> Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.
>>> 
>>> Fair enough - would "isIOSonAppleSiliconMac" be too wordy?
>> 
>> No, "isIOSOnAppleSiliconMac" seems fine. (Note that the terminology is wrong, because Apple doesn’t call these frameworks running on a Mac "iOS".)
> 
> Actually "isiOSAppOnMac" would be better.

Our public API uses a flag called "iOSAppOnMac" - https://developer.apple.com/documentation/foundation/nsprocessinfo/3608556-iosapponmac

I wonder if I should use that instead. It would avoid the dyld_get_active call (which is private as far as I can tell) as well as the return values (which are private but use #defines so probably can never change). It would mean I'd have to change some files to .mm though.

[Kenneth Russell]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>>>>>>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>>>>>>> +    bool isIOSOnMac      = false;
>>>>>> 
>>>>>> Could you rename this to "isIOSOnAppleSilicon" for clarity?
>>>>> 
>>>>> Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.
>>>> 
>>>> Fair enough - would "isIOSonAppleSiliconMac" be too wordy?
>>> 
>>> No, "isIOSOnAppleSiliconMac" seems fine. (Note that the terminology is wrong, because Apple doesn’t call these frameworks running on a Mac "iOS".)
>> 
>> Actually "isiOSAppOnMac" would be better.
> 
> Our public API uses a flag called "iOSAppOnMac" - https://developer.apple.com/documentation/foundation/nsprocessinfo/3608556-iosapponmac
> 
> I wonder if I should use that instead. It would avoid the dyld_get_active call (which is private as far as I can tell) as well as the return values (which are private but use #defines so probably can never change). It would mean I'd have to change some files to .mm though.

If it's possible to use the public API - and still have the code work when built against earlier SDK versions, so some sort of weak linkage for that Objective-C property - that would be preferable in ANGLE.

[Darin Adler]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>>>>>>>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>>>>>>>> +    bool isIOSOnMac      = false;
>>>>>>> 
>>>>>>> Could you rename this to "isIOSOnAppleSilicon" for clarity?
>>>>>> 
>>>>>> Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.
>>>>> 
>>>>> Fair enough - would "isIOSonAppleSiliconMac" be too wordy?
>>>> 
>>>> No, "isIOSOnAppleSiliconMac" seems fine. (Note that the terminology is wrong, because Apple doesn’t call these frameworks running on a Mac "iOS".)
>>> 
>>> Actually "isiOSAppOnMac" would be better.
>> 
>> Our public API uses a flag called "iOSAppOnMac" - https://developer.apple.com/documentation/foundation/nsprocessinfo/3608556-iosapponmac
>> 
>> I wonder if I should use that instead. It would avoid the dyld_get_active call (which is private as far as I can tell) as well as the return values (which are private but use #defines so probably can never change). It would mean I'd have to change some files to .mm though.
> 
> If it's possible to use the public API - and still have the code work when built against earlier SDK versions, so some sort of weak linkage for that Objective-C property - that would be preferable in ANGLE.

I assume only one file would have to be .mm

[Dean Jackson]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>>>>>>>>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>>>>>>>>> +    bool isIOSOnMac      = false;
>>>>>>>> 
>>>>>>>> Could you rename this to "isIOSOnAppleSilicon" for clarity?
>>>>>>> 
>>>>>>> Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.
>>>>>> 
>>>>>> Fair enough - would "isIOSonAppleSiliconMac" be too wordy?
>>>>> 
>>>>> No, "isIOSOnAppleSiliconMac" seems fine. (Note that the terminology is wrong, because Apple doesn’t call these frameworks running on a Mac "iOS".)
>>>> 
>>>> Actually "isiOSAppOnMac" would be better.
>>> 
>>> Our public API uses a flag called "iOSAppOnMac" - https://developer.apple.com/documentation/foundation/nsprocessinfo/3608556-iosapponmac
>>> 
>>> I wonder if I should use that instead. It would avoid the dyld_get_active call (which is private as far as I can tell) as well as the return values (which are private but use #defines so probably can never change). It would mean I'd have to change some files to .mm though.
>> 
>> If it's possible to use the public API - and still have the code work when built against earlier SDK versions, so some sort of weak linkage for that Objective-C property - that would be preferable in ANGLE.
> 
> I assume only one file would have to be .mm

The call to the API would be inside a #if block that's only true on Apple Silicon devices, so shouldn't need an SDK check or weak linking.

[Dean Jackson]

Comment on attachment 409268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409268&action=review

>> Source/ThirdParty/ANGLE/src/common/platform.h:120
>> +#    define ANGLE_DYLD_PLATFORM_IOS 2
> 
> Please add a comment that these are well-known constants which come back from the Apple-specific dyld_get_active_platform(), and must not be changed.

Removed in place of public API.

>>>>>>>>>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo.h:71
>>>>>>>>>> +    bool isIOSOnMac      = false;
>>>>>>>>> 
>>>>>>>>> Could you rename this to "isIOSOnAppleSilicon" for clarity?
>>>>>>>> 
>>>>>>>> Done exactly like this it would harm clarity, too; OK to mention Apple Silicon but still need to mention Mac. iOS is almost always running on Apple Silicon when running on iPhone and iPad.
>>>>>>> 
>>>>>>> Fair enough - would "isIOSonAppleSiliconMac" be too wordy?
>>>>>> 
>>>>>> No, "isIOSOnAppleSiliconMac" seems fine. (Note that the terminology is wrong, because Apple doesn’t call these frameworks running on a Mac "iOS".)
>>>>> 
>>>>> Actually "isiOSAppOnMac" would be better.
>>>> 
>>>> Our public API uses a flag called "iOSAppOnMac" - https://developer.apple.com/documentation/foundation/nsprocessinfo/3608556-iosapponmac
>>>> 
>>>> I wonder if I should use that instead. It would avoid the dyld_get_active call (which is private as far as I can tell) as well as the return values (which are private but use #defines so probably can never change). It would mean I'd have to change some files to .mm though.
>>> 
>>> If it's possible to use the public API - and still have the code work when built against earlier SDK versions, so some sort of weak linkage for that Objective-C property - that would be preferable in ANGLE.
>> 
>> I assume only one file would have to be .mm
> 
> The call to the API would be inside a #if block that's only true on Apple Silicon devices, so shouldn't need an SDK check or weak linking.

I settled on mirroring the public API: isiOSAppOnMac

>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo_apple.cpp:17
>> +   extern uint32_t dyld_get_active_platform(void);
> 
> Could you add a comment about where this extern is defined? Perhaps a documentation URL?

Removed.

>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo_apple.cpp:30
>> +#endif
> 
> Can these prototypes be moved to headers with appropriate #include guards?

Is this necessary? They are only called from this file - the entry point is GetSystemInfo. I even considered that they both should be inline here.

I moved them into SystemInfo_internal.h.

> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo_apple.cpp:35
> +    if (dyld_get_active_platform() == ANGLE_DYLD_PLATFORM_IOS)

This was replaced.

>> Source/ThirdParty/ANGLE/src/gpu_info_util/SystemInfo_ios.cpp:19
>> +#endif
> 
> Can this declaration be moved to some header which is common to SystemInfo_apple.cpp and SystemInfo_ios.cpp?

Removed.

>> Source/ThirdParty/ANGLE/src/libANGLE/Caps.cpp:631
>> +        // FIXME: I think this needs to be a runtime check when running an iOS app on Mac.
> 
> I'm pretty sure - but not 100% sure - that later patches made the removal of this requirement unnecessary, because lower levels emulate the support for this format on top of either 16- or 24-bit formats. See https://bugs.chromium.org/p/angleproject/issues/detail?id=4591#c13 . Let's please plan on revisiting this. I'm not 100% sure how to test it - with Unity content and the conformance suite on iOS hardware?

Great news. I'll check this separately but leave the FIXME in this patch.

I am working on getting an agreement from Unity to allow a compiled version of their Tiny3D renderer in the WebKit test suite.

As for hardware testing... that's still impossible without Apple internal bits.

>> Source/ThirdParty/ANGLE/src/libANGLE/Display.cpp:282
>> +                if (info.isIOSOnMac)
> 
> This is confusing - why do iOS binaries on Apple Silicon use CGL? Per comments above I thought they were supposed to use EAGL. Is this a bug?

It is confusing!!

There are two types of binaries on Apple Silicon inside Catalyst: ones that were compiled against the Catalyst SDK (where only OpenGL is available) and ones that were compiled against the iOS SDK (where only OpenGLES is available - those are the iOS apps on macOS). The former should use CGL and the latter should use EAGL. We have to check this at runtime because there is only a single WebKit binary for both. WebGL content worked fine using OpenGL with the iOS Apps, but there was breakage if the app itself linked to OpenGLES (we can't have both frameworks loaded at once).

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/cgl/WindowSurfaceCGL.mm:42
> +@implementation WebSwapLayerCGL

I noted in the ChangeLog that I had to rename these because there is a configuration that compiled both the WindowSurfaceCGL and WindowSurfaceEAGL files.

I'm not sure if any of this code actually works. It isn't used by WebKit.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/DisplayEAGL.mm:-30
> -#    define GLES_SILENCE_DEPRECATION

I forgot to mention in the ChangeLog that I moved this into the .xcconfig file.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/EAGLFunctions.h:26
>> +//SOFT_LINK_FUNCTION_HEADER(OpenGL, CGLChoosePixelFormat, CGLError, (const CGLPixelFormatAttribute *attribs, CGLPixelFormatObj *pix, GLint *npix), (attribs, pix, npix))
> 
> Please remove commented-out code.

Oops!

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/EAGLFunctions.mm:23
>> +//SOFT_LINK_FUNCTION_SOURCE(OpenGL, CGLChoosePixelFormat, CGLError, (const CGLPixelFormatAttribute *attribs, CGLPixelFormatObj *pix, GLint *npix), (attribs, pix, npix))
> 
> Same here.

Oops!

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/WindowSurfaceEAGL.mm:-25
>> -// TODO(anglebug.com/4275): It's not clear why this needs to be an EAGLLayer.
> 
> Is this clear now? Could you drop a comment on this bug, or reference it in your upstreaming CL to the ANGLE project?

This code isn't used by WebKit so I haven't confirmed anything. I guess it is part of the EGL interface that we don't use because we provide the framebuffers and compositing externally, whereas this is for a native window showing in an app.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/WindowSurfaceEAGL.mm:32
>> +- (BOOL)presentRenderbuffer:(NSUInteger)target;
> 
> Is this like a forward declaration of something in a system header? Can it be pulled in via some other mechanism rather than declared inline here?

Hmmm... good question. There was a reason I did this, related to me removing the OpenGL/EAGL.h include.

Ok, I checked. I can remove the EAGLContext one, but the CAEAGLLayer has to remain due to a quirk in our SDKs. The QuartzCore headers on Mac Catalyst don't define that class (OpenGL is not available in that configuration, and definitely not OpenGLES).

I have a slight fear that this could cause problems for ANGLE clients outside of WebKit. If they submit to the App Store, they might get flagged as using SPI. But this would be the case anyway for Catalyst apps. Definitely worth investigating.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/renderergl_utils.cpp:1348
>> +    if (angle::GetSystemInfo(&info))
> 
> Will querying the system info at this point have any side-effects, startup time or otherwise? If it's been queried before, can it be passed in to this point?

I wondered that but found a bunch of other places in ANGLE where it is fetching the SystemInfo on demand. I think I'll file an ANGLE bug suggesting a global SystemInfo be kept somewhere to avoid this.

[Dean Jackson]

Created attachment 409520 [details]
Patch

[Kenneth Russell]

Comment on attachment 409520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409520&action=review

One initial comment / question.

> Source/ThirdParty/ANGLE/src/libANGLE/Display.cpp:284
> +                    impl = new rx::DisplayCGL(state);

I'm still confused about the sense of the "isiOSAppOnMac" flag. The ChangeLog says:

            Apple Silicon Mac Catalyst (with Mac app) -> CGL
            Apple Silicon Mac Catalyst (with iOS app) -> EAGL

Isn't "isiOSAppOnMac" the "Apple Silicon Mac Catalyst (with iOS app)" case? And if that's so, then why is this arm using DisplayCGL rather than DisplayEAGL?

[Dean Jackson]

Comment on attachment 409520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409520&action=review

>> Source/ThirdParty/ANGLE/src/libANGLE/Display.cpp:284
>> +                    impl = new rx::DisplayCGL(state);
> 
> I'm still confused about the sense of the "isiOSAppOnMac" flag. The ChangeLog says:
> 
>             Apple Silicon Mac Catalyst (with Mac app) -> CGL
>             Apple Silicon Mac Catalyst (with iOS app) -> EAGL
> 
> Isn't "isiOSAppOnMac" the "Apple Silicon Mac Catalyst (with iOS app)" case? And if that's so, then why is this arm using DisplayCGL rather than DisplayEAGL?

You're right. This patch has them the wrong way around.

[Dean Jackson]

Created attachment 409547 [details]
Patch

[Tim Horton]

Comment on attachment 409547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409547&action=review

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/DisplayEAGL.mm:73
> +    mContext = [[getEAGLContextClass() alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];

usually want allocEAGLContextInstance() or you'll get mysterious ambiguous selector warnings years down the road

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/DisplayEAGL.mm:355
> +    context                = [[getEAGLContextClass() alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];

here too

[Dean Jackson]

Comment on attachment 409547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409547&action=review

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/DisplayEAGL.mm:73
>> +    mContext = [[getEAGLContextClass() alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
> 
> usually want allocEAGLContextInstance() or you'll get mysterious ambiguous selector warnings years down the road

Thanks!

[Kenneth Russell]

Can I please review this too before committing?

[Dean Jackson]

Created attachment 409640 [details]
EWS test

[Kenneth Russell]

Comment on attachment 409547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409547&action=review

Looks good to me too. Would like to upstream the ANGLE changes as soon as possible after this lands.

> Source/ThirdParty/ANGLE/ChangeLog:20
> +        application is it attempting to run. In that case ANGLE must compile

"on the application is it" -> "on the type of the application it is"?

> Source/ThirdParty/ANGLE/src/libANGLE/Caps.cpp:631
> +        // FIXME: I think this needs to be a runtime check when running an iOS app on Mac.

Could you perhaps make this "TODO(dino)" instead of FIXME? Here and below. TODO is Chromium's preferred style.

> Source/ThirdParty/ANGLE/src/libANGLE/formatutils.cpp:1038
> +        if (!info.isiOSAppOnMac)

Would this read more nicely if it were the positive rather than the negative? "if (info.isiOSAppOnMac) ..."

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/WindowSurfaceEAGL.mm:27
> +// FIXME: Necessary because CAEAGLLayer is not in the public QuartzCore headers in this configuration.

Could you mention any pitfalls about SPI and embedding ANGLE in iOS apps here?

Also: would you mind using TODO(dino) or TODO([e-mail])?

[Dean Jackson]

Created attachment 409643 [details]
EWS test 2

[Dean Jackson]

Comment on attachment 409547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409547&action=review

>> Source/ThirdParty/ANGLE/ChangeLog:20
>> +        application is it attempting to run. In that case ANGLE must compile
> 
> "on the application is it" -> "on the type of the application it is"?

Fixed!

>> Source/ThirdParty/ANGLE/src/libANGLE/Caps.cpp:631
>> +        // FIXME: I think this needs to be a runtime check when running an iOS app on Mac.
> 
> Could you perhaps make this "TODO(dino)" instead of FIXME? Here and below. TODO is Chromium's preferred style.

Sure thing. Fixed through the entire patch.

>> Source/ThirdParty/ANGLE/src/libANGLE/formatutils.cpp:1038
>> +        if (!info.isiOSAppOnMac)
> 
> Would this read more nicely if it were the positive rather than the negative? "if (info.isiOSAppOnMac) ..."

Agreed. Fixed.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/WindowSurfaceEAGL.mm:27
>> +// FIXME: Necessary because CAEAGLLayer is not in the public QuartzCore headers in this configuration.
> 
> Could you mention any pitfalls about SPI and embedding ANGLE in iOS apps here?
> 
> Also: would you mind using TODO(dino) or TODO([e-mail])?

Fixed.

[Dean Jackson]

Committed r267602: <https://trac.webkit.org/changeset/267602>