Implement File constructor.

Implement File constructor.

The feature is behind the FileConstructor RuntimeEnabledFeature with
status=experimental.

BUG=164933
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162116

[pwnall-personal, 2013-11-04 03:40:44]

Apologies for the large patch. Can you please take a look?

After I implemented File constructor, there are two types of File instances: the
ones built by the constructor, and the ones backed by a file on a real
filesystem. 

Some of the code that used Blob before was already hacky due to having to call
isFile(). This patch makes the problem worse by introducing an isOnFilesystem()
method. Code that wants to distinguish between filesystem files and constructed
File/Blob instances needs to call isOnFilesystem() instead of isFile(). I think
I already did this where it was appropriate.

Positive notes about this patch:
* all the changes are driven by tests in LayoutTests
* File constructor matches the Firefox spec
* both the Blob and File constructor accept File parts
* XHR can send constructed File instances (just like it can send Blobs)
* XHR can send FormData instances whose fields are constructed File instances;
the filename argument in FormData.append() correctly overrides the name in the
File instance

To summarize, I think most of the value in this patch is adding tests for all
the new interactions that are enabled by having a File constructor. I didn't put
too much work into designing the File changes, because I don't want to disrupt
others working on File too much, and I would like to discuss design changes
before implementing them.

[Inactive, 2013-11-04 14:13:10]

https://codereview.chromium.org/57483002/diff/110001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/110001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:54: RefPtr<Blob> blob =
Blob::create();
Why are we creating a blob in the error case?

https://codereview.chromium.org/57483002/diff/110001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:60: if (!firstArg->IsArray()) {
Actually, the spec says it is a sequence, not an array so we need to be more
permissive than this. You likely want to use toV8Sequence() instead to check
that it is a sequence (e.g. it can be an object than has a length property but
is not an array)

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/Blob...
File Source/core/fileapi/BlobBuilder.cpp (right):

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.cpp:152: return file;
nit: return file.release();

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/File...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/File...
Source/core/fileapi/File.cpp:134: , m_path()
Not needed

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/File...
File Source/core/fileapi/File.idl (right):

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/File...
Source/core/fileapi/File.idl:28: CustomConstructor,
There is no File constructor that takes no argument so this one should probably
be removed. This likely messes up the calculation of File.length.

[pwnall-personal, 2013-11-04 17:16:25]

Thank you for the very fast and really good feedback!

I addressed everything except for the sequence vs array issue. I "borrowed" the
custom File constructor code from Blob, so Blob also has this problem. I'm
inclined to file a bug to remind myself to fix this in a follow-up CL. What do
you think?

https://codereview.chromium.org/57483002/diff/110001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/110001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:54: RefPtr<Blob> blob =
Blob::create();
On 2013/11/04 14:13:11, Chris Dumez wrote:
> Why are we creating a blob in the error case?

Done. (Fixed.)

This was an unfortunate mix between a bad copy-paste and me just getting used to
V8 bindings code.

https://codereview.chromium.org/57483002/diff/110001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:60: if (!firstArg->IsArray()) {
On 2013/11/04 14:13:11, Chris Dumez wrote:
> Actually, the spec says it is a sequence, not an array so we need to be more
> permissive than this. You likely want to use toV8Sequence() instead to check
> that it is a sequence (e.g. it can be an object than has a length property but
> is not an array)

Good point!

I got this from V8BlobCustom, so the Blob constructor is busted as well. Should
I fix both in this CL, or should I file a follow-up bug?

Also, what's the best way of sharing some code between V8BlobCustom and
V8FileCustom?

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/Blob...
File Source/core/fileapi/BlobBuilder.cpp (right):

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.cpp:152: return file;
On 2013/11/04 14:13:11, Chris Dumez wrote:
> nit: return file.release();

Done.

Thank you!
To be honest, I replicated the pattern from getBlob(). I fixed that as well.

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/File...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/File...
Source/core/fileapi/File.cpp:134: , m_path()
On 2013/11/04 14:13:11, Chris Dumez wrote:
> Not needed

Done.

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/File...
File Source/core/fileapi/File.idl (right):

https://codereview.chromium.org/57483002/diff/110001/Source/core/fileapi/File...
Source/core/fileapi/File.idl:28: CustomConstructor,
On 2013/11/04 14:13:11, Chris Dumez wrote:
> There is no File constructor that takes no argument so this one should
probably
> be removed. This likely messes up the calculation of File.length.

Done.
I also tweaked the IDL for the Blob constructor.

[Inactive, 2013-11-04 18:33:52]

https://codereview.chromium.org/57483002/diff/200001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/200001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:58: if (!firstArg->IsArray()) {
Filing a bug and adding a FIXME comment here and in Blob to explain that we
should handle Web IDL sequences would be acceptable for now.

https://codereview.chromium.org/57483002/diff/200001/Source/core/fileapi/Blob...
File Source/core/fileapi/Blob.idl (left):

https://codereview.chromium.org/57483002/diff/200001/Source/core/fileapi/Blob...
Source/core/fileapi/Blob.idl:34: CustomConstructor,
No we actually need to keep this one. The File API spec does specify a
constructor with no argument for Blob:
http://dev.w3.org/2006/webapi/FileAPI/#dfn-Blob

We only needed to remove the one from File.

[pwnall-personal, 2013-11-04 19:20:52]

Thank you for your guidance and patience!

https://codereview.chromium.org/57483002/diff/200001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/200001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:58: if (!firstArg->IsArray()) {
On 2013/11/04 18:33:53, Chris Dumez wrote:
> Filing a bug and adding a FIXME comment here and in Blob to explain that we
> should handle Web IDL sequences would be acceptable for now.

Done.

Bug: http://crbug.com/314755

https://codereview.chromium.org/57483002/diff/200001/Source/core/fileapi/Blob...
File Source/core/fileapi/Blob.idl (left):

https://codereview.chromium.org/57483002/diff/200001/Source/core/fileapi/Blob...
Source/core/fileapi/Blob.idl:34: CustomConstructor,
On 2013/11/04 18:33:53, Chris Dumez wrote:
> No we actually need to keep this one. The File API spec does specify a
> constructor with no argument for Blob:
> http://dev.w3.org/2006/webapi/FileAPI/#dfn-Blob
> 
> We only needed to remove the one from File.

Done.

Sorry, I missed the difference between the version with two optional parameters
and a no-parameter version. I figured it out now.

[esprehn, 2013-11-04 19:28:08]

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/f...
File LayoutTests/fast/files/file-constructor.html (right):

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor.html:7: <script
src="script-tests/file-constructor.js"></script>
Blah script tests suck, I wish we'd just put the test in the html file.

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor.html:8: <script
src="../js/resources/js-test-post.js"></script>
Remove post.js, ojan is removing it.

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/r...
File LayoutTests/fast/files/resources/blob-slice-common.js (right):

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/r...
LayoutTests/fast/files/resources/blob-slice-common.js:67:
testRunner(notifyDone());
You don't need this, js-test-pre should do it for you?

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/post-formdata.html (right):

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/post-formdata.html:3: <head>
We often omit the html, head and body elements.

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/post-formdata.html:7: <script
type="text/javascript">
remove type attr

