Extension3D needs to provide a way to check if an extension is enabled

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

At the moment, we have supports(), which checks if an extension is enabled or request-able.

However, to avoid NPOT texture checking in both WebGLRenderingContext and chromium/command-buffer, we need to check if an extension is enabled without enabling it, something like a function named isEnabled().

Currently we use supports(), which results in NPOT checking in WebGLRenderingContext, which is unnecessary because NPOT support is not enabled, it's only request-able.

[Zhenyao Mo]

Created attachment 89420 [details]
Patch

[Kenneth Russell]

Comment on attachment 89420 [details]
Patch

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

This looks good overall but there are a couple of issues.

> Source/WebCore/platform/graphics/Extensions3D.h:63
> +    virtual bool isEnabled(const String&) = 0;

This new method needs better documentation. I suggest:

"""
Takes full name of extension: for example, "GL_EXT_texture_format_BGRA8888". Checks to see whether the given extension is actually enabled (see ensureEnabled). Has no other side-effects.
"""

Also, I would add it underneath ensureEnabled, which will be folded into supports() per bug 57393.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:113
> +#if PLATFORM(MAC)
> +    if (name == "GL_OES_standard_derivatives") {
> +        // Enable support in ANGLE (if not enabled already)
> +        ANGLEWebKitBridge& compiler = m_context->m_compiler;
> +        ShBuiltInResources ANGLEResources = compiler.getResources();
> +        return ANGLEResources.OES_standard_derivatives;
> +    }
> +#endif

This code is incorrect -- isEnabled() should not have any side-effects.

[Zhenyao Mo]

(In reply to comment #2)
> (From update of attachment 89420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89420&action=review
> 
> This looks good overall but there are a couple of issues.
> 
> > Source/WebCore/platform/graphics/Extensions3D.h:63
> > +    virtual bool isEnabled(const String&) = 0;
> 
> This new method needs better documentation. I suggest:
> 
> """
> Takes full name of extension: for example, "GL_EXT_texture_format_BGRA8888". Checks to see whether the given extension is actually enabled (see ensureEnabled). Has no other side-effects.
> """
> 
> Also, I would add it underneath ensureEnabled, which will be folded into supports() per bug 57393.
> 
> > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:113
> > +#if PLATFORM(MAC)
> > +    if (name == "GL_OES_standard_derivatives") {
> > +        // Enable support in ANGLE (if not enabled already)
> > +        ANGLEWebKitBridge& compiler = m_context->m_compiler;
> > +        ShBuiltInResources ANGLEResources = compiler.getResources();
> > +        return ANGLEResources.OES_standard_derivatives;
> > +    }
> > +#endif
> 
> This code is incorrect -- isEnabled() should not have any side-effects.

Ah I copied and modified, but forget to remove the comment.  The code itself doesn't have side-effects.  It simply queries if that extension is enabled in ANGLE or not.

[Kenneth Russell]

(In reply to comment #3)
> Ah I copied and modified, but forget to remove the comment.  The code itself doesn't have side-effects.  It simply queries if that extension is enabled in ANGLE or not.

Ah, I see. OK, let's just remove the incorrect comment then.

[Zhenyao Mo]

Created attachment 89445 [details]
Patch

[Kenneth Russell]

Comment on attachment 89445 [details]
Patch

Thanks. Looks good.

[Zhenyao Mo]

Committed r83762: <http://trac.webkit.org/changeset/83762>