Bug 84787 – WebGLRenderingContext methods should throw TypeError for not enough arguments

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kentaro Hara]

Currently, WebGLRenderingcontext methods implement "Not enough arguments" error as SyntaxError. The Web IDL spec requires that it should be TypeError:
http://www.w3.org/TR/WebIDL/#dfn-overload-resolution-algorithm

I wanted to confirm the behavior of Firefox and Opera, but they do not implement WebGL yet.

[Kentaro Hara]

Created attachment 138662 [details]
Patch

[Kenneth Russell]

It should be possible to get WebGL to work within Firefox with an up-to-date build, either Beta or Aurora. Also, Opera Next (currently 12) should implement WebGL.

It would be good to align this error behavior with that of Web IDL.

Ideally tests would be added to the WebGL conformance suite verifying this behavior. See http://www.khronos.org/webgl/wiki/Testing/Conformance . I am not sure whether all browser vendors would be in favor of doing so. Previously there was some agreement that we would not tie the WebGL implementation to proper implementation of all of the corner case behaviors of Web IDL. As far as I know, doing so would cause some significant problems in the handling of numbers.

[Kenneth Russell]

Comment on attachment 138662 [details]
Patch

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

The change seems positive overall but some cleanups are needed. Please also work with zmo to incorporate this test into the WebGL conformance test suite.

> Source/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:146
> +        return throwError(exec, createTypeError(exec, "Not enough arguments"));

Please refactor this so that the string isn't duplicated throughout this file.

> Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:220
> +        return throwError("Not enough arguments");

Same comment about refactoring here.

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 138662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138662&action=review
> 
> The change seems positive overall but some cleanups are needed. Please also work with zmo to incorporate this test into the WebGL conformance test suite.
> 
> > Source/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:146
> > +        return throwError(exec, createTypeError(exec, "Not enough arguments"));
> 
> Please refactor this so that the string isn't duplicated throughout this file.
> 
> > Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:220
> > +        return throwError("Not enough arguments");
> 
> Same comment about refactoring here.

OK. Two notes:

- Almost all methods are throwing TypeError for not enough arguments. Now WebGLRenderingContextCustom is an exception.
- All methods in JSC/V8 custom bindings are hard-coding "Not enough arguments". (Too many "Not enough arguments" in the world!:-) I agree we should make throwNotEnoughArgumentError(). I'll do it.

[Zhenyao Mo]

What about the auto-generated bindings?  Are they throwing SyntaxError or TypeError?

[Kentaro Hara]

(In reply to comment #5)
> What about the auto-generated bindings?  Are they throwing SyntaxError or TypeError?

TypeError. Now I've been fixing the bug of custom bindings. There are ~10 bindings that throw wrong type of errors.

[Kentaro Hara]

Created attachment 139484 [details]
Patch

[Kentaro Hara]

(In reply to comment #3)
> The change seems positive overall but some cleanups are needed.
>
> > Source/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:146
> > +        return throwError(exec, createTypeError(exec, "Not enough arguments"));
> 
> Please refactor this so that the string isn't duplicated throughout this file.

Done.
 
Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:220
> > +        return throwError("Not enough arguments");
> 
> Same comment about refactoring here.

Done.

> Please also work with zmo to incorporate this test into the WebGL conformance test suite.

zmo: Do you have any idea on this?

[Kenneth Russell]

Comment on attachment 139484 [details]
Patch

Looks good. r=me

Thinking more about incorporating this test into the WebGL conformance suite: that suite doesn't use test expectations, so it would be necessary to explicitly check the exception object and verify that it is a TypeError. Is that feasible?

[Kentaro Hara]

(In reply to comment #9)
> Thinking more about incorporating this test into the WebGL conformance suite: that suite doesn't use test expectations, so it would be necessary to explicitly check the exception object and verify that it is a TypeError. Is that feasible?

I think it's easy. I'll look into the conformance suite.

[Kentaro Hara]

(In reply to comment #9)
> Thinking more about incorporating this test into the WebGL conformance suite

kbr: I'll add the test to the conformance test later, but should I remove fast/canvas/webgl/webgl-exceptions.html from the WebKit patch?

[Kenneth Russell]

(In reply to comment #11)
> kbr: I'll add the test to the conformance test later, but should I remove fast/canvas/webgl/webgl-exceptions.html from the WebKit patch?

No, please leave it in the patch; the more testing the better.

[WebKit Review Bot]

Comment on attachment 139484 [details]
Patch

Clearing flags on attachment: 139484

Committed r115673: <http://trac.webkit.org/changeset/115673>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.