https://codereview.chromium.org/57483002/diff/320001/Source/core/fileapi/Blob...
File Source/core/fileapi/BlobBuilder.cpp (right):

https://codereview.chromium.org/57483002/diff/320001/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.cpp:140: PassRefPtr<File>
BlobBuilder::getFile(const String& contentType, const String& fileName, double
modificationTime)
createFile, method that create objects shouldn't be called get*()

https://codereview.chromium.org/57483002/diff/320001/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.cpp:150:
m_items.append(BlobDataItem(file->blobDataHandle(), 0, m_size));
What cleans this up to prevent leaks?

[pwnall-personal, 2013-11-04 23:01:20]

Thank you very much for the feedback! I addressed most of it, and have a couple
of questions. Can you please take another look?

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/f...
File LayoutTests/fast/files/file-constructor.html (right):

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor.html:7: <script
src="script-tests/file-constructor.js"></script>
On 2013/11/04 19:28:08, esprehn wrote:
> Blah script tests suck, I wish we'd just put the test in the html file.

I agree, separate files are a pain that's not worth it when the content isn't
shared by multiple tests.
I inlined {blob,file}-constructor in their tests.

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor.html:8: <script
src="../js/resources/js-test-post.js"></script>
On 2013/11/04 19:28:08, esprehn wrote:
> Remove post.js, ojan is removing it.

Done.

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/r...
File LayoutTests/fast/files/resources/blob-slice-common.js (right):

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/fast/files/r...
LayoutTests/fast/files/resources/blob-slice-common.js:67:
testRunner(notifyDone());
On 2013/11/04 19:28:08, esprehn wrote:
> You don't need this, js-test-pre should do it for you?

These tests weren't using js-test-pre. I reworked them. As a side benefit,
opening the test in a browser window gives some useful output.

js-test-pre didn't handle async tests well. I got a timeout and no error info
before. I added a 1-line fix to js-test-pre. Is this OK?

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/post-formdata.html (right):

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/post-formdata.html:3: <head>
On 2013/11/04 19:28:08, esprehn wrote:
> We often omit the html, head and body elements.

Done.
Also did this in the scripts around that I used for inspiration.

https://codereview.chromium.org/57483002/diff/320001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/post-formdata.html:7: <script
type="text/javascript">
On 2013/11/04 19:28:08, esprehn wrote:
> remove type attr

Done.

https://codereview.chromium.org/57483002/diff/320001/Source/core/fileapi/Blob...
File Source/core/fileapi/BlobBuilder.cpp (right):

https://codereview.chromium.org/57483002/diff/320001/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.cpp:140: PassRefPtr<File>
BlobBuilder::getFile(const String& contentType, const String& fileName, double
modificationTime)
On 2013/11/04 19:28:08, esprehn wrote:
> createFile, method that create objects shouldn't be called get*()

Actually, I think this whole class should be moved to platform. It is here
because it implements the deprecated BlobBuilder spec.
https://developer.mozilla.org/en-US/docs/Web/API/BlobBuilder

I used "getFile" because it is in the spec, and the signature roughly matches.
Do you still want me to rename it?

https://codereview.chromium.org/57483002/diff/320001/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.cpp:150:
m_items.append(BlobDataItem(file->blobDataHandle(), 0, m_size));
On 2013/11/04 19:28:08, esprehn wrote:
> What cleans this up to prevent leaks?

I borrowed this code from "getBlob", so I don't know too much about it. Here is
why I suspect it won't leak.

m_items is an empty Vector<BlobDataItem> after the blobData->swapItems() call.
The BlobDataItem is appended to it here, and will be cleaned up in the
auto-generated BlobBuilder destructor that will call the Vector<BlobDataItem>
destructor.

blobDataHandle() returns a PassRefPtr<BlobDataHandle>, and the BlobDataItem()
constructor takes one as an argument. If I understand the WebKit blog post
correctly, PassRefPtr acts a bit like C++ 11's move constructor, so the original
BlobDataHandle* is ref()'d in blobDataHandle() and then is passed straight into
BlobDataItem without ref()/deref() churn.

I'll read the whole source code for RefPtr and PassRefPtr and re-analyze. This
is what I think based on the bits I read so far, and on the WebKit blog post.
http://www.webkit.org/coding/RefPtr.html

[Inactive, 2013-11-05 00:38:47]

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/b...
File LayoutTests/fast/files/blob-constructor.html (right):

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/b...
LayoutTests/fast/files/blob-constructor.html:8: shouldBeTrue("(new Blob())
instanceof window.Blob");
nit: Would be nice to check that Blob.length is equal to 0

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/f...
File LayoutTests/fast/files/file-constructor-expected.txt (right):

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor-expected.txt:5: 
nit: Would be nice to check that File.length is equal to 2

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/js/reso...
File LayoutTests/fast/js/resources/js-test-pre.js (right):

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/js/reso...
LayoutTests/fast/js/resources/js-test-pre.js:98: finishJSTest();
The idea of this change was to help you debug your tests, right? or is this
really necessary for your tests to pass?

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/post-formdata-expected.txt (right):

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/post-formdata-expected.txt:12: FAIL
echoResult should be bareFile=file-name-override.html:text/html:file-value. Was
renamedFile=file-name-override.html:text/html:file-value.
Why are we failing this one?

https://codereview.chromium.org/57483002/diff/240005/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8BlobCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/240005/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8BlobCustom.cpp:53: // FIXME: handle WebIDL
sequences, possibly by using toV8Sequence
Nit: Would be good to indicate the bug number.

https://codereview.chromium.org/57483002/diff/240005/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/240005/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:58: // FIXME: handle WebIDL
sequences, possibly by using toV8Sequence
Nit: missing bug number

https://codereview.chromium.org/57483002/diff/240005/Source/core/fileapi/Blob...
File Source/core/fileapi/BlobBuilder.h (right):

https://codereview.chromium.org/57483002/diff/240005/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.h:52: PassRefPtr<File> getFile(const String&
contentType, const String& fileName, double modificationTime);
I agree with esprehn that getBlob() / getFile() is a poor name for factories. We
usually use createXXX() format. We don't expose this to JS so I don't think we
really need to be consistent with the naming of BlobBuilder (Could not find a
corresponding BlobBuilder.idl).

[pwnall-personal, 2013-11-05 01:14:23]

Thank you so much for the feedback!!

