[webkit-reviews] review denied: [Bug 49460] Refactor GL backend flags : [Attachment 73765] patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 49460: Refactor GL backend flags
https://bugs.webkit.org/show_bug.cgi?id=49460

Attachment 73765: patch
https://bugs.webkit.org/attachment.cgi?id=73765&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73765&action=review

r- because of the need for more documentation of the Chromium-specific
extensions.

> WebCore/html/canvas/WebGLRenderingContext.cpp:166
> +    m_isErrorGeneratedOnOutOfBoundsAccesses =
m_context->getExtensions()->supports("GL_CHROMIUM_strict_attribs");
> +    m_isResourceSafe =
m_context->getExtensions()->supports("GL_CHROMIUM_resource_safe");

These extension names minimally need to be added to
WebCore/platform/graphics/chromium/Extensions3DChromium.h, ideally with at
least a little documentation of their semantics since they aren't standard,
registered extensions.

> WebCore/html/canvas/WebGLRenderingContext.cpp:172
> +	   m_isGLES2NPOTStrict =
!m_context->getExtensions()->supports("GL_ARB_texture_non_power_of_two");
> +	   m_isDepthStencilSupported =
m_context->getExtensions()->supports("GL_EXT_packed_depth_stencil");

Are these checks sufficient? These extensions were folded into core OpenGL at
some point. I'm concerned that if the GraphicsContext3D implementation actually
uses say OpenGL 4.0 that these extensions might not be advertised, so you
should probably have a check of the GL version too. You could consider having
the Extensions3D interface advertise extensions like GL_VERSION_2_1 if the
OpenGL version is 2.1 or greater.