Bug 84555 - Add support for the Blob constructor
: Add support for the Blob constructor
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Sam Weinig
http://dev.w3.org/2006/webapi/FileAPI...
:
Depends on:
Blocks: 84036
  Show dependency treegraph
 
Reported: 2012-04-22 16:55 PDT by Sam Weinig
Modified: 2012-04-29 13:08 PDT (History)
14 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2012-04-22 16:55:53 PDT
The File API spec has been updated to include a constructor for Blob (http://dev.w3.org/2006/webapi/FileAPI/#constructorBlob).
Comment 1 Sam Weinig 2012-04-22 17:02:37 PDT
Created attachment 138279 [details]
Patch
Comment 2 Sam Weinig 2012-04-22 17:03:46 PDT
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 WebKit Review Bot 2012-04-22 17:04:35 PDT
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 Gyuyoung Kim 2012-04-22 17:22:23 PDT
Comment on attachment 138279 [details]
Patch

Attachment 138279 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12469716
Comment 5 Kinuko Yasuda 2012-04-23 01:24:13 PDT
Created attachment 138307 [details]
Filled in the V8 code

Added constructor code for V8 based on Sam's patch.
Comment 6 Kentaro Hara 2012-04-23 01:55:00 PDT
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).
Comment 7 Kinuko Yasuda 2012-04-23 04:24:54 PDT
Created attachment 138324 [details]
new patch with v8 code
Comment 8 Kinuko Yasuda 2012-04-23 04:54:29 PDT
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 Gyuyoung Kim 2012-04-23 05:39:42 PDT
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
Comment 10 Kinuko Yasuda 2012-04-23 06:23:51 PDT
Created attachment 138331 [details]
new patch with v8 code

efl build fix.
Comment 11 Kentaro Hara 2012-04-23 08:51:21 PDT
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.
Comment 12 Kentaro Hara 2012-04-23 08:55:38 PDT
(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 Sam Weinig 2012-04-23 14:08:11 PDT
At least my part, was not ready to be reviewed ( I am pretty sure I didn't mark it r?).
Comment 14 Sam Weinig 2012-04-23 14:12:23 PDT
(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?
Comment 15 Sam Weinig 2012-04-23 14:13:12 PDT
In addition to no having completed writing my patch, I had not yet added the test case.  Please let me finish :D.
Comment 16 Kentaro Hara 2012-04-23 14:23:01 PDT
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.
Comment 17 David Levin 2012-04-23 14:46:47 PDT
Comment on attachment 138331 [details]
new patch with v8 code

r- if nothing else because Sam hasn't finished :)
Comment 18 WebKit Review Bot 2012-04-23 15:54:40 PDT
Comment on attachment 138279 [details]
Patch

Attachment 138279 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12517048
Comment 19 Kinuko Yasuda 2012-04-23 19:38:59 PDT
(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 21 Sam Weinig 2012-04-23 21:41:19 PDT
(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.
Comment 22 Kentaro Hara 2012-04-24 05:04:30 PDT
(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 Sam Weinig 2012-04-24 15:15:08 PDT
(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 Sam Weinig 2012-04-26 19:56:14 PDT
Created attachment 139124 [details]
Patch
Comment 25 WebKit Review Bot 2012-04-26 19:58:00 PDT
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 Sam Weinig 2012-04-26 21:56:49 PDT
Ok, I think this is ready for review now.  I am happy with my test case.
Comment 27 Gyuyoung Kim 2012-04-26 22:35:15 PDT
Comment on attachment 139124 [details]
Patch

Attachment 139124 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12558104
Comment 28 Darin Adler 2012-04-27 10:22:14 PDT
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 Kinuko Yasuda 2012-04-27 10:39:52 PDT
(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 Sam Weinig 2012-04-27 10:43:44 PDT
Created attachment 139226 [details]
Patch
Comment 31 WebKit Review Bot 2012-04-27 10:46:59 PDT
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 Sam Weinig 2012-04-27 11:04:17 PDT
(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 Maciej Stachowiak 2012-04-27 13:16:01 PDT
Comment on attachment 139226 [details]
Patch

r=me
Comment 34 Sam Weinig 2012-04-27 13:23:02 PDT
Committed r115484: <http://trac.webkit.org/changeset/115484>
Comment 35 Kentaro Hara 2012-04-27 13:24:12 PDT
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())
Comment 36 Dimitri Glazkov (Google) 2012-04-27 13:51:12 PDT
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 Dimitri Glazkov (Google) 2012-04-27 14:06:51 PDT
(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 Sam Weinig 2012-04-27 14:18:31 PDT
(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.
Comment 39 Kentaro Hara 2012-04-27 14:25:47 PDT
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.
Comment 40 Kentaro Hara 2012-04-27 14:31:40 PDT
(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 Sam Weinig 2012-04-27 14:43:08 PDT
(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 Sam Weinig 2012-04-27 14:58:14 PDT
Created attachment 139283 [details]
Patch
Comment 43 Kentaro Hara 2012-04-27 15:00:19 PDT
Sam: Did you update blob-constructor-expected.txt?
Comment 44 WebKit Review Bot 2012-04-27 15:01:29 PDT
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 Ms2ger 2012-04-28 00:46:54 PDT
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 WebKit Review Bot 2012-04-28 01:33:43 PDT
Comment on attachment 139283 [details]
Patch

Attachment 139283 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12555550
Comment 47 Sam Weinig 2012-04-28 15:25:02 PDT
Re-landed with suggested changes and hopeful buildfixes in http://trac.webkit.org/changeset/115582.
Comment 48 Sam Weinig 2012-04-28 15:26:13 PDT
(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 Sam Weinig 2012-04-28 16:14:36 PDT
> > 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 Sam Weinig 2012-04-28 16:21:03 PDT
Reopening to address a bit more feedback.
Comment 51 Sam Weinig 2012-04-28 17:21:06 PDT
Created attachment 139379 [details]
Patch addressing additional feedback.
Comment 52 Kentaro Hara 2012-04-28 17:34:22 PDT
Comment on attachment 139379 [details]
Patch addressing additional feedback.

r+ from my side.
Comment 53 Kentaro Hara 2012-04-28 20:27:08 PDT
Ms2ger: Does it look good?
Comment 54 Csaba Osztrogonác 2012-04-28 23:07:19 PDT
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 Ms2ger 2012-04-29 03:55:58 PDT
(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 Sam Weinig 2012-04-29 12:31:37 PDT
(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 WebKit Review Bot 2012-04-29 13:07:55 PDT
Comment on attachment 139379 [details]
Patch addressing additional feedback.

Clearing flags on attachment: 139379

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