[webkit-reviews] review granted: [Bug 231011] ScopedEGLDefaultDisplay should be removed : [Attachment 439755] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 231011: ScopedEGLDefaultDisplay should be removed
https://bugs.webkit.org/show_bug.cgi?id=231011

Attachment 439755: Patch

https://bugs.webkit.org/attachment.cgi?id=439755&action=review




--- Comment #4 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 439755
  --> https://bugs.webkit.org/attachment.cgi?id=439755
Patch

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

Looks good overall, and a good step toward better supporting multiple
EGLDisplays. A couple of small naming suggestions, but feel free to take/ignore
as you see fit. r+

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:107
> +	       EGL_Terminate(display);

If TerminateAndReleaseThreadResources is passed, this code calls EGL_Terminate
before falling through to the EGL_ReleaseThread call below. Is that the correct
order in which to call these fairly low-level APIs? It looks like it probably
is - just asking for a double-check. I hope this is tested in UIWebView (?)
tests on the ios-wk2 bot and not just in the wild.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:64
>      return !!EGL_Initialize;

How does this turn into a function pointer that can be tested in this way? Via
some of the weak-linked dylib magic that was added recently?

Worth adding a comment?

> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:121
> +    enum class ReleaseResourcesBehavior {

Reading this code afresh, the term "ReleaseResources" in this enum and in the
method call is a little confusing - it sounds like OpenGL resources like
textures or buffers are being released, where in fact thread-related resources
like the current context or access to the EGL API are being released.

I wonder whether a different name could be chosen like:
  ReleaseThreadResourcesBehavior
  ReleasePerThreadResourcesBehavior
  ReleaseEGLResourcesBehavior
(and releasePerThreadResources(), releaseEGLResources(), or similar.)

> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:584
> +    static void platformReleaseResources();

If releaseResources is renamed, let's rename this to match.