Bug 58410 - Extension3D needs to provide a way to check if an extension is enabled
Summary: Extension3D needs to provide a way to check if an extension is enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-12 18:39 PDT by Zhenyao Mo
Modified: 2011-04-13 14:09 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.59 KB, patch)
2011-04-13 11:53 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (10.33 KB, patch)
2011-04-13 13:41 PDT, Zhenyao Mo
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2011-04-12 18:39:27 PDT
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.
Comment 1 Zhenyao Mo 2011-04-13 11:53:29 PDT
Created attachment 89420 [details]
Patch
Comment 2 Kenneth Russell 2011-04-13 12:31:53 PDT
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.
Comment 3 Zhenyao Mo 2011-04-13 12:59:58 PDT
(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.
Comment 4 Kenneth Russell 2011-04-13 13:06:11 PDT
(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.
Comment 5 Zhenyao Mo 2011-04-13 13:41:53 PDT
Created attachment 89445 [details]
Patch
Comment 6 Kenneth Russell 2011-04-13 13:47:17 PDT
Comment on attachment 89445 [details]
Patch

Thanks. Looks good.
Comment 7 Zhenyao Mo 2011-04-13 14:09:00 PDT
Committed r83762: <http://trac.webkit.org/changeset/83762>