Bug 36512 – Expose constructor functions for instanceof checks of WebGL objects

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Currently the constructor functions for types like WebGLBuffer and WebGLRenderBuffer are not exposed on the DOMWindow object. This presents problems when trying to determine the type of these objects. The JavaScript instanceof operator would support type checks against these objects, if the constructor functions were exposed on the DOMWindow under the appropriate type names.

I haven't checked the Web IDL specification to understand whether support for these type checks is supposed to be implicit, in which case this would simply be a bug in WebKit's JavaScript bindings for WebGL. If not then this issue may need to be discussed with the WebGL working group.

[Zhenyao Mo]

Created attachment 76128 [details]
patch

[Chris Marrin]

This worried me at first, because it seems to indicate that this change will allow you to construct and object with "new WebGLBuffer()", which would be anything from useless to very bad. But Ollie helped me understand that the word "constructor" in this context is a complete misnomer and these new objects will properly throw exceptions if something that is attempted.

Still might be useful to create a test that attempts to create them and see that it properly fails.

[Kenneth Russell]

Comment on attachment 76128 [details]
patch

As Chris points out the test really does need to ensure that attempts to call these constructors throw exceptions. r- for this reason. Looks good otherwise.

[Zhenyao Mo]

(In reply to comment #2)
> This worried me at first, because it seems to indicate that this change will allow you to construct and object with "new WebGLBuffer()", which would be anything from useless to very bad. But Ollie helped me understand that the word "constructor" in this context is a complete misnomer and these new objects will properly throw exceptions if something that is attempted.
> 
> Still might be useful to create a test that attempts to create them and see that it properly fails.

CanBeConstructed needs to be in the idl interface and probably some extra hook-up if you wanna do new T().

I'll add the test cases.

[Chris Marrin]

Comment on attachment 76128 [details]
patch

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

> WebCore/bindings/generic/RuntimeEnabledFeatures.h:122
> +    static bool webGLUniformLocationEnabled() { return isWebGLEnabled; }

Why are you not handling WebGLObject and WebGLContextAttributes here (and throughout the rest of the patch)? Also should obsolete objects like WebGLFloatArray be removed?

[Chris Marrin]

(In reply to comment #4)
> (In reply to comment #2)
> > This worried me at first, because it seems to indicate that this change will allow you to construct and object with "new WebGLBuffer()", which would be anything from useless to very bad. But Ollie helped me understand that the word "constructor" in this context is a complete misnomer and these new objects will properly throw exceptions if something that is attempted.
> > 
> > Still might be useful to create a test that attempts to create them and see that it properly fails.
> 
> CanBeConstructed needs to be in the idl interface and probably some extra hook-up if you wanna do new T().

Just to be clear, we definitely don't want to be able to do new T() on any of these objects. The tests are just to make sure that is the case.

[Zhenyao Mo]

Created attachment 76138 [details]
Patch

[Kenneth Russell]

Comment on attachment 76138 [details]
Patch

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

Looks good. One minor grammatical correction, fix on landing.

> LayoutTests/fast/canvas/webgl/instanceof-test.html:67
> +        testPassed('new ' + objectName + ' throw an error');

throw -> threw

[Zhenyao Mo]

(In reply to comment #5)
> (From update of attachment 76128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76128&action=review
> 
> > WebCore/bindings/generic/RuntimeEnabledFeatures.h:122
> > +    static bool webGLUniformLocationEnabled() { return isWebGLEnabled; }
> 
> Why are you not handling WebGLObject and WebGLContextAttributes here (and throughout the rest of the patch)? Also should obsolete objects like WebGLFloatArray be removed?

WebGLObject is not exposed.  As for WebGLContextAttributes and WebGLContextEvent, I am not sure if it's worth exposing the constructors.  If yes, we can add them in a follow-up patch, so is the clean-up.

[Zhenyao Mo]

Committed r73669: <http://trac.webkit.org/changeset/73669>