Bug 84555 – Add support for the Blob constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Sam Weinig]

The File API spec has been updated to include a constructor for Blob (http://dev.w3.org/2006/webapi/FileAPI/#constructorBlob).

[Sam Weinig]

Created attachment 138279 [details]
Patch

[Sam Weinig]

The attached patch implements the constructor for JSC, but not for V8.  I am not set up to do that, so ideally someone from the V8 side could fill in that bit.

[WebKit Review Bot]

Attachment 138279 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:62:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 138279 [details]
Patch

Attachment 138279 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12469716

[Kinuko Yasuda]

Created attachment 138307 [details]
Filled in the V8 code

Added constructor code for V8 based on Sam's patch.

[Kentaro Hara]

Comment on attachment 138307 [details]
Filled in the V8 code

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).

[Kinuko Yasuda]

Created attachment 138324 [details]
new patch with v8 code

[Kinuko Yasuda]

Thanks, I've updated the binding code.

View in context: https://bugs.webkit.org/attachment.cgi?id=138307&action=review

>> Source/WebCore/bindings/js/JSBlobCustom.cpp:69
>> +        return throwVMError(exec, createSyntaxError(exec, "Not enough arguments"));
> 
> This should be createTypeError().
> 
> Also please move this argument check to the head of this method.

Done.

>> Source/WebCore/bindings/js/JSBlobCustom.cpp:84
>> +        if (!blobPropertyBagValue.isUndefinedOrNull()) {
> 
> Maybe this should be 'blobPropertyBagValue.isObject()'?

Done.

>> Source/WebCore/bindings/js/JSBlobCustom.cpp:89
>> +            JSDictionary dictionary(exec, blobPropertyBagObject);
> 
> Please use Dictionary instead of JSDictionary. We are going to remove direct usages of JSDictionary.
> 
> exec->HadExcepton() check please.

Done.

>> Source/WebCore/bindings/js/JSBlobCustom.cpp:92
>> +            if (!dictionary.tryGetProperty("type", type))
> 
> You might want to use dictionary.get().

Done.

>> Source/WebCore/bindings/js/JSBlobCustom.cpp:98
>> +                return JSValue::encode(jsUndefined());
> 
> Remove this.

Done.

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

Done.

>> 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<*>?

It looked there objects could be simply stack allocated since they're only used within this function, but I'm not very familiar with the v8 conventions and I'd follow if you have recommendations.

>> 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.

Sounds like true.  Done.

>> 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?

Done.

>> 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?

Likely not. (Removed this code anyway)

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

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).

Done.

[Gyuyoung Kim]

Comment on attachment 138324 [details]
new patch with v8 code

Attachment 138324 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12477759

[Kinuko Yasuda]

Created attachment 138331 [details]
new patch with v8 code

efl build fix.

[Kentaro Hara]

Comment on attachment 138331 [details]
new patch with v8 code

View in context: https://bugs.webkit.org/attachment.cgi?id=138331&action=review

r+ for the binding part an the IDL part. I am not familiar with the blob test cases though.

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

Nit: Shall we move this to just before 'for (unsigned i = 0; i < length; ++i)'?

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

