If someone disables WebGL Windows still needs ANGLE support.
Created attachment 425999 [details] Patch
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).
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.
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?
Kenneth is also right. That TemporaryOpenGLSetting is bad for several reasons and I really shouldn't have r+'ed it.
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)".
Created attachment 426040 [details] Patch Address review feedback
Comment on attachment 426040 [details] Patch r- for comment#6.
Created attachment 426127 [details] Patch
(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.
Looks nice. If you finish the USE_ANGLE migration for GTK and WPE ports, WebKit will remove the USE_ANGLE macro 😁
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].
rdar://76720641
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.