The File API spec has been updated to include a constructor for Blob (http://dev.w3.org/2006/webapi/FileAPI/#constructorBlob).
Created attachment 138279 [details] Patch
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.
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 on attachment 138279 [details] Patch Attachment 138279 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12469716
Created attachment 138307 [details] Filled in the V8 code Added constructor code for V8 based on Sam's patch.
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).
Created attachment 138324 [details] new patch with v8 code
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 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
Created attachment 138331 [details] new patch with v8 code efl build fix.
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.
(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.
At least my part, was not ready to be reviewed ( I am pretty sure I didn't mark it r?).
(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?
In addition to no having completed writing my patch, I had not yet added the test case. Please let me finish :D.
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 on attachment 138331 [details] new patch with v8 code r- if nothing else because Sam hasn't finished :)
Comment on attachment 138279 [details] Patch Attachment 138279 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12517048
(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!
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
(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.
(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().
(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.
Created attachment 139124 [details] Patch
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.
Ok, I think this is ready for review now. I am happy with my test case.
Comment on attachment 139124 [details] Patch Attachment 139124 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12558104
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&)
(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)
Created attachment 139226 [details] Patch
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.
(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 on attachment 139226 [details] Patch r=me
Committed r115484: <http://trac.webkit.org/changeset/115484>
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())
This broke Chromium: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/23046/steps/compile/logs/stdio Can haz rollout?
(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/
(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 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.
(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 () {});
(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.
Created attachment 139283 [details] Patch
Sam: Did you update blob-constructor-expected.txt?
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.
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 on attachment 139283 [details] Patch Attachment 139283 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12555550
Re-landed with suggested changes and hopeful buildfixes in http://trac.webkit.org/changeset/115582.
(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).
> > 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?
Reopening to address a bit more feedback.
Created attachment 139379 [details] Patch addressing additional feedback.
Comment on attachment 139379 [details] Patch addressing additional feedback. r+ from my side.
Ms2ger: Does it look good?
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
(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.
(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 on attachment 139379 [details] Patch addressing additional feedback. Clearing flags on attachment: 139379 Committed r115599: <http://trac.webkit.org/changeset/115599>
All reviewed patches have been landed. Closing bug.