Nit: JSValue() => jsUndefined(). Both are OK but let's make it consistent in the same method.

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:99
> +    for (uint32_t i = 0; i < blobParts->Length(); ++i) {

Nit: uint32_t => unsinged. Let's make it consistent between JSC and V8.

[Kentaro Hara]

(In reply to comment #8)
> > Just an out-of-curiosity question: Is there any reason to use v8::Local<*> instead of v8::Handle<*>?
> 
> It looked there objects could be simply stack allocated since they're only used within this function, but I'm not very familiar with the v8 conventions and I'd follow if you have recommendations.

Both are OK. Handle is just a base class of Local and Persistent. We often use Handle for these cases since we can pass the variable around without considering whether it is Local or Persistent. I was just wondering it since you were mixing Handle and Local in your first patch.

[Sam Weinig]

At least my part, was not ready to be reviewed ( I am pretty sure I didn't mark it r?).

[Sam Weinig]

(In reply to comment #6)
> (From update of attachment 138307 [details])
> 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.
> 

This is actually completely wrong. The spec says the Blob constructor should support being called with no arguments.

> > Source/WebCore/bindings/js/JSBlobCustom.cpp:84
> > +        if (!blobPropertyBagValue.isUndefinedOrNull()) {
> 
> Maybe this should be 'blobPropertyBagValue.isObject()'?

This is the idiomatic way of doing this that I added when doing the EventInit work.

> 
> > 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.

Why?  What is the benefit of the added abstraction. Why have both?

> 
> exec->HadExcepton() check please.
> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:92
> > +            if (!dictionary.tryGetProperty("type", type))
> 
> You might want to use dictionary.get().

Again, why?

[Sam Weinig]

In addition to no having completed writing my patch, I had not yet added the test case.  Please let me finish :D.

[Kentaro Hara]

Comment on attachment 138307 [details]
Filled in the V8 code

View in context: https://bugs.webkit.org/attachment.cgi?id=138307&action=review

>>> Source/WebCore/bindings/js/JSBlobCustom.cpp:84
>>> +        if (!blobPropertyBagValue.isUndefinedOrNull()) {
>> 
>> Maybe this should be 'blobPropertyBagValue.isObject()'?
> 
> This is the idiomatic way of doing this that I added when doing the EventInit work.

I am not sure why this is an idiomatic way, but if you want to use the idiom, you need to check exec->hadException() after toObject(). toObject() can throw exception. (In that sense, I think that isObject() check would be better.)

>>> Source/WebCore/bindings/js/JSBlobCustom.cpp:89
>>> +            JSDictionary dictionary(exec, blobPropertyBagObject);
>> 
>> Please use Dictionary instead of JSDictionary. We are going to remove direct usages of JSDictionary.
>> 
>> exec->HadExcepton() check please.
> 
> Why?  What is the benefit of the added abstraction. Why have both?

There has been work to make js/Dictionary.h and v8/Dictionary.h consistent, so that we can use .get() to access a dictionary in both bindings. (e.g. https://bugs.webkit.org/show_bug.cgi?id=80477) We do not want to have both, and we are killing the direct usage of JSDictionary.

>>> Source/WebCore/bindings/js/JSBlobCustom.cpp:92
>>> +            if (!dictionary.tryGetProperty("type", type))
>> 
>> You might want to use dictionary.get().
> 
> Again, why?

Ditto.

[David Levin]

Comment on attachment 138331 [details]
new patch with v8 code

r- if nothing else because Sam hasn't finished :)

[WebKit Review Bot]

Comment on attachment 138279 [details]
Patch

Attachment 138279 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12517048

[Kinuko Yasuda]

(In reply to comment #15)
> In addition to no having completed writing my patch, I had not yet added the test case.  Please let me finish :D.

Sorry looks like I was too hasty!

[Sam Weinig]

After talking to the spec editor, it looks like this part of the spec is still being ironed out.  Simon Pieters filed a bunch of really good bugs on it, all of which I agree with:
- https://www.w3.org/Bugs/Public/show_bug.cgi?id=16721
- https://www.w3.org/Bugs/Public/show_bug.cgi?id=16723
- https://www.w3.org/Bugs/Public/show_bug.cgi?id=16724
- https://www.w3.org/Bugs/Public/show_bug.cgi?id=16727
- https://www.w3.org/Bugs/Public/show_bug.cgi?id=16729
- https://www.w3.org/Bugs/Public/show_bug.cgi?id=16732
- https://www.w3.org/Bugs/Public/show_bug.cgi?id=16746

[Sam Weinig]

(In reply to comment #16)
> (From update of attachment 138307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138307&action=review
> 
> >>> Source/WebCore/bindings/js/JSBlobCustom.cpp:84
> >>> +        if (!blobPropertyBagValue.isUndefinedOrNull()) {
> >> 
> >> Maybe this should be 'blobPropertyBagValue.isObject()'?
> > 
> > This is the idiomatic way of doing this that I added when doing the EventInit work.
> 
> I am not sure why this is an idiomatic way, but if you want to use the idiom, you need to check exec->hadException() after toObject(). toObject() can throw exception. (In that sense, I think that isObject() check would be better.)

I am pretty sure that if the value is not undefined or null, toObject can't throw.

> 
> >>> Source/WebCore/bindings/js/JSBlobCustom.cpp:89
> >>> +            JSDictionary dictionary(exec, blobPropertyBagObject);
> >> 
> >> Please use Dictionary instead of JSDictionary. We are going to remove direct usages of JSDictionary.
> >> 
> >> exec->HadExcepton() check please.
> > 
> > Why?  What is the benefit of the added abstraction. Why have both?
> 
> There has been work to make js/Dictionary.h and v8/Dictionary.h consistent, so that we can use .get() to access a dictionary in both bindings. (e.g. https://bugs.webkit.org/show_bug.cgi?id=80477) We do not want to have both, and we are killing the direct usage of JSDictionary.

The Dictionary interface seems like an odd mish-mash of stuff.  Why does it include EventTarget related code?  This seems like a case where we are trying to share code between JSC and V8, and thus far that has been a faulty strategy.  ScriptObject and ScriptValue continue to be error prone objects to use.

[Kentaro Hara]

(In reply to comment #21)
> The Dictionary interface seems like an odd mish-mash of stuff.  Why does it include EventTarget related code?  This seems like a case where we are trying to share code between JSC and V8, and thus far that has been a faulty strategy.  ScriptObject and ScriptValue continue to be error prone objects to use.

Basically -- I am not intending to say something strong on JSC bindings, and just wanted to point out that there has been work to introduce js/Dictionary.h and align interfaces between js/Dictionary.h and v8/Dictoinary.h (which was originally v8/OptionsObject.{h,cpp}).

> Why does it include EventTarget related code?

Do you mean 'class EventTarget' in JSDictionary.h? It is needed for converting EventTarget value, i.e. convertValue(JSC::ExecState*, JSC::JSValue, RefPtr<EventTarget>& result).

> ScriptObject and ScriptValue continue to be error prone objects to use.

How is this concern related to the indirection of Dictionary interfaces? Basically Dictionary.get() just redirects to JSDictionary.tryGetProperty().

[Sam Weinig]

(In reply to comment #22)
> (In reply to comment #21)
> > The Dictionary interface seems like an odd mish-mash of stuff.  Why does it include EventTarget related code?  This seems like a case where we are trying to share code between JSC and V8, and thus far that has been a faulty strategy.  ScriptObject and ScriptValue continue to be error prone objects to use.
> 
> Basically -- I am not intending to say something strong on JSC bindings, and just wanted to point out that there has been work to introduce js/Dictionary.h and align interfaces between js/Dictionary.h and v8/Dictoinary.h (which was originally v8/OptionsObject.{h,cpp}).
> 
> > Why does it include EventTarget related code?
> 
> Do you mean 'class EventTarget' in JSDictionary.h? It is needed for converting EventTarget value, i.e. convertValue(JSC::ExecState*, JSC::JSValue, RefPtr<EventTarget>& result).

Sorry, I meant the getEventListener() function. It seems out of place.  


> > ScriptObject and ScriptValue continue to be error prone objects to use.
> 
> How is this concern related to the indirection of Dictionary interfaces? Basically Dictionary.get() just redirects to JSDictionary.tryGetProperty().

The concern is with trying to create abstractions around the two JS engines.  In the past, it has not worked well.  Pieces were implemented for one, and not the other, and just lead to more trouble than it was worth.  It seems that is already the case here, with getWithUndefinedOrNullCheck() not being implemented for JSC.

[Sam Weinig]

Created attachment 139124 [details]
Patch

[WebKit Review Bot]

Attachment 139124 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:72:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Ok, I think this is ready for review now.  I am happy with my test case.

[Gyuyoung Kim]

Comment on attachment 139124 [details]
Patch

Attachment 139124 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12558104

[Darin Adler]

EFL build failure:


/mnt/eflews/git/webkit/Source/WebCore/bindings/js/JSBlobCustom.cpp: In static member function 'static void* WebCore::JSBlobConstructor::constructJSBlob(JSC::ExecState*)':
/mnt/eflews/git/webkit/Source/WebCore/bindings/js/JSBlobCustom.cpp:121: error: no matching function for call to 'WebCore::WebKitBlobBuilder::append(WTF::ArrayBuffer*)'
/mnt/eflews/git/webkit/Source/WebCore/fileapi/WebKitBlobBuilder.h:49: note: candidates are: void WebCore::WebKitBlobBuilder::append(WebCore::Blob*)
/mnt/eflews/git/webkit/Source/WebCore/fileapi/WebKitBlobBuilder.h:50: note:                 void WebCore::WebKitBlobBuilder::append(const WTF::String&, WebCore::ExceptionCode&)
/mnt/eflews/git/webkit/Source/WebCore/fileapi/WebKitBlobBuilder.h:51: note:                 void WebCore::WebKitBlobBuilder::append(const WTF::String&, const WTF::String&, WebCore::ExceptionCode&)

[Kinuko Yasuda]

(In reply to comment #28)
> EFL build failure:
> 
> /mnt/eflews/git/webkit/Source/WebCore/bindings/js/JSBlobCustom.cpp:121: error: no matching function for call to 'WebCore::WebKitBlobBuilder::append(WTF::ArrayBuffer*)'

WebKitBlobBuilder::append(ArrayBuffer) is not enabled on all platforms.
Since BlobBuilder is anyway going to be deprecated probably we should just remove '#if ENABLE(BLOB)' around append(ArrayBuffer) in WebKitBlobBuilder.{h,cpp,idl}?
(And if we do so I'd like the same conditional directive in V8BlobCustom.cpp to be removed)

[Sam Weinig]

Created attachment 139226 [details]
Patch

[WebKit Review Bot]

Attachment 139226 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:72:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

(In reply to comment #29)
> (In reply to comment #28)
> > EFL build failure:
> > 
> > /mnt/eflews/git/webkit/Source/WebCore/bindings/js/JSBlobCustom.cpp:121: error: no matching function for call to 'WebCore::WebKitBlobBuilder::append(WTF::ArrayBuffer*)'
> 
> WebKitBlobBuilder::append(ArrayBuffer) is not enabled on all platforms.
> Since BlobBuilder is anyway going to be deprecated probably we should just remove '#if ENABLE(BLOB)' around append(ArrayBuffer) in WebKitBlobBuilder.{h,cpp,idl}?
> (And if we do so I'd like the same conditional directive in V8BlobCustom.cpp to be removed)

I just added the correct #ifdef for now.

[Maciej Stachowiak]

Comment on attachment 139226 [details]
Patch

r=me

[Sam Weinig]

Committed r115484: <http://trac.webkit.org/changeset/115484>

[Kentaro Hara]

Comment on attachment 139226 [details]
Patch

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())

[Dimitri Glazkov (Google)]

This broke Chromium: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/23046/steps/compile/logs/stdio

Can haz rollout?

[Dimitri Glazkov (Google)]

(In reply to comment #36)
> This broke Chromium: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/23046/steps/compile/logs/stdio
> 
> Can haz rollout?

Haven't heard a response, reverted in http://trac.webkit.org/changeset/115491/

[Sam Weinig]

(In reply to comment #35)
> (From update of attachment 139226 [details])
> 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.

It's cool.  I got rolled out :D.

> 
> > 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()'.

I don't follow. This is checking if the toString() in get() threw an exception.

> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:103
> > +            if (exec->hadException())
> > +                return JSValue::encode(jsUndefined());
> 
> Why is this check needed?

Same here.

> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:130
> > +                return JSValue::encode(JSValue());
> 
> Nit: JSValue() => jsUndefined(), just for consistency in the same method.

Ok.

> 
> > 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.

Ok.

> 
> > Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:69
> > +        return toV8(blob.get());
> 
> Please pass Isolate. 'toV8(blob.get(), args.GetIsolate())'

Ok.

> 
> > 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.

I didn't write the V8 bindings.  It was provided for me.

> 
> > Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:82
> > +        Dictionary dictionary(args[1]);
> 
> Use EXCEPTION_BLOCK() here. This can throw.

What is the condition under which this throws?

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

Ok.

[Kentaro Hara]

Comment on attachment 139226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139226&action=review

>>> Source/WebCore/bindings/js/JSBlobCustom.cpp:98
>>> +                return JSValue::encode(jsUndefined());
>> 
>> Move this just below 'JSDictionary dictionary()'.
> 
> I don't follow. This is checking if the toString() in get() threw an exception.

Makes sense.

>>> 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.
> 
> I didn't write the V8 bindings.  It was provided for me.

Actually I am not sure. As you commented in the JSC side, we are not sure if we should use isUndefinedOrNull() or isObject() in this case. This is already confusing in the Event constructor. Maybe we can fix the behavior in another patch.

>>> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:82
>>> +        Dictionary dictionary(args[1]);
>> 
>> Use EXCEPTION_BLOCK() here. This can throw.
> 
> What is the condition under which this throws?

Sorry, you're right, this cannot throw.

[Kentaro Hara]

(In reply to comment #39)
> >> I wonder why you use IsObject() in V8 but isUndefinedOrNull() in JSC.
> > 
> > I didn't write the V8 bindings.  It was provided for me.
> 
> Actually I am not sure. As you commented in the JSC side, we are not sure if we should use isUndefinedOrNull() or isObject() in this case. This is already confusing in the Event constructor. Maybe we can fix the behavior in another patch.

But I think that it is important to align the behavior between JSC and V8. Would you add the following tests and confirm the test results?

new Blob(['hello'], null);
new Blob(['hello'], undefined);
new Blob(['hello'], 123);
new Blob(['hello'], 123.4);
new Blob(['hello'], []);
new Blob(['hello'], true);
new Blob(['hello'], "abc");
new Blob(['hello'], /abc/);
new Blob(['hello'], function () {});

[Sam Weinig]

(In reply to comment #40)
> (In reply to comment #39)
> > >> I wonder why you use IsObject() in V8 but isUndefinedOrNull() in JSC.
> > > 
> > > I didn't write the V8 bindings.  It was provided for me.
> > 
> > Actually I am not sure. As you commented in the JSC side, we are not sure if we should use isUndefinedOrNull() or isObject() in this case. This is already confusing in the Event constructor. Maybe we can fix the behavior in another patch.
> 
> But I think that it is important to align the behavior between JSC and V8. Would you add the following tests and confirm the test results?
> 
> new Blob(['hello'], null);
> new Blob(['hello'], undefined);
> new Blob(['hello'], 123);
> new Blob(['hello'], 123.4);
> new Blob(['hello'], []);
> new Blob(['hello'], true);
> new Blob(['hello'], "abc");
> new Blob(['hello'], /abc/);
> new Blob(['hello'], function () {});

Will do.

[Sam Weinig]

Created attachment 139283 [details]
Patch

[Kentaro Hara]

Sam: Did you update blob-constructor-expected.txt?

[WebKit Review Bot]

Attachment 139283 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:76:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

There are a couple of WebIDL issues here, still.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:80
> +    if (exec->argumentCount() > 1) {

This should happen after converting the array elements.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:83
> +        // FIXME: Should we throw if the blobPropertyBag is not an object?

Yes; a TypeError: <http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary>.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:98
> +            bool containsEndings = dictionary.get("endings", endings);

These properties should be got in lexicographic order; first endings and then type.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:104
> +                    setDOMException(exec, INVALID_STATE_ERR);

Per <https://www.w3.org/Bugs/Public/show_bug.cgi?id=16729> (mentioned in the ChangeLog) and <http://dev.w3.org/2006/webapi/WebIDL/#es-enumeration>, this should be a TypeError.

> Source/WebCore/bindings/js/JSBlobCustom.cpp:123
> +            blobBuilder->append(toArrayBuffer(item));

Is this the right condition?

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:56
> +v8::Handle<v8::Value> V8Blob::constructorCallback(const v8::Arguments& args)

Same issues here.

[WebKit Review Bot]

Comment on attachment 139283 [details]
Patch

Attachment 139283 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12555550

[Sam Weinig]

Re-landed with suggested changes and hopeful buildfixes in http://trac.webkit.org/changeset/115582.

[Sam Weinig]

(In reply to comment #45)
> There are a couple of WebIDL issues here, still.
> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:80
> > +    if (exec->argumentCount() > 1) {
> 
> This should happen after converting the array elements.
> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:83
> > +        // FIXME: Should we throw if the blobPropertyBag is not an object?
> 
> Yes; a TypeError: <http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary>.
> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:98
> > +            bool containsEndings = dictionary.get("endings", endings);
> 
> These properties should be got in lexicographic order; first endings and then type.
> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:104
> > +                    setDOMException(exec, INVALID_STATE_ERR);
> 
> Per <https://www.w3.org/Bugs/Public/show_bug.cgi?id=16729> (mentioned in the ChangeLog) and <http://dev.w3.org/2006/webapi/WebIDL/#es-enumeration>, this should be a TypeError.
> 
> > Source/WebCore/bindings/js/JSBlobCustom.cpp:123
> > +            blobBuilder->append(toArrayBuffer(item));
> 
> Is this the right condition?
> 
> > Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:56
> > +v8::Handle<v8::Value> V8Blob::constructorCallback(const v8::Arguments& args)
> 
> Same issues here.

I'll tackle these in a follow up patch.  Thanks for the review!!  (This is one reason I would love to see test cases go hand in hand with spec writing).

[Sam Weinig]

> > Source/WebCore/bindings/js/JSBlobCustom.cpp:123
> > +            blobBuilder->append(toArrayBuffer(item));
> 
> Is this the right condition?

I am not sure what you are asking here.  Do you think we should use ArrayBufferView?

[Sam Weinig]

Reopening to address a bit more feedback.

[Sam Weinig]

Created attachment 139379 [details]
Patch addressing additional feedback.

[Kentaro Hara]

Comment on attachment 139379 [details]
Patch addressing additional feedback.

r+ from my side.

[Kentaro Hara]

Ms2ger: Does it look good?

[Csaba Osztrogonác]

FYI: I updated the Qt expected files after this patch - http://trac.webkit.org/changeset/115592 and fixed a typo in fast/files/workers/inline-worker-via-blob-url.html: http://trac.webkit.org/changeset/115592/trunk/LayoutTests/fast/files/workers/inline-worker-via-blob-url.html

[Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #48)
> (This is one reason I would love to see test cases go hand in hand with
> spec writing).

Fair point; these edge cases aren't too important, though.

(In reply to comment #49)
> > > Source/WebCore/bindings/js/JSBlobCustom.cpp:123
> > > +            blobBuilder->append(toArrayBuffer(item));
> > 
> > Is this the right condition?
> 
> I am not sure what you are asking here.  Do you think we should use ArrayBufferView?

Sorry, I thought more context would be quoted; I was wondering about #if ENABLE(BLOB), where I would have expected a check related to Typed Arrays.


The followup patch looks good to me.

[Sam Weinig]

(In reply to comment #55)
> (In reply to comment #48)
> > (This is one reason I would love to see test cases go hand in hand with
> > spec writing).
> 
> Fair point; these edge cases aren't too important, though.
> 

Well...

> (In reply to comment #49)
> > > > Source/WebCore/bindings/js/JSBlobCustom.cpp:123
> > > > +            blobBuilder->append(toArrayBuffer(item));
> > > 
> > > Is this the right condition?
> > 
> > I am not sure what you are asking here.  Do you think we should use ArrayBufferView?
> 
> Sorry, I thought more context would be quoted; I was wondering about #if ENABLE(BLOB), where I would have expected a check related to Typed Arrays.
> 
> 
> The followup patch looks good to me.

Yeah, the #ifdef is correct, just odd.

[WebKit Review Bot]

Comment on attachment 139379 [details]
Patch addressing additional feedback.

Clearing flags on attachment: 139379

Committed r115599: <http://trac.webkit.org/changeset/115599>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.