[webkit-reviews] review denied: [Bug 91056] [V8Bindings] Implement generalised method to validates that the passed object is a sequence type. : [Attachment 151891] Updated Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>'s request for review:
Bug 91056: [V8Bindings] Implement generalised method to validates that the
passed object is a sequence type.
https://bugs.webkit.org/show_bug.cgi?id=91056

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

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


> Source/WebCore/bindings/v8/V8Binding.h:447
> +	       V8Proxy::throwTypeError("Type error");

To align with toJSSequence(), this could be V8Proxy::throwTypeError(). (though
it would just print "Type error".)

> Source/WebCore/bindings/v8/V8Binding.h:448
> +	       return v8::Local<v8::Object>();

You can simply write 'return V8Proxy::throwTypeError()'.

> Source/WebCore/bindings/v8/V8Binding.h:453
> +	   v8::Local<v8::Value> lengthValue =
object->Get(v8::String::New("length"));

Isn't there any possibility that this code throws exception? Then we need to
use TryCatch. (See macros in V8BindingMacros.h)

> Source/WebCore/bindings/v8/V8Binding.h:457
> +	       V8Proxy::throwTypeError("Type error");
> +	       return v8::Local<v8::Object>();

return V8Proxy::throwTypeError();

> Source/WebCore/bindings/v8/V8Binding.h:463
> +	   if (!lengthValue->IsNumber()) {
> +	       V8Proxy::throwTypeError("Type error");
> +	       return v8::Local<v8::Object>();
> +	   }

Is this check needed? toJSSequence() does not have the check.

> Source/WebCore/bindings/v8/V8Binding.h:465
> +	   length = lengthValue->Uint32Value();

Isn't there any possibility that this code throws exception? Then we need to
use TryCatch. (See macros in V8BindingMacros.h)