[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