Summary: | Implement strict type checking in js bindings for WebGL functions | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zhenyao Mo <zmo> | ||||||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cmarrin, dglazkov, kbr, kling | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Zhenyao Mo
2010-08-18 13:53:47 PDT
Created attachment 64769 [details]
patch
Tested both in Safari and Chromium.
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. 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.
Created attachment 64804 [details]
revised patch: responding to kbr's review
Changed from TYPE_MIS_MATCH to TypeError.
Again, tested in Safari and Chromium.
Created attachment 64805 [details]
revised patch: tiny fix
remove a change in shader file that shouldn't be in this patch
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. Created attachment 64871 [details]
revised patch: fixing the error in the comment
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
(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. Committed r65689: <http://trac.webkit.org/changeset/65689> |