I'm not sure about what {Blob, File}.length should return. Can you please read
below and share your thoughts?

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/b...
File LayoutTests/fast/files/blob-constructor.html (right):

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/b...
LayoutTests/fast/files/blob-constructor.html:8: shouldBeTrue("(new Blob())
instanceof window.Blob");
On 2013/11/05 00:38:48, Chris Dumez wrote:
> nit: Would be nice to check that Blob.length is equal to 0

There actually is a test down below under "Odds and ends", and we're failing it,
because it wants Blob.length to be 2.

I'm a bit conflicted on this. MDN says that Function.length is the number of
formal parameters, and I think I would want that to be 2 for Blob and 3 for
File.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

What do you think? I can definitely change the expectation. The "FAIL" bothers
me a bit :)

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/f...
File LayoutTests/fast/files/file-constructor-expected.txt (right):

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor-expected.txt:5: 
On 2013/11/05 00:38:48, Chris Dumez wrote:
> nit: Would be nice to check that File.length is equal to 2

I can do it, if we agree that Blob.length should also be 0.

Alternatively, I can add a failing check for File.length == 3.

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/js/reso...
File LayoutTests/fast/js/resources/js-test-pre.js (right):

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/js/reso...
LayoutTests/fast/js/resources/js-test-pre.js:98: finishJSTest();
On 2013/11/05 00:38:48, Chris Dumez wrote:
> The idea of this change was to help you debug your tests, right? or is this
> really necessary for your tests to pass?

It is not necessary for tests to pass. At the same time, I think an error
message is better than a timeout for async tests, so I think this can be helpful
in the future.

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/post-formdata-expected.txt (right):

https://codereview.chromium.org/57483002/diff/240005/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/post-formdata-expected.txt:12: FAIL
echoResult should be bareFile=file-name-override.html:text/html:file-value. Was
renamedFile=file-name-override.html:text/html:file-value.
On 2013/11/05 00:38:48, Chris Dumez wrote:
> Why are we failing this one?

Fixed. Thank you for catching this!!

It was a FAIL because I can't read :(

https://codereview.chromium.org/57483002/diff/240005/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8BlobCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/240005/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8BlobCustom.cpp:53: // FIXME: handle WebIDL
sequences, possibly by using toV8Sequence
On 2013/11/05 00:38:48, Chris Dumez wrote:
> Nit: Would be good to indicate the bug number.

Done.

https://codereview.chromium.org/57483002/diff/240005/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/240005/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:58: // FIXME: handle WebIDL
sequences, possibly by using toV8Sequence
On 2013/11/05 00:38:48, Chris Dumez wrote:
> Nit: missing bug number

Done.

https://codereview.chromium.org/57483002/diff/240005/Source/core/fileapi/Blob...
File Source/core/fileapi/BlobBuilder.h (right):

https://codereview.chromium.org/57483002/diff/240005/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.h:52: PassRefPtr<File> getFile(const String&
contentType, const String& fileName, double modificationTime);
On 2013/11/05 00:38:48, Chris Dumez wrote:
> I agree with esprehn that getBlob() / getFile() is a poor name for factories.
We
> usually use createXXX() format. We don't expose this to JS so I don't think we
> really need to be consistent with the naming of BlobBuilder (Could not find a
> corresponding BlobBuilder.idl).

Done.
I renamed get{Blob, File} to create{Blob, File}.

BlobBuilder was a thing a few years ago. Then it was deprecated in favor of the
Blob constructor. I think this is why the IDL is gone, but the class still
exists.
http://dev.w3.org/2009/dap/file-system/file-writer.html#idl-def-BlobBuilder

I still think the whole class needs to go into platform/blob, or its
functionality should be completely assimilated into BlobData.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Follow-up refactoring maybe?

[kinuko, 2013-11-05 11:20:28]

https://codereview.chromium.org/57483002/diff/240005/Source/core/fileapi/Blob...
File Source/core/fileapi/BlobBuilder.h (right):

https://codereview.chromium.org/57483002/diff/240005/Source/core/fileapi/Blob...
Source/core/fileapi/BlobBuilder.h:52: PassRefPtr<File> getFile(const String&
contentType, const String& fileName, double modificationTime);
On 2013/11/05 01:14:23, pwnall wrote:
> On 2013/11/05 00:38:48, Chris Dumez wrote:
> > I agree with esprehn that getBlob() / getFile() is a poor name for
factories.
> We
> > usually use createXXX() format. We don't expose this to JS so I don't think
we
> > really need to be consistent with the naming of BlobBuilder (Could not find
a
> > corresponding BlobBuilder.idl).
> 
> Done.
> I renamed get{Blob, File} to create{Blob, File}.
> 
> BlobBuilder was a thing a few years ago. Then it was deprecated in favor of
the
> Blob constructor. I think this is why the IDL is gone, but the class still
> exists.
> http://dev.w3.org/2009/dap/file-system/file-writer.html#idl-def-BlobBuilder
> 
> I still think the whole class needs to go into platform/blob, or its
> functionality should be completely assimilated into BlobData.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Follow-up refactoring maybe?

Right, there's no reason to keep this class any more. We should be able to clean
up BlobBuilder and code around it.

https://codereview.chromium.org/57483002/diff/660001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/660001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:128: 
nit: no need to add an empty line here

https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File.h
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File...
Source/core/fileapi/File.h:90: virtual bool isOnFilesystem() const { return
m_isOnFilesystem; }
nit: the term FileSystem is often used to note FileSystem API in File API
context, and I'm afraid this method name may be confusing (while this name
itself perfectly makes sense).  I don't have a great idea but could this be
renamed to, like, hasBackingFile or something that is less confusing?

[pwnall-personal, 2013-11-05 11:44:34]

I created a new bug to track the BlobBuilder refactoring.
http://crbug.com/315038

https://codereview.chromium.org/57483002/diff/660001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/660001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:128: 
On 2013/11/05 11:20:29, kinuko wrote:
> nit: no need to add an empty line here

Done.

https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File.h
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File...
Source/core/fileapi/File.h:90: virtual bool isOnFilesystem() const { return
m_isOnFilesystem; }
On 2013/11/05 11:20:29, kinuko wrote:
> nit: the term FileSystem is often used to note FileSystem API in File API
> context, and I'm afraid this method name may be confusing (while this name
> itself perfectly makes sense).  I don't have a great idea but could this be
> renamed to, like, hasBackingFile or something that is less confusing?

I agree that the name isn't very good, and will change it to anything we agree
on.

[Inactive, 2013-11-05 14:36:14]

