Bug 36512 - Expose constructor functions for instanceof checks of WebGL objects
: Expose constructor functions for instanceof checks of WebGL objects
Status: RESOLVED FIXED
: WebKit
WebGL
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-03-23 15:53 PST by
Modified: 2010-12-09 17:59 PST (History)


Attachments
patch (16.52 KB, patch)
2010-12-09 15:46 PST, Zhenyao Mo
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.84 KB, patch)
2010-12-09 17:08 PST, Zhenyao Mo
kbr: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-23 15:53:13 PST
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.
------- Comment #1 From 2010-12-09 15:46:30 PST -------
Created an attachment (id=76128) [details]
patch
------- Comment #2 From 2010-12-09 16:27:42 PST -------
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.
------- Comment #3 From 2010-12-09 16:34:23 PST -------
(From update of attachment 76128 [details])
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.
------- Comment #4 From 2010-12-09 16:36:06 PST -------
(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.
------- Comment #5 From 2010-12-09 16:36:16 PST -------
(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?
------- Comment #6 From 2010-12-09 16:37:38 PST -------
(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.
------- Comment #7 From 2010-12-09 17:08:15 PST -------
Created an attachment (id=76138) [details]
Patch
------- Comment #8 From 2010-12-09 17:15:24 PST -------
(From update of attachment 76138 [details])
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
------- Comment #9 From 2010-12-09 17:55:22 PST -------
(In reply to comment #5)
> (From update of attachment 76128 [details] [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.
------- Comment #10 From 2010-12-09 17:59:12 PST -------
Committed r73669: <http://trac.webkit.org/changeset/73669>