Bug 224555 – ANGLE is only being built when WebGL is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Don Olmstead]

If someone disables WebGL Windows still needs ANGLE support.

[Don Olmstead]

Created attachment 425999 [details]
Patch

[Darin Adler]

Comment on attachment 425999 [details]
Patch

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

> Source/WebKitLegacy/win/ChangeLog:8
> +        Remove use of TemporaryOpenGLSetting since it is guarded with ENABLE(WEBGL).

This seems unfortunate, and expedient rather than what we’d want. Wouldn’t we prefer to guard it correctly so it can be used wherever it’s useful? I would assume that it’s useful to have this class on any platform where we use OpenGL, not on any platform where we implement WebGL. For example, if WebGL is implemented on top of ANGLE, seems TemporaryOpenGLSetting is not necessarily at all useful. And if we are using OpenGL to implement other things, it would be useful. So it would be guarded with USE(OPENGL).

[Alex Christensen]

Comment on attachment 425999 [details]
Patch

Darin's right.  I thought it's not so bad to remove one use of it, but it would be even better to keep using it.

[Kenneth Russell]

Comment on attachment 425999 [details]
Patch

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

Looks OK overall with one comment.

> Source/WebKitLegacy/win/WebCoreSupport/AcceleratedCompositingContext.cpp:181
> +        glEnable(GL_SCISSOR_TEST);

This isn't the same semantic as the earlier code; it would be necessary to query the enabled state via glIsEnabled(GL_SCISSOR_TEST) and then decide whether to re-enable it at the end of the block.

Does the context used by this code interact with any other code that might be making assumptions about the scissor test? In particular, can it end up reusing a WebGL rendering context's underlying OpenGL context?

[Alex Christensen]

Kenneth is also right.  That TemporaryOpenGLSetting is bad for several reasons and I really shouldn't have r+'ed it.

[Fujii Hironori]

Comment on attachment 425999 [details]
Patch

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

> ChangeLog:3
> +        [WinCairo] USE_ANGLE should still work even without ENABLE_WEBGL

This sentence doesn't make sense.
USE_ANGLE macro is confusion. It doesn't mean what you expect.
USE_ANGLE means using ANGLE for WebGL.
In other word, USE_ANGLE is a switch to use GraphicsContextGLANGLE.cpp instead of GraphicsContextGLOpenGLCommon.cpp.

> Source/CMakeLists.txt:16
> +if (ENABLE_WEBGL OR USE_ANGLE)

In CMake scripts, there is USE_ANGLE_EGL variable.
This condition should be "if (ENABLE_WEBGL OR USE_ANGLE_EGL)".

> Source/WebCore/CMakeLists.txt:1728
> +elseif (USE_ANGLE)

This condition should be "if (USE_ANGLE_EGL)".

[Don Olmstead]

Created attachment 426040 [details]
Patch

Address review feedback

[Fujii Hironori]

Comment on attachment 426040 [details]
Patch

r- for comment#6.

[Don Olmstead]

Created attachment 426127 [details]
Patch

[Don Olmstead]

(In reply to Fujii Hironori from comment #8)
> Comment on attachment 426040 [details]
> Patch
> 
> r- for comment#6.

Sorry I had missed those. Patch is updated with your feedback addressed.

[Fujii Hironori]

Looks nice. If you finish the USE_ANGLE migration for GTK and WPE ports, WebKit will remove the USE_ANGLE macro 😁

[EWS]

Committed r276070 (236587@main): <https://commits.webkit.org/236587@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426127 [details].

[Ryan Haddad]

rdar://76720641

[Fujii Hironori]

Comment on attachment 425999 [details]
Patch

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

>> Source/CMakeLists.txt:16
>> +if (ENABLE_WEBGL OR USE_ANGLE)
> 
> In CMake scripts, there is USE_ANGLE_EGL variable.
> This condition should be "if (ENABLE_WEBGL OR USE_ANGLE_EGL)".

My bad. I was wrong. USE_ANGLE_EGL doesn't mean what I expected.
This condition should be 
if (ENABLE_WEBGL OR USE_ANGLE_FOR_TEXTURE_MAPPER)
And define USE_ANGLE_FOR_TEXTURE_MAPPER for WinCairo port and FTW port.