According to the WebIDL in the File API, the Blob constructor should accept a sequence for the parts argument. http://www.w3.org/TR/FileAPI/#dfn-Blob In WebKit, the constructor only accepts arrays, as can be shown by the test case in the bug's URL. Chromium bug (fixed, contains LayoutTests that might be useful): https://code.google.com/p/chromium/issues/detail?id=314755 Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=937348
Created attachment 216654 [details] Patch
Comment on attachment 216654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216654&action=review > Source/WebCore/bindings/js/JSBlobCustom.cpp:81 > + } else if (firstArg.isObject()) { The Web IDL specification says "Any kind of object except for a native Date object or a native RegExp object" http://heycam.github.io/webidl/#es-sequence You currently accept Date / RegExp objects which is against the spec and also does not match Blink's behavior. Also, It might be a good idea to move this to a separate utility function since there are other places that need to handle sequences. > LayoutTests/fast/files/blob-constructor-expected.txt:13 > +FAIL new Blob('hello') should throw TypeError: First argument of the constructor is not of type Array. Threw exception TypeError: First argument is not a sequence. The test needs updating. Leaving them as FAIL is not a good idea.
Comment on attachment 216654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216654&action=review >> LayoutTests/fast/files/blob-constructor-expected.txt:13 >> +FAIL new Blob('hello') should throw TypeError: First argument of the constructor is not of type Array. Threw exception TypeError: First argument is not a sequence. > > The test needs updating. Leaving them as FAIL is not a good idea. Also, I am not sure we want to expose the term "sequence" in the exception message since this is a Web IDL term and JavaScript developers won't necessarily know what it is.
JSCryptoOperationData.cpp also handles sequences, would be nice to see if some code can be shared.
Actually, that uses code that's already in a shared location, toJSSequence().
Thank you for the fast and thorough feedback! toJSSequence() throws a TypeError with no description. I copied its code and tweaked it to do what I wanted. The two solutions I thought about are: 1) Adding a toJSSequence overload that takes an extra String containing the error description, and changing the current version to call it with the string TypeError. 2) Creating a separate toJSSequenceWithCustomErrorMessage (I'm terrible at names, sorry) with the tweaks that I added there. Which way would you like me to go? I inherited the Date / RegExp issue from the original implementation. I know the V8 bindings in Blink explicitly test, and I thought there's some reason why this isn't happening in JavaScriptCore / WebKit. Should I add the checks to toJSSequence?
(In reply to comment #6) > Thank you for the fast and thorough feedback! > > > toJSSequence() throws a TypeError with no description. I copied its code and tweaked it to do what I wanted. > > The two solutions I thought about are: > 1) Adding a toJSSequence overload that takes an extra String containing the error description, and changing the current version to call it with the string TypeError. > > 2) Creating a separate toJSSequenceWithCustomErrorMessage (I'm terrible at names, sorry) with the tweaks that I added there. Technically, we don't strictly need this custom error message. Throwing a TypeError is enough, but I'll let Alexey decide. > Which way would you like me to go? > > I inherited the Date / RegExp issue from the original implementation. I know the V8 bindings in Blink explicitly test, and I thought there's some reason why this isn't happening in JavaScriptCore / WebKit. Should I add the checks to toJSSequence? This is not really related to this patch then and can / should be addressed separately.
Providing extra debugging information to web developers is certainly useful. But "First argument is not a sequence" is IMO not materially better than "Value is not a sequence" that could be added to toJSSequence() without any extra arguments.
Created attachment 216729 [details] Patch
Created attachment 216730 [details] Patch
I removed the custom code in favor of calling toJSSequence and implemented Alexey's suggested error message. I also updated test expectations to match the new error message. Please take another look?
Comment on attachment 216730 [details] Patch LGTM, r=me, assuming Alexey is also fine with this.
Thank you, Chris! Alexey, I'll wait for you to flip the commit-queue flag, or to give me more feedback.
Comment on attachment 216730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216730&action=review > Source/WebCore/bindings/js/JSBlobCustom.cpp:116 > + JSValue item = blobParts->get(exec, i); Can't get() throw an exception? > Source/WebCore/bindings/js/JSBlobCustom.cpp:119 > if (item.inherits(JSArrayBuffer::info())) > blobBuilder.append(toArrayBuffer(item)); This is not the best way to write this - toArrayBuffer will check the type again. I'd write if (ArrayBuffer* arrayBuffer = toArrayBuffer(item)) blobBuilder.append(arrayBuffer); Same inefficiency is repeated below again.
Comment on attachment 216730 [details] Patch Clearing flags on attachment: 216730 Committed r159275: <http://trac.webkit.org/changeset/159275>
All reviewed patches have been landed. Closing bug.
Alexey, I'm preparing a new patch with the feedback you gave me. While doing that, I ran into something odd. toArrayBuffer is defined in Source/JavaScriptCore/JSArrayBuffer.h as inline ArrayBuffer* toArrayBuffer(JSValue value) toArrayBufferView is defined in Source/WebCore/bindings/js/JSDomBinding.h as inline PassRefPtr<JSC::ArrayBufferView> toArrayBufferView(JSC::JSValue value) Otherwise, the method bodies are really similar. I think this is weird because, in the Blob constructor, I'd have to use RefPtr<ArrayBufferView> on one branch, and a bare ArrayBuffer* on the other branch, to follow convention. What do you advise? Should I do this, or change one of the defintions? If I go down that route, I'd also move toArrayBufferView in JavaScriptCore/JSArrayBufferView.h.
It seems worthwhile to look into whether toArrayBufferView can return a raw pointer. But my guess is that it's not possible - an ArrayBufferView only exists when referenced. JSArrayBufferView is not a wrapper, it's a proxy that creates an ArrayBufferView on demand. This is based on a very brief look at the code, so it may be incorrect.
Thank you very much for your guidance, Alexey! I implemented your suggestions and submitted a new bug+patch. https://bugs.webkit.org/show_bug.cgi?id=124343 I'll look into the ArrayBufferView issue sometime soon. I'm completely new to the JavaScriptCore codebase, so it will take a while.