Bug 84555 - Add support for the Blob constructor
: Add support for the Blob constructor
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://dev.w3.org/2006/webapi/FileAPI...
:
:
: 84036
  Show dependency treegraph
 
Reported: 2012-04-22 16:55 PST by
Modified: 2012-04-29 13:08 PST (History)


Attachments
Patch (43.39 KB, patch)
2012-04-22 17:02 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
Filled in the V8 code (47.77 KB, patch)
2012-04-23 01:24 PST, Kinuko Yasuda
haraken: review-
Review Patch | Details | Formatted Diff | Diff
new patch with v8 code (47.07 KB, patch)
2012-04-23 04:24 PST, Kinuko Yasuda
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
new patch with v8 code (47.14 KB, patch)
2012-04-23 06:23 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (51.58 KB, patch)
2012-04-26 19:56 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
Patch (51.62 KB, patch)
2012-04-27 10:43 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
Patch (49.58 KB, patch)
2012-04-27 14:58 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
Patch addressing additional feedback. (14.06 KB, patch)
2012-04-28 17:21 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-22 16:55:53 PST
The File API spec has been updated to include a constructor for Blob (http://dev.w3.org/2006/webapi/FileAPI/#constructorBlob).
------- Comment #1 From 2012-04-22 17:02:37 PST -------
Created an attachment (id=138279) [details]
Patch
------- Comment #2 From 2012-04-22 17:03:46 PST -------
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.
------- Comment #3 From 2012-04-22 17:04:35 PST -------
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.
------- Comment #4 From 2012-04-22 17:22:23 PST -------
(From update of attachment 138279 [details])
Attachment 138279 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12469716
------- Comment #5 From 2012-04-23 01:24:13 PST -------
Created an attachment (id=138307) [details]
Filled in the V8 code

Added constructor code for V8 based on Sam's patch.
------- Comment #6 From 2012-04-23 01:55:00 PST -------
(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.

> 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).
------- Comment #7 From 2012-04-23 04:24:54 PST -------
Created an attachment (id=138324) [details]
new patch with v8 code
------- Comment #8 From 2012-04-23 04:54:29 PST -------
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.
------- Comment #9 From 2012-04-23 05:39:42 PST -------
(From update of attachment 138324 [details])
Attachment 138324 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12477759
------- Comment #10 From 2012-04-23 06:23:51 PST -------
Created an attachment (id=138331) [details]
new patch with v8 code

efl build fix.
------- Comment #11 From 2012-04-23 08:51:21 PST -------
(From update of attachment 138331 [details])
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.
------- Comment #12 From 2012-04-23 08:55:38 PST -------
(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.
------- Comment #13 From 2012-04-23 14:08:11 PST -------
At least my part, was not ready to be reviewed ( I am pretty sure I didn't mark it r?).
------- Comment #14 From 2012-04-23 14:12:23 PST -------
(In reply to comment #6)
> (From update of attachment 138307 [details] [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?
------- Comment #15 From 2012-04-23 14:13:12 PST -------
In addition to no having completed writing my patch, I had not yet added the test case.  Please let me finish :D.
------- Comment #16 From 2012-04-23 14:23:01 PST -------
(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.)

>>> 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.
------- Comment #17 From 2012-04-23 14:46:47 PST -------
(From update of attachment 138331 [details])
r- if nothing else because Sam hasn't finished :)
------- Comment #18 From 2012-04-23 15:54:40 PST -------
(From update of attachment 138279 [details])
Attachment 138279 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12517048
------- Comment #19 From 2012-04-23 19:38:59 PST -------
(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!
------- Comment #20 From 2012-04-23 21:19:52 PST -------
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
------- Comment #21 From 2012-04-23 21:41:19 PST -------
(In reply to comment #16)
> (From update of attachment 138307 [details] [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.
------- Comment #22 From 2012-04-24 05:04:30 PST -------
(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().
------- Comment #23 From 2012-04-24 15:15:08 PST -------
(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.
------- Comment #24 From 2012-04-26 19:56:14 PST -------
Created an attachment (id=139124) [details]
Patch
------- Comment #25 From 2012-04-26 19:58:00 PST -------
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.
------- Comment #26 From 2012-04-26 21:56:49 PST -------
Ok, I think this is ready for review now.  I am happy with my test case.
------- Comment #27 From 2012-04-26 22:35:15 PST -------
(From update of attachment 139124 [details])
Attachment 139124 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12558104
------- Comment #28 From 2012-04-27 10:22:14 PST -------
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&)
------- Comment #29 From 2012-04-27 10:39:52 PST -------
(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)
------- Comment #30 From 2012-04-27 10:43:44 PST -------
Created an attachment (id=139226) [details]
Patch
------- Comment #31 From 2012-04-27 10:46:59 PST -------
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.
------- Comment #32 From 2012-04-27 11:04:17 PST -------
(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.
------- Comment #33 From 2012-04-27 13:16:01 PST -------
(From update of attachment 139226 [details])
r=me
------- Comment #34 From 2012-04-27 13:23:02 PST -------
Committed r115484: <http://trac.webkit.org/changeset/115484>
------- Comment #35 From 2012-04-27 13:24:12 PST -------
(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.

> 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())
------- Comment #36 From 2012-04-27 13:51:12 PST -------
This broke Chromium: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/23046/steps/compile/logs/stdio

Can haz rollout?
------- Comment #37 From 2012-04-27 14:06:51 PST -------
(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/
------- Comment #38 From 2012-04-27 14:18:31 PST -------
(In reply to comment #35)
> (From update of attachment 139226 [details] [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.
------- Comment #39 From 2012-04-27 14:25:47 PST -------
(From update of attachment 139226 [details])
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.
------- Comment #40 From 2012-04-27 14:31:40 PST -------
(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 () {});
------- Comment #41 From 2012-04-27 14:43:08 PST -------
(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.
------- Comment #42 From 2012-04-27 14:58:14 PST -------
Created an attachment (id=139283) [details]
Patch
------- Comment #43 From 2012-04-27 15:00:19 PST -------
Sam: Did you update blob-constructor-expected.txt?
------- Comment #44 From 2012-04-27 15:01:29 PST -------
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.
------- Comment #45 From 2012-04-28 00:46:54 PST -------
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.
------- Comment #46 From 2012-04-28 01:33:43 PST -------
(From update of attachment 139283 [details])
Attachment 139283 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12555550
------- Comment #47 From 2012-04-28 15:25:02 PST -------
Re-landed with suggested changes and hopeful buildfixes in http://trac.webkit.org/changeset/115582.
------- Comment #48 From 2012-04-28 15:26:13 PST -------
(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).
------- Comment #49 From 2012-04-28 16:14:36 PST -------
> > 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?
------- Comment #50 From 2012-04-28 16:21:03 PST -------
Reopening to address a bit more feedback.
------- Comment #51 From 2012-04-28 17:21:06 PST -------
Created an attachment (id=139379) [details]
Patch
------- Comment #52 From 2012-04-28 17:34:22 PST -------
(From update of attachment 139379 [details])
r+ from my side.
------- Comment #53 From 2012-04-28 20:27:08 PST -------
Ms2ger: Does it look good?
------- Comment #54 From 2012-04-28 23:07:19 PST -------
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
------- Comment #55 From 2012-04-29 03:55:58 PST -------
(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.
------- Comment #56 From 2012-04-29 12:31:37 PST -------
(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.
------- Comment #57 From 2012-04-29 13:07:55 PST -------
(From update of attachment 139379 [details])
Clearing flags on attachment: 139379

Committed r115599: <http://trac.webkit.org/changeset/115599>
------- Comment #58 From 2012-04-29 13:08:04 PST -------
All reviewed patches have been landed.  Closing bug.