On 2013/11/05 01:14:23, pwnall wrote:
> Thank you so much for the feedback!!
> 
> I'm not sure about what {Blob, File}.length should return. Can you please read
> below and share your thoughts?
> 
>
https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/b...
> File LayoutTests/fast/files/blob-constructor.html (right):
> 
>
https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/b...
> LayoutTests/fast/files/blob-constructor.html:8: shouldBeTrue("(new Blob())
> instanceof window.Blob");
> On 2013/11/05 00:38:48, Chris Dumez wrote:
> > nit: Would be nice to check that Blob.length is equal to 0
> 
> There actually is a test down below under "Odds and ends", and we're failing
it,
> because it wants Blob.length to be 2.
> 
> I'm a bit conflicted on this. MDN says that Function.length is the number of
> formal parameters, and I think I would want that to be 2 for Blob and 3 for
> File.
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> 
> What do you think? I can definitely change the expectation. The "FAIL" bothers
> me a bit :)
> 
>
https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/f...
> File LayoutTests/fast/files/file-constructor-expected.txt (right):
> 
>
https://codereview.chromium.org/57483002/diff/240005/LayoutTests/fast/files/f...
> LayoutTests/fast/files/file-constructor-expected.txt:5: 
> On 2013/11/05 00:38:48, Chris Dumez wrote:
> > nit: Would be nice to check that File.length is equal to 2
> 
> I can do it, if we agree that Blob.length should also be 0.
> 
> Alternatively, I can add a failing check for File.length == 3.

The expected results are (as I mentioned earlier):
- Blob.length === 0
- File.length === 2
This is the shortest number of mandatory arguments among the constructors for
those interfaces, as per the Web IDL specification (end of
http://dev.w3.org/2006/webapi/WebIDL/#es-interface-call). If there are
expectations checking otherwise, please fix them as they are incorrect.
In any case, we need test coverage for this.

[Inactive, 2013-11-05 14:37:51]

https://codereview.chromium.org/57483002/diff/730001/LayoutTests/fast/files/b...
File LayoutTests/fast/files/blob-constructor.html (right):

https://codereview.chromium.org/57483002/diff/730001/LayoutTests/fast/files/b...
LayoutTests/fast/files/blob-constructor.html:70: shouldBe("window.Blob.length",
"2");
This should be 0 as per Web IDL spec.

[Inactive, 2013-11-05 14:43:38]

On 2013/11/05 11:20:28, kinuko wrote:
>
https://codereview.chromium.org/57483002/diff/240005/Source/core/fileapi/Blob...
> File Source/core/fileapi/BlobBuilder.h (right):
> 
>
https://codereview.chromium.org/57483002/diff/240005/Source/core/fileapi/Blob...
> Source/core/fileapi/BlobBuilder.h:52: PassRefPtr<File> getFile(const String&
> contentType, const String& fileName, double modificationTime);
> On 2013/11/05 01:14:23, pwnall wrote:
> > On 2013/11/05 00:38:48, Chris Dumez wrote:
> > > I agree with esprehn that getBlob() / getFile() is a poor name for
> factories.
> > We
> > > usually use createXXX() format. We don't expose this to JS so I don't
think
> we
> > > really need to be consistent with the naming of BlobBuilder (Could not
find
> a
> > > corresponding BlobBuilder.idl).
> > 
> > Done.
> > I renamed get{Blob, File} to create{Blob, File}.
> > 
> > BlobBuilder was a thing a few years ago. Then it was deprecated in favor of
> the
> > Blob constructor. I think this is why the IDL is gone, but the class still
> > exists.
> > http://dev.w3.org/2009/dap/file-system/file-writer.html#idl-def-BlobBuilder
> > 
> > I still think the whole class needs to go into platform/blob, or its
> > functionality should be completely assimilated into BlobData.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Follow-up refactoring maybe?
> 
> Right, there's no reason to keep this class any more. We should be able to
clean
> up BlobBuilder and code around it.
> 
>
https://codereview.chromium.org/57483002/diff/660001/Source/bindings/v8/custo...
> File Source/bindings/v8/custom/V8FileCustom.cpp (right):
> 
>
https://codereview.chromium.org/57483002/diff/660001/Source/bindings/v8/custo...
> Source/bindings/v8/custom/V8FileCustom.cpp:128: 
> nit: no need to add an empty line here
> 
>
https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File.h
> File Source/core/fileapi/File.h (right):
> 
>
https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File...
> Source/core/fileapi/File.h:90: virtual bool isOnFilesystem() const { return
> m_isOnFilesystem; }
> nit: the term FileSystem is often used to note FileSystem API in File API
> context, and I'm afraid this method name may be confusing (while this name
> itself perfectly makes sense).  I don't have a great idea but could this be
> renamed to, like, hasBackingFile or something that is less confusing?

Technically, we don't really need a method or a m_isOnFileSystem boolean, right?
I mean we could simply use the existing API:
blob->isFile() && !toFile(blob)->path().isNull()

[pwnall-personal, 2013-11-05 17:21:32]

On 2013/11/05 14:43:38, Chris Dumez wrote:

>
https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File.h
> > File Source/core/fileapi/File.h (right):
> > 
> >
>
https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File...
> > Source/core/fileapi/File.h:90: virtual bool isOnFilesystem() const { return
> > m_isOnFilesystem; }
> > nit: the term FileSystem is often used to note FileSystem API in File API
> > context, and I'm afraid this method name may be confusing (while this name
> > itself perfectly makes sense).  I don't have a great idea but could this be
> > renamed to, like, hasBackingFile or something that is less confusing?
> 
> Technically, we don't really need a method or a m_isOnFileSystem boolean,
right?
> I mean we could simply use the existing API:
> blob->isFile() && !toFile(blob)->path().isNull()

I can definitely replace the call with those lines. I preferred a method because
I think that makes callers know too much about Blob and File's internals. I
added a method because I expect that in the (near) future we'll pull the code
for dealing with actual files in a subclass of File, and have the File::create*
methods do the right thing.

By the way, maybe the right method name is pointsToRealFile() or
pathPointsToRealFile()?

[Inactive, 2013-11-05 19:49:34]

On 2013/11/05 17:21:32, pwnall wrote:
> On 2013/11/05 14:43:38, Chris Dumez wrote:
> 
> >
>
https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File.h
> > > File Source/core/fileapi/File.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/57483002/diff/660001/Source/core/fileapi/File...
> > > Source/core/fileapi/File.h:90: virtual bool isOnFilesystem() const {
return
> > > m_isOnFilesystem; }
> > > nit: the term FileSystem is often used to note FileSystem API in File API
> > > context, and I'm afraid this method name may be confusing (while this name
> > > itself perfectly makes sense).  I don't have a great idea but could this
be
> > > renamed to, like, hasBackingFile or something that is less confusing?
> > 
> > Technically, we don't really need a method or a m_isOnFileSystem boolean,
> right?
> > I mean we could simply use the existing API:
> > blob->isFile() && !toFile(blob)->path().isNull()
> 
> I can definitely replace the call with those lines. I preferred a method
because
> I think that makes callers know too much about Blob and File's internals. I
> added a method because I expect that in the (near) future we'll pull the code
> for dealing with actual files in a subclass of File, and have the
File::create*
> methods do the right thing.
> 
> By the way, maybe the right method name is pointsToRealFile() or
> pathPointsToRealFile()?

