[webkit-reviews] review denied: [Bug 46541] Implement DataView interface from Typed Array specification : [Attachment 74797] Proposed Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 24 15:57:47 PST 2010
Kenneth Russell <kbr at google.com> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 46541: Implement DataView interface from Typed Array specification
https://bugs.webkit.org/show_bug.cgi?id=46541
Attachment 74797: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=74797&action=review
------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=74797&action=review
This looks excellent. I apologize but there are a couple of issues in the
custom bindings which I didn't notice the first time. Let me know if you'd
prefer to fix them upon landing the patch, but for now marking r-.
> WebCore/bindings/js/JSDataViewCustom.cpp:151
> + if (value < -128 || value > 127)
> + return throwError(exec, createSyntaxError(exec, "The value to
set to is out of range"));
> + imp->setInt8(byteOffset, static_cast<unsigned char>(value), ec);
unsigned char -> char.
I don't think we should be raising exceptions for out-of-range values. Web IDL
and ECMA-262 specify the conversions from e.g. double to octet, and they don't
involve throwing exceptions.
> WebCore/bindings/js/JSDataViewCustom.cpp:155
> + if (value < 0 || value > 255)
> + return throwError(exec, createSyntaxError(exec, "The value to
set to is out of range"));
Here as well.
> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:64
> + if (!isUndefinedOrNull(args[0]) && !args[0]->IsNumber() &&
!args[0]->IsBoolean()) {
> + V8Proxy::throwTypeError();
> + return notHandledByInterceptor();
> + }
The behavior of the StrictTypeChecking extended attribute in WebKit IDL was
recently changed so that it is more compliant to ECMA-262. See
https://bugs.webkit.org/show_bug.cgi?id=49218 . This code which converts to
numbers needs to be changed to match the current autogenerated code, which
doesn't throw exceptions.
> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:85
> + if (!isUndefinedOrNull(args[0]) && !args[0]->IsNumber() &&
!args[0]->IsBoolean()) {
> + V8Proxy::throwTypeError();
> + return notHandledByInterceptor();
> + }
Same here.
> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:110
> + if (!isUndefinedOrNull(args[0]) && !args[0]->IsNumber() &&
!args[0]->IsBoolean()) {
> + V8Proxy::throwTypeError();
> + return notHandledByInterceptor();
> + }
> + if (!isUndefinedOrNull(args[1]) && !args[1]->IsNumber() &&
!args[1]->IsBoolean()) {
> + V8Proxy::throwTypeError();
> + return notHandledByInterceptor();
> + }
Shouldn't throw exceptions for conversions to numbers.
> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:117
> + if (value < -128 || value > 127)
> + return throwError("The value to set to is out of range",
V8Proxy::SyntaxError);
Shouldn't throw exceptions for out-of-range values.
> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:136
> + if (!isUndefinedOrNull(args[0]) && !args[0]->IsNumber() &&
!args[0]->IsBoolean()) {
> + V8Proxy::throwTypeError();
> + return notHandledByInterceptor();
> + }
> + if (!isUndefinedOrNull(args[1]) && !args[1]->IsNumber() &&
!args[1]->IsBoolean()) {
> + V8Proxy::throwTypeError();
> + return notHandledByInterceptor();
> + }
Shouldn't throw exceptions for conversions to numbers.
> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:143
> + if (value < 0 || value > 255)
> + return throwError("The value to set to is out of range",
V8Proxy::SyntaxError);
Shouldn't throw exceptions for out-of-range values.
More information about the webkit-reviews
mailing list