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.
Created attachment 138662 [details] Patch
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.
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.
(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.
What about the auto-generated bindings? Are they throwing SyntaxError or TypeError?
(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.
Created attachment 139484 [details] Patch
(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?
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?
(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.
(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?
(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.
Comment on attachment 139484 [details] Patch Clearing flags on attachment: 139484 Committed r115673: <http://trac.webkit.org/changeset/115673>
All reviewed patches have been landed. Closing bug.