Bug 44202 – Implement strict type checking in js bindings for WebGL functions

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

An exception will be thrown in case of type mismatch for Wrapper types and DomString, the same behavior as for overloaded functions.

[Zhenyao Mo]

Created attachment 64769 [details]
patch

Tested both in Safari and Chromium.

[Zhenyao Mo]

Forget to mention: all the current demos run fine after this change.  The test has been added in khronos already and was copied over using kbr's script.

[Kenneth Russell]

Comment on attachment 64769 [details]
patch

Good work overall. A few minor issues.

WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:171
 +          setDOMException(exec, TYPE_MISMATCH_ERR);
In the JSC custom bindings and in CodeGeneratorJS.pm I think you should use "return throwVMTypeError(exec)", which is used for the overloaded method handling's fall-through case. The code generator is used to generate bindings to objects which aren't strictly part of the DOM (like the TypedArrays, for example), so I think it is more appropriate to throw an exception that is not DOM-specific.


WebCore/bindings/scripts/CodeGeneratorJS.pm:1967
 +                              # an TYPE_MISMATCH_ERR is thrown instead of casting to null.
See above about using throwVMTypeError instead.


WebCore/bindings/scripts/CodeGeneratorV8.pm:1249
 +              # an TYPE_MISMATCH_ERR is thrown instead of casting to null.
I think this should use V8Proxy::throwTypeError to match the behavior of the overloaded function generation.


WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:231
 +          return throwError(TYPE_MISMATCH_ERR);
I think the V8 custom bindings should use V8Proxy::throwTypeError per above.

[Zhenyao Mo]

Created attachment 64804 [details]
revised patch: responding to kbr's review

Changed from TYPE_MIS_MATCH to TypeError.

Again, tested in Safari and Chromium.

[Zhenyao Mo]

Created attachment 64805 [details]
revised patch: tiny fix

remove a change in shader file that shouldn't be in this patch

[Andreas Kling]

Comment on attachment 64805 [details]
revised patch: tiny fix

>WebCore/bindings/scripts/CodeGeneratorV8.pm:1249
> +              # an TYPE_MISMATCH_ERR is thrown instead of casting to null.
Incorrect comment. It throws TypeError, not TYPE_MISMATCH_ERR.

[Zhenyao Mo]

Created attachment 64871 [details]
revised patch: fixing the error in the comment

[Kenneth Russell]

Comment on attachment 64871 [details]
revised patch: fixing the error in the comment

Looks fine. Minor grammatical comments, not strictly necessary to fix.

WebCore/bindings/scripts/CodeGeneratorJS.pm:1967
 +                              # an TypeError is thrown instead of casting to null.
an -> a

WebCore/bindings/scripts/CodeGeneratorV8.pm:1249
 +              # an TypeError is thrown instead of casting to null.
an -> a

[Zhenyao Mo]

(In reply to comment #8)
> (From update of attachment 64871 [details])
> Looks fine. Minor grammatical comments, not strictly necessary to fix.
> 
> WebCore/bindings/scripts/CodeGeneratorJS.pm:1967
>  +                              # an TypeError is thrown instead of casting to null.
> an -> a
> 
> WebCore/bindings/scripts/CodeGeneratorV8.pm:1249
>  +              # an TypeError is thrown instead of casting to null.
> an -> a

I'll fix it on the landing patch.

[Zhenyao Mo]

Committed r65689: <http://trac.webkit.org/changeset/65689>