[webkit-reviews] review requested: [Bug 84555] Add support for the Blob constructor : [Attachment 139226] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has asked  for review:
Bug 84555: Add support for the Blob constructor
https://bugs.webkit.org/show_bug.cgi?id=84555

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

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


Sorry for the late comments. I'd be happy if you could land a follow-up patch.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:78
> +    unsigned length = array->length();

Nit: Let's move this just before 'for (unsigned i = 0; i < length; i++)'.

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

Move this just below 'JSDictionary dictionary()'.

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

Why is this check needed?

> Source/WebCore/bindings/js/JSBlobCustom.cpp:130
> +		   return JSValue::encode(JSValue());

Nit: JSValue() => jsUndefined(), just for consistency in the same method.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:65
> +	   return throwError("Blob constructor's associated frame is not
available", V8Proxy::ReferenceError);

Please make this error message consistent with the JSC's one.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:69
> +	   return toV8(blob.get());

Please pass Isolate. 'toV8(blob.get(), args.GetIsolate())'

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

I wonder why you use IsObject() in V8 but isUndefinedOrNull() in JSC.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:82
> +	   Dictionary dictionary(args[1]);

Use EXCEPTION_BLOCK() here. This can throw.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:116
> +    return toV8(blob.get());

toV8(blob.get(), args.GetIsolate())