Bug 36512 - Expose constructor functions for instanceof checks of WebGL objects
Summary: Expose constructor functions for instanceof checks of WebGL objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-23 15:53 PDT by Kenneth Russell
Modified: 2010-12-09 17:59 PST (History)
6 users (show)

See Also:


Attachments
patch (16.52 KB, patch)
2010-12-09 15:46 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (17.84 KB, patch)
2010-12-09 17:08 PST, 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 Kenneth Russell 2010-03-23 15:53:13 PDT
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 Zhenyao Mo 2010-12-09 15:46:30 PST
Created attachment 76128 [details]
patch
Comment 2 Chris Marrin 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 Kenneth Russell 2010-12-09 16:34:23 PST
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.
Comment 4 Zhenyao Mo 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 Chris Marrin 2010-12-09 16:36:16 PST
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?
Comment 6 Chris Marrin 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 Zhenyao Mo 2010-12-09 17:08:15 PST
Created attachment 76138 [details]
Patch
Comment 8 Kenneth Russell 2010-12-09 17:15:24 PST
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
Comment 9 Zhenyao Mo 2010-12-09 17:55:22 PST
(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.
Comment 10 Zhenyao Mo 2010-12-09 17:59:12 PST
Committed r73669: <http://trac.webkit.org/changeset/73669>