Bug 156511

Won't be able to pass 100% of all IndexedDB tests until this is implemented.

I just modernized File.idl and am now building. Wish me luck.

New title: Implement latest File object spec (including its constructor)

Created attachment 277189 [details] Patch v1 Currently running layout tests locally, but also relying on EWS.

Comment on attachment 277189 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=277189&action=review Looks pretty good. Build seems to be failing, though. > Source/WebCore/bindings/js/JSFileCustom.cpp:45 > + DOMConstructorObject* jsConstructor = jsCast<DOMConstructorObject*>(exec->callee()); I suggest auto* instead of naming the type twice. I suggest the name "constructor" rather than the name "jsConstructor". > Source/WebCore/bindings/js/JSFileCustom.cpp:48 > + return throwVMError(exec, createReferenceError(exec, "File constructor associated document is unavailable")); Strange that there is not a helper to allow you to use only one function call here. That’s definitely what we want. Same thought on all the other uses of throwVMError. > Source/WebCore/bindings/js/JSFileCustom.cpp:51 > + if (exec->argumentCount() < 2) > + return throwVMError(exec, createTypeError(exec, "File constructor requires at least 2 arguments")); As I understand it, in modern bindings it is better to check for undefined rather than actually looking at argument count. We never want "undefined" to behave differently from omitting an argument entirely. Is there some way to write this without a literal argumentCount check the still gives the results we are looking for? > Source/WebCore/bindings/js/JSFileCustom.cpp:57 > + ASSERT(blobParts); Would be an improvement to put the result into a reference rather than a pointer since it can’t be null, although I suppose that seems wrong in the case where there is an exception. The assertion annoys me since toJSSequence guarantees this, but somehow it can’t convey that guarantee to us clearly enough, so we feel the need to assert. In fact, it would be more efficient if toJSSequence guaranteed it would return nullptr when there’s an exception, since checking that is cheaper than checking exec->hadException(). > Source/WebCore/bindings/js/JSFileCustom.cpp:61 > + JSValue jsFilename = exec->argument(1); > + String filename = jsFilename.toWTFString(exec); > + filename.replace('/', ':'); - Since we checked argumentCount, we can use uncheckedArgument(1). But I’d prefer that we not check argumentCount! - We need a check of hadException here, because converting an object to a string can involve calling a custom valueOf function which may raise an exception. - Do we want to convert undefined to "undefined", or would it be better to raise an exception in that case? See my comment above about treating undefined consistently with omitted arguments. If we do keep the code exactly like this, here’s how I would write it: String filename = exec->argument(1).toWTFString(exec).replace('/', ':'); if (exec->hadException()) return <...>; I don’t think it’s better spread out over three lines of code with the second local variable. I particularly don’t like the "js" prefix. Or we could use toStringOrNull so we can make the exception check more efficient. In that case we would have to do the value call separately. Or we could make a new version of toWTFString that returns String() to make the exception check more efficient. Sorry, not trying to make this excessively complex, just trying to get it right. > Source/WebCore/bindings/js/JSFileCustom.cpp:66 > + if (exec->argumentCount() > 2) { This check isn’t needed, because the handling of "undefined" for argument(2) already does the same thing. So this should be removed. > Source/WebCore/bindings/js/JSFileCustom.cpp:67 > + do { I think the use of "do while (0)" to make a "goto without goto" is not great. Plain old nested if statements would probably be better or perhaps a helper function would be good. > Source/WebCore/bindings/js/JSFileCustom.cpp:77 > + // Given the above test, this will always yield an object. > + JSObject* filePropertyBagObject = filePropertyBagValue.toObject(exec); toObject(exec) is not needed when you already know something is an object. Correct idiom in that case is: asObject(filePropertyBagValue) Or you could use getObject() to both check if something is an object and get it. I think I would write this: JSObject* filePropertyBagObject = filePropertyBagValue.getObject(); if (!filePropertyBagObject) return throwTypeError(*exec, "Third argument of the constructor is not an object"); > Source/WebCore/bindings/js/JSFileCustom.cpp:86 > + // Create the dictionary wrapper from the initializer object. > + JSDictionary dictionary(exec, filePropertyBagObject); > + > + // Attempt to get the type property. > + String type; > + dictionary.get("type", type); > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); Not the greatest way to get a property in the bindings code. We don’t need a JSDictionary just to get a property. But I guess it’s OK for now. > Source/WebCore/bindings/js/JSFileCustom.cpp:89 > + // If contenttype was invalid, abort the substeps of parsing the FilePropertyBag What is "contenttype"? Also we normally use a period for something like this. > Source/WebCore/bindings/js/JSFileCustom.cpp:111 > + else if (RefPtr<ArrayBufferView> arrayBufferView = toArrayBufferView(item)) Should use auto here instead of writing out RefPtr. > Source/WebCore/bindings/js/JSFileCustom.cpp:112 > + blobBuilder.append(arrayBufferView.release()); This should be WTFMove(), not release(). > Source/WebCore/bindings/js/JSFileCustom.cpp:116 > + String string = item.toString(exec)->value(exec); This is toWTFString, but written out. > Source/WebCore/bindings/js/JSFileCustom.cpp:123 > + RefPtr<File> file = File::create(blobBuilder.finalize(), filename, normalizedType, lastModified.value()); Normally better to use auto instead of writing out RefPtr here. > Source/WebCore/fileapi/File.cpp:72 > + m_size = -1; Why is this being set with assignment instead of construction syntax? What’s with the magic number -1? > Source/WebCore/fileapi/File.cpp:82 > + // FIXME: This does sync-i/o on the main thread and also recalculates every time the method is called. > + // The i/o should be performed on a background thread, > + // and the result should be cached along with an asynchronous monitor for changes to the file. The idea that this should monitor changes to the file is possibly peculiar since it sets up even more race conditions between the file system status and the caches here. Is that really right for the intent for this class?

Comment on attachment 277189 [details] Patch v1 Attachment 277189 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1213470 New failing tests: http/tests/local/fileapi/file-last-modified-after-delete.html http/tests/local/fileapi/file-last-modified.html imported/blink/storage/indexeddb/blob-basics-metadata.html

Created attachment 277191 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 277189 [details] Patch v1 Attachment 277189 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1213485 New failing tests: http/tests/local/fileapi/file-last-modified-after-delete.html http/tests/local/fileapi/file-last-modified.html imported/blink/storage/indexeddb/blob-basics-metadata.html

Created attachment 277192 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

The build breakage is either GCC's inability to deal with this template expansion, or a legitimate issue with "long int" not resolving to one of the signed int types we already have specializations for.

Most of your comments I simply addressed the way you suggested or a similar way. (In reply to comment #5) > Comment on attachment 277189 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277189&action=review > > Looks pretty good. Build seems to be failing, though. > > > Source/WebCore/bindings/js/JSFileCustom.cpp:45 > > + DOMConstructorObject* jsConstructor = jsCast<DOMConstructorObject*>(exec->callee()); > > I suggest auto* instead of naming the type twice. I suggest the name > "constructor" rather than the name "jsConstructor". > > > Source/WebCore/bindings/js/JSFileCustom.cpp:48 > > + return throwVMError(exec, createReferenceError(exec, "File constructor associated document is unavailable")); > > Strange that there is not a helper to allow you to use only one function > call here. That’s definitely what we want. Same thought on all the other > uses of throwVMError. > > > Source/WebCore/bindings/js/JSFileCustom.cpp:51 > > + if (exec->argumentCount() < 2) > > + return throwVMError(exec, createTypeError(exec, "File constructor requires at least 2 arguments")); > > As I understand it, in modern bindings it is better to check for undefined > rather than actually looking at argument count. We never want "undefined" to > behave differently from omitting an argument entirely. Is there some way to > write this without a literal argumentCount check the still gives the results > we are looking for? > > > Source/WebCore/bindings/js/JSFileCustom.cpp:57 > > + ASSERT(blobParts); > > Would be an improvement to put the result into a reference rather than a > pointer since it can’t be null, although I suppose that seems wrong in the > case where there is an exception. > > The assertion annoys me since toJSSequence guarantees this, but somehow it > can’t convey that guarantee to us clearly enough, so we feel the need to > assert. In fact, it would be more efficient if toJSSequence guaranteed it > would return nullptr when there’s an exception, since checking that is > cheaper than checking exec->hadException(). > > > Source/WebCore/bindings/js/JSFileCustom.cpp:61 > > + JSValue jsFilename = exec->argument(1); > > + String filename = jsFilename.toWTFString(exec); > > + filename.replace('/', ':'); > > - Since we checked argumentCount, we can use uncheckedArgument(1). But I’d > prefer that we not check argumentCount! > - We need a check of hadException here, because converting an object to a > string can involve calling a custom valueOf function which may raise an > exception. > - Do we want to convert undefined to "undefined", or would it be better to > raise an exception in that case? See my comment above about treating > undefined consistently with omitted arguments. > > If we do keep the code exactly like this, here’s how I would write it: > > String filename = exec->argument(1).toWTFString(exec).replace('/', ':'); > if (exec->hadException()) > return <...>; > > I don’t think it’s better spread out over three lines of code with the > second local variable. I particularly don’t like the "js" prefix. > > Or we could use toStringOrNull so we can make the exception check more > efficient. In that case we would have to do the value call separately. Or we > could make a new version of toWTFString that returns String() to make the > exception check more efficient. > > Sorry, not trying to make this excessively complex, just trying to get it > right. > > > Source/WebCore/bindings/js/JSFileCustom.cpp:66 > > + if (exec->argumentCount() > 2) { > > This check isn’t needed, because the handling of "undefined" for argument(2) > already does the same thing. So this should be removed. > > > Source/WebCore/bindings/js/JSFileCustom.cpp:67 > > + do { > > I think the use of "do while (0)" to make a "goto without goto" is not > great. Plain old nested if statements would probably be better or perhaps a > helper function would be good. Originally there were a lot more breaks. But in the uploaded patch there were only two, and after other suggested changes there's only one. So it's a no brainer to do one nested if. > > Source/WebCore/bindings/js/JSFileCustom.cpp:89 > > + // If contenttype was invalid, abort the substeps of parsing the FilePropertyBag > > What is "contenttype"? Also we normally use a period for something like this. The previous name for "type" Fixed. > > > Source/WebCore/bindings/js/JSFileCustom.cpp:111 > > + else if (RefPtr<ArrayBufferView> arrayBufferView = toArrayBufferView(item)) > > Should use auto here instead of writing out RefPtr. toArrayBufferView returns a PassRefPtr (still!), so I did leave it specified as a RefPtr. But... > > > Source/WebCore/bindings/js/JSFileCustom.cpp:112 > > + blobBuilder.append(arrayBufferView.release()); > > This should be WTFMove(), not release(). I did that! > > Source/WebCore/fileapi/File.cpp:72 > > + m_size = -1; > > Why is this being set with assignment instead of construction syntax? What’s > with the magic number -1? Needed in an earlier iteration of the patch (since m_size belongs to Blob and not File), but no longer needed. It tells Blob to lazily fetch the size from the BlobRegistry. Magic numbers are bad, but this one is fairly established. :( > > Source/WebCore/fileapi/File.cpp:82 > > + // FIXME: This does sync-i/o on the main thread and also recalculates every time the method is called. > > + // The i/o should be performed on a background thread, > > + // and the result should be cached along with an asynchronous monitor for changes to the file. > > The idea that this should monitor changes to the file is possibly peculiar > since it sets up even more race conditions between the file system status > and the caches here. Is that really right for the intent for this class? I think this is right. When a File object is created from an <input type="file"> picker, it points to an underlying file on disk, and the properties you get via javascript are meant to reflect changes to the underlying file that have occurred since the user picked it. I think setting the object up to know "The file has changed, so the next time somebody asks you have to recalculate the size" is much better than "always calculate the size every time somebody asks"

Created attachment 277216 [details] Patch v2

Comment on attachment 277216 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=277216&action=review > Source/WebCore/bindings/js/JSFileCustom.cpp:72 > + if (!arg.isUndefinedOrNull()) { I understand we should skip this if it’s undefined. But do we really want this treatment for null? Why is null different from false in this respect? Do we have this covered by a test? > Source/WebCore/bindings/js/JSFileCustom.cpp:73 > + // Given the above test, this will always yield an object. This comment is not correct! For example, the argument could be a boolean or a number, and that’s not an object.

Comment on attachment 277216 [details] Patch v2 Attachment 277216 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1216068 New failing tests: fast/files/file-constructor.html

Created attachment 277218 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 277216 [details] Patch v2 Attachment 277216 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1216082 New failing tests: fast/files/file-constructor.html

Created attachment 277220 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 277216 [details] Patch v2 Attachment 277216 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1216083 New failing tests: fast/files/file-constructor.html

Created attachment 277221 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4

Comment on attachment 277216 [details] Patch v2 Attachment 277216 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1216088 New failing tests: fast/files/file-constructor.html

Created attachment 277222 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

(In reply to comment #13) > Comment on attachment 277216 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277216&action=review > > > Source/WebCore/bindings/js/JSFileCustom.cpp:72 > > + if (!arg.isUndefinedOrNull()) { > > I understand we should skip this if it’s undefined. But do we really want > this treatment for null? Why is null different from false in this respect? > Do we have this covered by a test? The IDL specifies the first argument is a non-optional sequence. In IDL parlance, null does not qualify. Yes, it's covered by a test: shouldThrow("new File(null, 'world.html')", '"TypeError: Value is not a sequence"'); ...And I didn't properly update that test after making the code changes! *sigh* doh! > > > Source/WebCore/bindings/js/JSFileCustom.cpp:73 > > + // Given the above test, this will always yield an object. > > This comment is not correct! For example, the argument could be a boolean or > a number, and that’s not an object. Forgot to remove the comment when removing the isObject test. Aren't comments fun?!?!

Created attachment 277256 [details] Patch for landing

Comment on attachment 277216 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=277216&action=review >>> Source/WebCore/bindings/js/JSFileCustom.cpp:72 >>> + if (!arg.isUndefinedOrNull()) { >> >> I understand we should skip this if it’s undefined. But do we really want this treatment for null? Why is null different from false in this respect? Do we have this covered by a test? > > The IDL specifies the first argument is a non-optional sequence. In IDL parlance, null does not qualify. > > Yes, it's covered by a test: > shouldThrow("new File(null, 'world.html')", '"TypeError: Value is not a sequence"'); > > ...And I didn't properly update that test after making the code changes! *sigh* doh! Uh, but the code here is about the 3rd parameter which *is* optional and is a Dictionary. I don't believe allowing null is correct for the 3rd parameter.

Comment on attachment 277256 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=277256&action=review > Source/WebCore/bindings/js/JSFileCustom.cpp:72 > + if (!arg.isUndefinedOrNull()) { This does not seem right: See http://heycam.github.io/webidl/#es-overloads (10.4). undefined is OK for optional parameters, null is not.

Comment on attachment 277256 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=277256&action=review >> Source/WebCore/bindings/js/JSFileCustom.cpp:72 >> + if (!arg.isUndefinedOrNull()) { > > This does not seem right: > See http://heycam.github.io/webidl/#es-overloads (10.4). undefined is OK for optional parameters, null is not. Actually never mind my comment, null is OK for a Dictionary parameter as per: http://heycam.github.io/webidl/#es-dictionary

(In reply to comment #24) > Comment on attachment 277216 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277216&action=review > > >>> Source/WebCore/bindings/js/JSFileCustom.cpp:72 > >>> + if (!arg.isUndefinedOrNull()) { > >> > >> I understand we should skip this if it’s undefined. But do we really want this treatment for null? Why is null different from false in this respect? Do we have this covered by a test? > > > > The IDL specifies the first argument is a non-optional sequence. In IDL parlance, null does not qualify. > > > > Yes, it's covered by a test: > > shouldThrow("new File(null, 'world.html')", '"TypeError: Value is not a sequence"'); > > > > ...And I didn't properly update that test after making the code changes! *sigh* doh! > > Uh, but the code here is about the 3rd parameter which *is* optional and is > a Dictionary. I don't believe allowing null is correct for the 3rd parameter. Whoops. Indeed. I was looking at line 51: if (arg.isUndefinedOrNull()) Not line 72: if (!arg.isUndefinedOrNull()) { There's no doubt that passing null there is not okay. Updating.

(In reply to comment #26) > Comment on attachment 277256 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277256&action=review > > >> Source/WebCore/bindings/js/JSFileCustom.cpp:72 > >> + if (!arg.isUndefinedOrNull()) { > > > > This does not seem right: > > See http://heycam.github.io/webidl/#es-overloads (10.4). undefined is OK for optional parameters, null is not. > > Actually never mind my comment, null is OK for a Dictionary parameter as per: > http://heycam.github.io/webidl/#es-dictionary Oh! Well nevermind then!!!