[webkit-reviews] review denied: [Bug 96798] [V8] Binding: Implement EnforceRange IDL Attribute for long long conversions : [Attachment 188184] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Joshua Bell
<jsbell at chromium.org>'s request for review:
Bug 96798: [V8] Binding: Implement EnforceRange IDL Attribute for long long
conversions
https://bugs.webkit.org/show_bug.cgi?id=96798

Attachment 188184: Patch
https://bugs.webkit.org/attachment.cgi?id=188184&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188184&action=review


> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3928
> +sub ConversionThrowsTypeError

Will this method be useful for other purpose? Otherwise, I would hard-code
$signature->extendedAttributes->{"EnforceRange"} in call sites and remove the
method.

> Source/WebCore/bindings/v8/V8Binding.cpp:115
>      v8::Local<v8::Number> numberObject = value->ToNumber();

This can throw. You need a try-catch block.

> Source/WebCore/bindings/v8/V8Binding.cpp:121
> +    if (attributes & FEnforceRange) {

[EnforgeRange] must not be in conjunction with [Clamp]:
http://www.w3.org/TR/WebIDL/#EnforceRange

So it would be useless to use a bit-masking approach.

> Source/WebCore/bindings/v8/V8Binding.cpp:128
> +	   if (x < -0x8000000 || x > 0x7fffffff) {

Nit: Shall we use a constant variable for these magic numbers?

> Source/WebCore/bindings/v8/V8Binding.cpp:166
> +    if (attributes & FEnforceRange) {

Ditto.

> Source/WebCore/bindings/v8/V8Binding.cpp:173
> +	   if (x < 0 || x > 0xffffffff) {

Nit: Use a constant variable for the magic number.

> Source/WebCore/bindings/v8/V8Binding.cpp:198
> +    v8::Local<v8::Number> numberObject = value->ToNumber();

This can throw. You need a try-catch block.

> Source/WebCore/bindings/v8/V8Binding.cpp:206
> +    if (attributes & FEnforceRange) {

Ditto.

> Source/WebCore/bindings/v8/V8Binding.cpp:237
> +	   if (result >= 0)
> +	       return result;

Can we also add a fast path for result < 0 ?

> Source/WebCore/bindings/v8/V8Binding.cpp:241
> +    v8::Local<v8::Number> numberObject = value->ToNumber();

This can throw. You need a try-catch block.

> Source/WebCore/bindings/v8/V8Binding.cpp:249
> +    if (attributes & FEnforceRange) {

Ditto.

> Source/WebCore/bindings/v8/V8Binding.h:348
> +    enum ConversionAttributes {

Nit: IntergerConversionConfiguration?

> Source/WebCore/bindings/v8/V8Binding.h:351
> +	   FNone = 0x0000,
> +	   FEnforceRange = 0x0001,
> +	   // FIXME: Implement FClamp = 0x0002

Nit: Are these hard-coded values (e.g. 0x0001) meaningful? Otherwise, we can
drop them.

Nit: Also is "FEnforceRange" a speced name somewhere? Otherwise, we can simply
say "EnforceRange".

> Source/WebCore/bindings/v8/V8BindingMacros.h:46
> +	   bool ok = true;			     \
> +	   var = (value);			     \
> +	   if (UNLIKELY(!ok)) { 		     \

Where is |ok| updated?

> Source/WebCore/bindings/v8/V8BindingMacros.h:71
> +	   if (UNLIKELY(!ok))					\

Ditto.