Bug 224555 - ANGLE is only being built when WebGL is enabled
Summary: ANGLE is only being built when WebGL is enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-14 09:59 PDT by Don Olmstead
Modified: 2021-04-25 14:35 PDT (History)
13 users (show)

See Also:


Attachments
Patch (7.55 KB, patch)
2021-04-14 10:22 PDT, Don Olmstead
achristensen: review-
Details | Formatted Diff | Diff
Patch (7.24 KB, patch)
2021-04-14 13:21 PDT, Don Olmstead
Hironori.Fujii: review-
Details | Formatted Diff | Diff
Patch (7.25 KB, patch)
2021-04-15 12:50 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2021-04-14 09:59:15 PDT
If someone disables WebGL Windows still needs ANGLE support.
Comment 1 Don Olmstead 2021-04-14 10:22:49 PDT
Created attachment 425999 [details]
Patch
Comment 2 Darin Adler 2021-04-14 10:51:49 PDT
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 3 Alex Christensen 2021-04-14 10:52:58 PDT
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 4 Kenneth Russell 2021-04-14 10:53:27 PDT
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?
Comment 5 Alex Christensen 2021-04-14 10:57:58 PDT
Kenneth is also right.  That TemporaryOpenGLSetting is bad for several reasons and I really shouldn't have r+'ed it.
Comment 6 Fujii Hironori 2021-04-14 13:14:00 PDT
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)".
Comment 7 Don Olmstead 2021-04-14 13:21:07 PDT
Created attachment 426040 [details]
Patch

Address review feedback
Comment 8 Fujii Hironori 2021-04-14 13:23:05 PDT
Comment on attachment 426040 [details]
Patch

r- for comment#6.
Comment 9 Don Olmstead 2021-04-15 12:50:38 PDT
Created attachment 426127 [details]
Patch
Comment 10 Don Olmstead 2021-04-15 12:51:19 PDT
(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.
Comment 11 Fujii Hironori 2021-04-15 12:58:53 PDT
Looks nice. If you finish the USE_ANGLE migration for GTK and WPE ports, WebKit will remove the USE_ANGLE macro 😁
Comment 12 EWS 2021-04-15 13:31:42 PDT
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].
Comment 13 Ryan Haddad 2021-04-22 17:33:16 PDT
rdar://76720641
Comment 14 Fujii Hironori 2021-04-25 14:35:37 PDT
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.