If we want to keep the method, I would vote for the name proposed by kinuko
instead: hasBackingFile().

[pwnall-personal, 2013-11-05 21:21:56]

On 2013/11/05 19:49:34, Chris Dumez wrote:
> If we want to keep the method, I would vote for the name proposed by kinuko
> instead: hasBackingFile().

I renamed the method to hasBackingFile(). Thank you for your feedback!!

[Inactive, 2013-11-07 14:27:43]

https://codereview.chromium.org/57483002/diff/820001/LayoutTests/fast/files/b...
File LayoutTests/fast/files/blob-constructor.html (right):

https://codereview.chromium.org/57483002/diff/820001/LayoutTests/fast/files/b...
LayoutTests/fast/files/blob-constructor.html:70: shouldBe("window.Blob.length",
"2");
This should be 0, not 2.

https://codereview.chromium.org/57483002/diff/820001/LayoutTests/fast/files/f...
File LayoutTests/fast/files/file-constructor.html (right):

https://codereview.chromium.org/57483002/diff/820001/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor.html:62: 
Could you please add a check to make sure File.length is 2 ?

[pwnall-personal, 2013-11-08 07:36:40]

Thank you very much for the feedback! Can you please take another look?

Asides from addressing feedback, I also pulled the change to
LayoutTests/resources/js-tests.js into its own CL.
https://codereview.chromium.org/59903016/

https://codereview.chromium.org/57483002/diff/820001/LayoutTests/fast/files/b...
File LayoutTests/fast/files/blob-constructor.html (right):

https://codereview.chromium.org/57483002/diff/820001/LayoutTests/fast/files/b...
LayoutTests/fast/files/blob-constructor.html:70: shouldBe("window.Blob.length",
"2");
On 2013/11/07 14:27:44, Chris Dumez wrote:
> This should be 0, not 2.

Done.

Sorry, I missed your earlier comment on the WebIDL spec, and I just saw it now.
Thank you very much for bringing this up again, and for being so patient with
me!

https://codereview.chromium.org/57483002/diff/820001/LayoutTests/fast/files/f...
File LayoutTests/fast/files/file-constructor.html (right):

https://codereview.chromium.org/57483002/diff/820001/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor.html:62: 
On 2013/11/07 14:27:44, Chris Dumez wrote:
> Could you please add a check to make sure File.length is 2 ?

Done.

[Inactive, 2013-11-08 20:37:13]

lgtm % nits.

Please let an API owner take a look before landing. kinuko will probably want to
take a look as well since this is File API.

[Inactive, 2013-11-08 20:37:33]

https://codereview.chromium.org/57483002/diff/890001/LayoutTests/fast/files/f...
File LayoutTests/fast/files/file-constructor-expected.txt (right):

https://codereview.chromium.org/57483002/diff/890001/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor-expected.txt:27: FAIL (new File([new
Blob([new File([new Blob], 'world.txt')]])], 'world.html')) instanceof
window.File should be true. Threw exception SyntaxError: Unexpected token ]
There is FAIL here?

https://codereview.chromium.org/57483002/diff/890001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/890001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:64: v8::Local<v8::Value> secondArg =
info[1];
nit: I would get rid of this variable and use info[1] directly on the next line.

[pwnall-personal, 2013-11-08 21:25:06]

Thank you for the thorough and speedy review!

I added kinuko as a reviewer, and will wait for further feedback.

https://codereview.chromium.org/57483002/diff/890001/LayoutTests/fast/files/f...
File LayoutTests/fast/files/file-constructor-expected.txt (right):

https://codereview.chromium.org/57483002/diff/890001/LayoutTests/fast/files/f...
LayoutTests/fast/files/file-constructor-expected.txt:27: FAIL (new File([new
Blob([new File([new Blob], 'world.txt')]])], 'world.html')) instanceof
window.File should be true. Threw exception SyntaxError: Unexpected token ]
On 2013/11/08 20:37:34, Chris Dumez wrote:
> There is FAIL here?

Done. (Fixed)

Wow, test mistake again. Sorry.
I grepped through my commit's diff for "FAIL" and made sure I didn't do this
anywhere else.

https://codereview.chromium.org/57483002/diff/890001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/890001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:64: v8::Local<v8::Value> secondArg =
info[1];
On 2013/11/08 20:37:34, Chris Dumez wrote:
> nit: I would get rid of this variable and use info[1] directly on the next
line.

Done.

[abarth-chromium, 2013-11-09 20:19:15]

https://codereview.chromium.org/57483002/diff/890007/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/890007/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:45: void
V8File::constructorCustom(const v8::FunctionCallbackInfo<v8::Value>& info)
This code appears to be a lot of copy/paste from V8BlobCustom.  Rather than
copy/pasting code, can we refactor the code so that we can share more of the
implementation?

[pwnall-personal, 2013-11-09 20:22:32]

On 2013/11/09 20:19:15, abarth wrote:
>
https://codereview.chromium.org/57483002/diff/890007/Source/bindings/v8/custo...
> File Source/bindings/v8/custom/V8FileCustom.cpp (right):
> 
>
https://codereview.chromium.org/57483002/diff/890007/Source/bindings/v8/custo...
> Source/bindings/v8/custom/V8FileCustom.cpp:45: void
> V8File::constructorCustom(const v8::FunctionCallbackInfo<v8::Value>& info)
> This code appears to be a lot of copy/paste from V8BlobCustom.  Rather than
> copy/pasting code, can we refactor the code so that we can share more of the
> implementation?

I really want to! I've been meaning to ask... what's the best way of doing that?

Would it be OK to create a V8BlobCustom.h in bindings/ and declare some
functions in a V8BlobCustomHelpers namespace?

[pwnall-personal, 2013-11-10 03:14:57]

I took a stab at the refactoring. Please take a look?

https://codereview.chromium.org/57483002/diff/890007/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/890007/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:45: void
V8File::constructorCustom(const v8::FunctionCallbackInfo<v8::Value>& info)
On 2013/11/09 20:19:16, abarth wrote:
> This code appears to be a lot of copy/paste from V8BlobCustom.  Rather than
> copy/pasting code, can we refactor the code so that we can share more of the
> implementation?

Done.

Can you please take a look at the refactoring? This is my first foray into V8
bindings code.

https://codereview.chromium.org/57483002/diff/1070001/Source/platform/Runtime...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/57483002/diff/1070001/Source/platform/Runtime...
Source/platform/RuntimeEnabledFeatures.in:104: SpeechSynthesis status=stable
This comes from a rebase. I'm not shipping features randomly :)
https://codereview.chromium.org/63043003/patch/220001/230002

