[webkit-reviews] review denied: [Bug 84555] Add support for the Blob constructor : [Attachment 138307] Filled in the V8 code

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 84555: Add support for the Blob constructor
https://bugs.webkit.org/show_bug.cgi?id=84555

Attachment 138307: Filled in the V8 code
https://bugs.webkit.org/attachment.cgi?id=138307&action=review

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


Just commented on binding code.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:69
> +    if (!exec->argumentCount())
> +	   return throwVMError(exec, createSyntaxError(exec, "Not enough
arguments"));

This should be createTypeError().

Also please move this argument check to the head of this method.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:84
> +	   if (!blobPropertyBagValue.isUndefinedOrNull()) {

Maybe this should be 'blobPropertyBagValue.isObject()'?

> Source/WebCore/bindings/js/JSBlobCustom.cpp:89
> +	       // Create the dictionary wrapper from the initializer object.
> +	       JSDictionary dictionary(exec, blobPropertyBagObject);

Please use Dictionary instead of JSDictionary. We are going to remove direct
usages of JSDictionary.

exec->HadExcepton() check please.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:92
> +	       if (!dictionary.tryGetProperty("type", type))

You might want to use dictionary.get().

> Source/WebCore/bindings/js/JSBlobCustom.cpp:98
> +	       if (exec->hadException())
> +		   return JSValue::encode(jsUndefined());

Remove this.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:68
> +	   return throwError("Not enough arguments", V8Proxy::SyntaxError);

This should be V8Proxy::TypeError.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:78
> +    v8::Local<v8::Array> blobParts = v8::Local<v8::Array>::Cast(firstArg);

You might want to use EXCEPTION_BLOCK here.

Just an out-of-curiosity question: Is there any reason to use v8::Local<*>
instead of v8::Handle<*>?

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:83
> +    if (args.Length() > 1 && !isUndefinedOrNull(args[1]) &&
args[1]->IsObject()) {

'!isUndefinedOrNull(args[1])' would not be necessary. If args[1] is undefined
or null, 'args[1]->IsObject()' will return false.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:85
> +	   v8::Local<v8::Value> v8Type = object->Get(v8::String::New("type"));

Could you use Dictionary, just like JSC binding code?

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:86
> +	   if (!v8Type.IsEmpty() && !isUndefinedOrNull(v8Type)) {

I am not sure if we should check '!isUndefinedOrNull(v8Type)'. Is this speced?

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:91
> +	   if (!v8Endings.IsEmpty() && !isUndefinedOrNull(v8Endings)) {

Ditto.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:105
> +	   v8::Local<v8::Value> item = blobParts->Get(v8::Int32::New(i));

You might want to use v8::Uint32::New(i).