[pwnall-personal, 2013-11-14 03:42:22]

I updated this patch twice because the code underneath changed.

Adam, Kinuko, can you please take a look?

[kinuko, 2013-11-14 05:41:05]

Mostly nits. (Sorry for my slow review)

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-constructor.html (left):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-constructor.html:7: <script
src="script-tests/blob-constructor.js"></script>
(It's fine for this CL, but it'd have been probably easier for review if you did
this expansion in a separate CL.)

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
File LayoutTests/fast/files/file-constructor.html (right):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/file-constructor.html:19: shouldThrow("(new File())",
"'TypeError: File constructor requires at least two arguments'");
Maybe also test 'new File([])' case?

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/file-constructor.html:26: // Test invalid blob parts
nit: please end comment with '.'

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
File LayoutTests/fast/files/resources/blob-slice-common.js (right):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/resources/blob-slice-common.js:5: function
testSlicing(start, end, expectedResult)
nit: I feel it might be cleaner to make this function take one more parameter
and run twice for blob and file rather than duplicating every code in it.

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/resources/blob-slice-common.js:24: blobReader.onload =
function(event) {
nit: I know this is coming from the original code, but onloadend might be more
appropriate here (and below)?

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-async.html
(right):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-async.html:56:
function testBlobContentTypeAsync(url, mimeType, fileName) {
mostly style/cleanup change code churn other than this part?

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/post-formdata.html (right):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/post-formdata.html:70: } else {
nit: no need of else as we return in the former case (well, looks like it's also
from original code)

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/post-formdata.html:78:
formData.append(fieldName, fieldValue);
nit: indent looks off?

https://codereview.chromium.org/57483002/diff/1390001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right):

https://codereview.chromium.org/57483002/diff/1390001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.h:30: 
This header file has no include guard-- is it intentional?

https://codereview.chromium.org/57483002/diff/1390001/Source/core/fileapi/Blob.h
File Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/57483002/diff/1390001/Source/core/fileapi/Blo...
Source/core/fileapi/Blob.h:65: // Only true for File instances sourced from the
filesystem.
'Only true for File instances that is backed by a platform file.' ? (to avoid
confusion around the term 'filesystem')

https://codereview.chromium.org/57483002/diff/1390001/Source/core/fileapi/Fil...
File Source/core/fileapi/File.idl (right):

https://codereview.chromium.org/57483002/diff/1390001/Source/core/fileapi/Fil...
Source/core/fileapi/File.idl:28: CustomConstructor(sequence<any> blobParts,
DOMString fileName, optional BlobPropertyBag options)
4 space indent?

[pwnall-personal, 2013-11-14 14:31:17]

Thank you very much for the thorough feedback!

I hope I addressed everything. Can you please take another look?

I added a fix from another CL, because of the code move to
V8BlobCustomHelpers.cpp and the LayoutTests inlining.

I also added a test for the empty-string file name, based on the comments in the
Firefox bug for the same feature.
https://bugzilla.mozilla.org/show_bug.cgi?id=819900

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
File LayoutTests/fast/files/blob-constructor.html (left):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/blob-constructor.html:7: <script
src="script-tests/blob-constructor.js"></script>
On 2013/11/14 05:41:06, kinuko wrote:
> (It's fine for this CL, but it'd have been probably easier for review if you
did
> this expansion in a separate CL.)

I'm really sorry about that, and appreciate you reviewing the CL my mess-up.

I'll do better in future CLs!

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
File LayoutTests/fast/files/file-constructor.html (right):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/file-constructor.html:19: shouldThrow("(new File())",
"'TypeError: File constructor requires at least two arguments'");
On 2013/11/14 05:41:06, kinuko wrote:
> Maybe also test 'new File([])' case?

Done.
Thank you, this is a great idea!

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/file-constructor.html:26: // Test invalid blob parts
On 2013/11/14 05:41:06, kinuko wrote:
> nit: please end comment with '.'

Done.
I fixed this throughout the file.

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
File LayoutTests/fast/files/resources/blob-slice-common.js (right):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/resources/blob-slice-common.js:5: function
testSlicing(start, end, expectedResult)
On 2013/11/14 05:41:06, kinuko wrote:
> nit: I feel it might be cleaner to make this function take one more parameter
> and run twice for blob and file rather than duplicating every code in it.

Done.
I really like this idea, thank you very much!

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/fast/files/...
LayoutTests/fast/files/resources/blob-slice-common.js:24: blobReader.onload =
function(event) {
On 2013/11/14 05:41:06, kinuko wrote:
> nit: I know this is coming from the original code, but onloadend might be more
> appropriate here (and below)?

Done.

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-async.html
(right):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-async.html:56:
function testBlobContentTypeAsync(url, mimeType, fileName) {
On 2013/11/14 05:41:06, kinuko wrote:
> mostly style/cleanup change code churn other than this part?

Yes. The "if (filename)" branch is new, and it uses the new test cases in
post-blob-content-type-tests.js.
Everything else is old code, with a bit of cleanup.

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/post-formdata.html (right):

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/post-formdata.html:70: } else {
On 2013/11/14 05:41:06, kinuko wrote:
> nit: no need of else as we return in the former case (well, looks like it's
also
> from original code)

Done.

https://codereview.chromium.org/57483002/diff/1390001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/post-formdata.html:78:
formData.append(fieldName, fieldValue);
On 2013/11/14 05:41:06, kinuko wrote:
> nit: indent looks off?

Done.
I also removed the brackets, since both branches only had one statement each.

https://codereview.chromium.org/57483002/diff/1390001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right):

https://codereview.chromium.org/57483002/diff/1390001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.h:30: 
On 2013/11/14 05:41:06, kinuko wrote:
> This header file has no include guard-- is it intentional?

Done.
Yikes! Thank you for catching this!

https://codereview.chromium.org/57483002/diff/1390001/Source/core/fileapi/Blob.h
File Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/57483002/diff/1390001/Source/core/fileapi/Blo...
Source/core/fileapi/Blob.h:65: // Only true for File instances sourced from the
filesystem.
On 2013/11/14 05:41:06, kinuko wrote:
> 'Only true for File instances that is backed by a platform file.' ? (to avoid
> confusion around the term 'filesystem')

Done.

https://codereview.chromium.org/57483002/diff/1390001/Source/core/fileapi/Fil...
File Source/core/fileapi/File.idl (right):

https://codereview.chromium.org/57483002/diff/1390001/Source/core/fileapi/Fil...
Source/core/fileapi/File.idl:28: CustomConstructor(sequence<any> blobParts,
DOMString fileName, optional BlobPropertyBag options)
On 2013/11/14 05:41:06, kinuko wrote:
> 4 space indent?

Done.

[kinuko, 2013-11-15 02:16:30]

lgtm, please have v8/binding part reviewed by abarth or haraken or someone who
know the part well. Thanks for working on this.

https://codereview.chromium.org/57483002/diff/1660001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-async.html
(right):

https://codereview.chromium.org/57483002/diff/1660001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-async.html:6:
description("Test verifies that content MIME type is set correctly " +
nit: usually I don't think you need to indent js code in <script>

[pwnall-personal, 2013-11-15 03:36:42]

Thank you very much for the feedback! I'm really grateful for the opportunity to
contribute!

https://codereview.chromium.org/57483002/diff/1660001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-async.html
(right):

https://codereview.chromium.org/57483002/diff/1660001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-async.html:6:
description("Test verifies that content MIME type is set correctly " +
On 2013/11/15 02:16:31, kinuko wrote:
> nit: usually I don't think you need to indent js code in <script>

Done.

[haraken, 2013-11-15 06:28:04]

Let me take a detailed look today.

[haraken, 2013-11-15 08:40:04]

LGTM

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:36: #include "wtf/RefPtr.h"

Nit: This won't be needed.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:44:
info.GetReturnValue().Set(toV8(blob.get(), info.Holder(), info.GetIsolate()));

Use v8SetReturnValue() in V8Binding.h.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:53: if
(toV8Sequence(info[sequenceArgumentIndex], length, info.GetIsolate()).IsEmpty())
{

I don't fully understand this code. You're calling toV8Sequence() for info[0],
which is not an Array. Then toV8Sequence() will always return false, won't it?

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:60: String endings = "transparent";

Nit: How about setting 'endings = "transparent"' in processBlobPropertyBag() ?
It's clearer to set both contentType and endings in processBlobPropertyBag().

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:67: if
(!V8BlobCustomHelpers::processBlobPropertyBag(info[1], info.GetIsolate(),
"Blob", contentType, endings))

processBlobPropertyBag => processBlobDictionary ?

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:78:
info.GetReturnValue().Set(toV8(blob.get(), info.Holder(), info.GetIsolate()));

Use v8SetReturnValue() in V8Binding.h.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:53:
throwTypeError(ExceptionMessages::failedToConstruct(blobClassName, "The
\"endings\" property must be either \"transparent\" or \"native\"."), isolate);

Nit: You could use '...' and avoid \".

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:61: throwError(v8SyntaxError,
ExceptionMessages::failedToConstruct(blobClassName, "The \"type\" property must
consist of ASCII characters."), isolate);

Ditto.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.h:46: bool
processBlobPropertyBag(v8::Local<v8::Value> propertyBag, v8::Isolate*, const
char* blobClassName, String& contentType, String& endings);

Move the v8::Isolate* parameter to the end (It's a convention).

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.h:51: bool
processBlobParts(v8::Local<v8::Object> blobParts, uint32_t blobPartsLength,
v8::Isolate*, const String& endings, BlobBuilder&);

Ditto.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8FileCustom.cpp:37: #include "wtf/RefPtr.h"

Nit: This won't be needed.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8FileCustom.cpp:58: if
(toV8Sequence(info[sequenceArgumentIndex], length, info.GetIsolate()).IsEmpty())
{

I wonder toV8Sequence will always return false because you're calling
toV8Sequence for info[0] which is not an Array.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8FileCustom.cpp:67: String endings = "transparent";

Nit: You can set |endings| in processBlobPropertyBag().

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8FileCustom.cpp:86:
info.GetReturnValue().Set(toV8(file.get(), info.Holder(), info.GetIsolate()));

Use v8SetReturnValue().

[pwnall-personal, 2013-11-15 08:58:41]

Thank you very much for the review! I'll post an updated patch in a bit.

I have one question, can you please take a look?

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:67: if
(!V8BlobCustomHelpers::processBlobPropertyBag(info[1], info.GetIsolate(),
"Blob", contentType, endings))
On 2013/11/15 08:40:05, haraken wrote:
> 
> processBlobPropertyBag => processBlobDictionary ?

I used BlobPropertyBag because that's the name that the File API uses.
http://dev.w3.org/2006/webapi/FileAPI/#dfn-BlobPropertyBag

Should I still change it?

[haraken, 2013-11-15 09:09:39]

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:67: if
(!V8BlobCustomHelpers::processBlobPropertyBag(info[1], info.GetIsolate(),
"Blob", contentType, endings))
On 2013/11/15 08:58:42, pwnall wrote:
> On 2013/11/15 08:40:05, haraken wrote:
> > 
> > processBlobPropertyBag => processBlobDictionary ?
> 
> I used BlobPropertyBag because that's the name that the File API uses.
> http://dev.w3.org/2006/webapi/FileAPI/#dfn-BlobPropertyBag
> 
> Should I still change it?

Then let's call it processBlobPropertyBag. Sorry about the noise.

[pwnall-personal, 2013-11-15 10:28:51]

Thank you very much for your feedback once again!

There were a couple of places where I couldn't follow your directions. Can you
please take a look below?

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:36: #include "wtf/RefPtr.h"
On 2013/11/15 08:40:05, haraken wrote:
> 
> Nit: This won't be needed.

Done.
Thank you!

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:44:
info.GetReturnValue().Set(toV8(blob.get(), info.Holder(), info.GetIsolate()));
On 2013/11/15 08:40:05, haraken wrote:
> 
> Use v8SetReturnValue() in V8Binding.h.

Done.
I just looked up how this works. It's really cool! Thank you for pointing it
out!

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:53: if
(toV8Sequence(info[sequenceArgumentIndex], length, info.GetIsolate()).IsEmpty())
{
On 2013/11/15 08:40:05, haraken wrote:
> 
> I don't fully understand this code. You're calling toV8Sequence() for info[0],
> which is not an Array. Then toV8Sequence() will always return false, won't it?

FWIW, I think the toV8Sequence() method name is misleading w.r.t. its behavior.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Actually, it _asserts_ that its argument is not an array, so each caller is
required to have a separate code path for arrays. Our current definition of
"sequence" is an object (that is not RegExp or Date) with a "length" property
that is a 32-bit unsigned integer. I think this matches the current WebIDL spec.
http://heycam.github.io/webidl/#es-sequence

The discussion in the File constructor bug on Mozilla's tracker makes me believe
the spec will change in the future.
https://bugzilla.mozilla.org/show_bug.cgi?id=819900

If it is desirable, I can try to prepare a CL that makes toV8Sequence accept
arrays and pass them through. Would you like me to do that?

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:60: String endings = "transparent";
On 2013/11/15 08:40:05, haraken wrote:
> 
> Nit: How about setting 'endings = "transparent"' in processBlobPropertyBag() ?
> It's clearer to set both contentType and endings in processBlobPropertyBag().

I can't get rid of the default easily, because processBlobPropertyBag() is not
called at all if the optional argument is ignored. I added an ASSERT to the
beginning to document that I expect the caller to set endings to "transparent".
Is this good enough?

Alternatively, I can add a struct and move the initialization to its constructor
so we don't need magic values in V8{Blob,File}::constructorCustom, but my
intuition tells me that is overkill. Would you like me to do that? Something
else?

I also moved the ASSERT to the beginning of processBlobParts(), because it
really looks like a precondition check to me.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:67: if
(!V8BlobCustomHelpers::processBlobPropertyBag(info[1], info.GetIsolate(),
"Blob", contentType, endings))
On 2013/11/15 09:09:40, haraken wrote:
> Then let's call it processBlobPropertyBag. Sorry about the noise.

FWIW, I also find the name a bit awkward :)

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:78:
info.GetReturnValue().Set(toV8(blob.get(), info.Holder(), info.GetIsolate()));
On 2013/11/15 08:40:05, haraken wrote:
> 
> Use v8SetReturnValue() in V8Binding.h.

Done.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:53:
throwTypeError(ExceptionMessages::failedToConstruct(blobClassName, "The
\"endings\" property must be either \"transparent\" or \"native\"."), isolate);
On 2013/11/15 08:40:05, haraken wrote:
> 
> Nit: You could use '...' and avoid \".

Hm... AFAIK, single quotes mean char in C++. I tried out the following on MacOS
10.9 and it doesn't compile.

'The "endings" property must be either "transparent" or "native".'

I think I'm misunderstanding you... please advise?

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:61: throwError(v8SyntaxError,
ExceptionMessages::failedToConstruct(blobClassName, "The \"type\" property must
consist of ASCII characters."), isolate);
On 2013/11/15 08:40:05, haraken wrote:
> 
> Ditto.

Same issue as above. Sorry for being dense :(

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.h:46: bool
processBlobPropertyBag(v8::Local<v8::Value> propertyBag, v8::Isolate*, const
char* blobClassName, String& contentType, String& endings);
On 2013/11/15 08:40:05, haraken wrote:
> 
> Move the v8::Isolate* parameter to the end (It's a convention).

Done.
Thank you very much for explaining!

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.h:51: bool
processBlobParts(v8::Local<v8::Object> blobParts, uint32_t blobPartsLength,
v8::Isolate*, const String& endings, BlobBuilder&);
On 2013/11/15 08:40:05, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8FileCustom.cpp:37: #include "wtf/RefPtr.h"
On 2013/11/15 08:40:05, haraken wrote:
> 
> Nit: This won't be needed.

Done.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8FileCustom.cpp:58: if
(toV8Sequence(info[sequenceArgumentIndex], length, info.GetIsolate()).IsEmpty())
{
On 2013/11/15 08:40:05, haraken wrote:
> 
> I wonder toV8Sequence will always return false because you're calling
> toV8Sequence for info[0] which is not an Array.

This works the same as in the Blob constructor. WebIDL sequences are a superset
of arrays.

FWIW, this code path is covered by some of the tests towards the bottom of
LayoutTests/fast/files/{blob,file}-constructor.html.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8FileCustom.cpp:67: String endings = "transparent";
On 2013/11/15 08:40:05, haraken wrote:
> 
> Nit: You can set |endings| in processBlobPropertyBag().

This has the same issue as in V8BlobCustom. I'll follow your guidance for both
files.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8FileCustom.cpp:86:
info.GetReturnValue().Set(toV8(file.get(), info.Holder(), info.GetIsolate()));
On 2013/11/15 08:40:05, haraken wrote:
> 
> Use v8SetReturnValue().

Done.

[haraken, 2013-11-15 10:39:11]

LGTM

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustom.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:53: if
(toV8Sequence(info[sequenceArgumentIndex], length, info.GetIsolate()).IsEmpty())
{
On 2013/11/15 10:28:53, pwnall wrote:
> On 2013/11/15 08:40:05, haraken wrote:
> > 
> > I don't fully understand this code. You're calling toV8Sequence() for
info[0],
> > which is not an Array. Then toV8Sequence() will always return false, won't
it?
> 
> FWIW, I think the toV8Sequence() method name is misleading w.r.t. its
behavior.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Actually, it _asserts_ that its argument is not an array, so each caller is
> required to have a separate code path for arrays. Our current definition of
> "sequence" is an object (that is not RegExp or Date) with a "length" property
> that is a 32-bit unsigned integer. I think this matches the current WebIDL
spec.
> http://heycam.github.io/webidl/#es-sequence
> 
> The discussion in the File constructor bug on Mozilla's tracker makes me
believe
> the spec will change in the future.
> https://bugzilla.mozilla.org/show_bug.cgi?id=819900
> 
> If it is desirable, I can try to prepare a CL that makes toV8Sequence accept
> arrays and pass them through. Would you like me to do that?

Ah, got it. I'd be happy if you could make the change in a follow-up. Per the
Web IDL spec (http://www.w3.org/TR/2012/CR-WebIDL-20120419/#es-sequence),
'sequence' should accept an array.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustom.cpp:60: String endings = "transparent";
> I can't get rid of the default easily, because processBlobPropertyBag() is not
> called at all if the optional argument is ignored. I added an ASSERT to the
> beginning to document that I expect the caller to set endings to
"transparent".
> Is this good enough?
> 
> Alternatively, I can add a struct and move the initialization to its
constructor
> so we don't need magic values in V8{Blob,File}::constructorCustom, but my
> intuition tells me that is overkill. Would you like me to do that? Something
> else?
> 
> I also moved the ASSERT to the beginning of processBlobParts(), because it
> really looks like a precondition check to me.

Understood. Let's keep it as is.

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8BlobCustomHelpers.cpp (right):

https://codereview.chromium.org/57483002/diff/1720001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8BlobCustomHelpers.cpp:53:
throwTypeError(ExceptionMessages::failedToConstruct(blobClassName, "The
\"endings\" property must be either \"transparent\" or \"native\"."), isolate);
On 2013/11/15 10:28:53, pwnall wrote:
> On 2013/11/15 08:40:05, haraken wrote:
> > 
> > Nit: You could use '...' and avoid \".
> 
> Hm... AFAIK, single quotes mean char in C++. I tried out the following on
MacOS
> 10.9 and it doesn't compile.
> 
> 'The "endings" property must be either "transparent" or "native".'
> 
> I think I'm misunderstanding you... please advise?

Sorry, you're right. Ignore my comment :)

[esprehn, 2013-11-15 11:17:10]

lgtm

[pwnall-personal, 2013-11-15 11:29:11]

On 2013/11/15 11:17:10, esprehn wrote:
> lgtm

Thank you very much!!

[commit-bot: I haz the power, 2013-11-15 12:09:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/costan@gmail.com/57483002/1900001

[commit-bot: I haz the power, 2013-11-15 13:24:44]

Change committed as 162116