819900 - Please add a File constructor

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Victor Costan]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.21 (KHTML, like Gecko) Chrome/25.0.1349.2 Safari/537.21

Steps to reproduce:

Type the following lines in the Web console:

var blob = new Blob(["<p>Hello world!</p>"], { type: "text/html" });
new File(blob, "hello.html");
new File(["<p>Hello world!</p>"], { type: "text/html", name: "hello.html" });


Actual results:

Lines 2 and 3 throw [Exception... "The operation is insecure."  code: "18" nsresult: "0x80530012 (SecurityError)"  location: "Web Console Line: 1"]


Expected results:

It should be possible to build Files out of Blobs. Either the 2nd line or the 3rd line should return a File instance.

Comment 1

[Victor Costan]

WHATWG is waiting for implementations for this spec change.
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-December/038269.html

[e-mail] is also discussing this change.
http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/0692.html

The ability to build Files out of Blobs was lost during the switch from BlobBuilder to the Blob constructor.

I would like to be able to build File instances so that I can write automated tests for all the input types that my code supports. In Chrome, I can use the FileSystem API to build File instances (though I consider that a hack around the lack of a File constructor), but in Firefox I can't find any way to create a File from JavaScript.

Cross-reference: http://crbug.com/164933

Comment 2

[Boris Zbarsky [:bzbarsky]]

Kyle, is the File constructor hooked up to nsDOMFileFile right now, basically?

If so, seems like the right thing here is to move File to WebIDL, and then we can use constructor overloads to select the right implementation...

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes to both, although we're about to move the File constructor to nsDOMMultipartFile.

Our File API code is a complete mess.  After b2g freezes I plan on refactoring it all.  That's probably late January/early February.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sounds like a plan.  On the bright side, WebIDL-ifying it should not be too much pain, I hope.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

WebIDL-ifying it is not really a pain if we don't want to de-virtualize anything.  But I do ;-)

Comment 6

[Eiji Kitamura]

Not sure if this is related but

var b = new Blob();
var name = b.constructor.name;
console.log(name)

returns undefined. File object as well.
This bug requires me special care on Blob and File while iterating through list of objects and check their types.

Comment 7

[Boris Zbarsky [:bzbarsky]]

That's not related, though note that nothing in ES5 says function objects have a .name, so relying on that is suspect to start with.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: constructor.patch

http://dev.w3.org/2006/webapi/FileAPI/#file
[Constructor(Blob fileBits, [EnsureUTF16] DOMString fileName)]

Only: new File(blob, DOMString name) is supported.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I really think we should use the syntax proposed here instead:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=23479

Comment 10

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) from comment #9)
> I really think we should use the syntax proposed here instead:
> 
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=23479

what about if we support both syntaxes?

new File([blob], { type: blob.type, name: name });
new File([blob], name);

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should just do

new File([blob], { type: blob.type, name: name });

for now, it seems less controversial to me since it just matches the Blob constructor syntax. Though feel free to join the discussion in the W3C bug.

Comment 12

[Boris Zbarsky [:bzbarsky]]

That seems like it allows creating name-less files.  Is that expected?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes. It's not perfect. But I think the consistency with |new Blob| is worth it.

I guess we could do |new File([data], name, { options })|.

That would even support doing |new File([blob], "theName", { blob })| as a way to wrap a named File around a Blob while forwarding all properties...

Comment 14

[Andrea Marchesini [:baku]]

Attachment: constructor.patch

Comment 15

[Andrew Overholt [:overholt]]

This blocks another koi+ blocker so I'm marking it as koi+, too.  If we find another way to solve bug 920905, we can remove the blocker status here.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, looks like the syntax is going to be:

new File([data], DOMString name, optional BlobPropertyBag options);

compare to

new Blob([data], optional BlobPropertyBag options);

I.e. the first and the last argument of the two is exactly the same. The only difference is that File has a required 'name' argument in the middle.

Comment 17

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #16)
> Ok, looks like the syntax is going to be:
> 
> new File([data], DOMString name, optional BlobPropertyBag options);
> 
> compare to
> 
> new Blob([data], optional BlobPropertyBag options);
> 
> I.e. the first and the last argument of the two is exactly the same. The
> only difference is that File has a required 'name' argument in the middle.

This means that if I have a blob, I cannot create a File, is it?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes you can. You can do

myFile = new File([myBlob], "filename")

That creates a new File with the same contents as myBlob. You can even do

myFile = new File([myBlob], "filename", myBlob)

Which treats myBlob as a dictionary and copies over the type (and any future parameters that we add).

Comment 19

[Jason Smith [:jsmith]]

Renom since bug 920905 has been renomed as well.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: constructor.patch

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 823944 [details] [diff] [review]
constructor.patch

Review of attachment 823944 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +180,5 @@
> +  if (!nsContentUtils::IsCallerChrome()) {
> +    return InitFile(aCx, aArgs.length(), aArgs.array());
> +  }
> +
> +  return InitChromeFile(aCx, aArgs.length(), aArgs.array());

Actually, even for chrome callers we should call into InitFile if the first argument is an array. Ideally chrome code should migrate to the new syntax too.

@@ +384,5 @@
> +  // File name
> +  if (aArgc > 1) {
> +    if (!aArgv[1].isString()) {
> +      return NS_ERROR_TYPE_ERR;
> +    }

Remove this. We should just call JS_ValueToString on whatever value is passed in.

@@ +400,5 @@
> +
> +  // Optional params
> +  bool nativeEOL = false;
> +  if (aArgc > 2) {
> +    FilePropertyBag d;

Use BlobPropertyBag here

@@ +423,5 @@
> +
> +  uint32_t length;
> +  JS_ALWAYS_TRUE(JS_GetArrayLength(aCx, obj, &length));
> +  for (uint32_t i = 0; i < length; ++i) {
> +    JS::Rooted<JS::Value> element(aCx);

We really should share this loop with the |new Blob| code.

@@ +458,5 @@
> +
> +      // neither Blob nor ArrayBuffer(View)
> +    } else if (element.isString()) {
> +      blobSet.AppendString(element.toString(), nativeEOL, aCx);
> +      continue;

You don't need this. The catch-all code below will cover this case.

Comment 22

[Preeti Raghunath(:Preeti)]

Moving to 1.3, please track for the release.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: constructor.patch

Comment 24

[Andrea Marchesini [:baku]]

> Ideally chrome code should migrate to the new syntax
> too.

This should be a follow up.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: constructor.patch

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 824145 [details] [diff] [review]
constructor.patch

Review of attachment 824145 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +180,5 @@
> +  if (!nsContentUtils::IsCallerChrome()) {
> +    return InitFile(aCx, aArgs.length(), aArgs.array());
> +  }
> +
> +  if (aArgs.length() > 1) {

This should be > 0, or >= 1

@@ +392,5 @@
> +  NS_ASSERTION(!mImmutable, "Something went wrong ...");
> +  NS_ENSURE_TRUE(!mImmutable, NS_ERROR_UNEXPECTED);
> +
> +  if (aArgc == 0) {
> +    return NS_ERROR_TYPE_ERR;

You should throw here if aArgc < 2. The name is a required argument.

@@ +396,5 @@
> +    return NS_ERROR_TYPE_ERR;
> +  }
> +
> +  // File name
> +  if (aArgc > 1) {

And then remove this check as it should always be true

::: content/base/test/test_file_from_blob.html
@@ +41,5 @@
> +    status = true;
> +  }
> +  ok(status, "File throws if the argument is not an array");
> +
> +  f = new File(['1234567890']);

This should throw due to lacking 2nd argument

@@ +71,5 @@
> +  is(f.size, 10, 'File has the right size');
> +  is(f.name, 'text.txt');
> +  is(f.type, 'plain/text');
> +
> +  f = new File([b]);

Same here

@@ +89,5 @@
> +  is(f.name, 'text.txt');
> +  is(f.type, '');
> +  is(f.size, b.size);
> +
> +  f = new File([b], 'test.txt', { type: 'plain/text' });

Also try passing |b| as the 3rd argument. We should pick up the .type from the blob.

Comment 27

[Andrea Marchesini [:baku]]

> > +  f = new File([b], 'test.txt', { type: 'plain/text' });

Can the name be an empty string? I don't think so.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: constructor.patch

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should allow empty-string names. As long as the name is specified explicitly. So the following should IMO be fine:

new File(["foopy"], "");

Comment 30

[Andrea Marchesini [:baku]]

Attachment: constructor.patch

https://tbpl.mozilla.org/?tree=Try&rev=fcd99db83056

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9640f9fda733

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9640f9fda733

Comment 33

[Preeti Raghunath(:Preeti)]

Ryan

Anything on master will automatically be on 1.3 right? At least till 12/9. SO removing 1.3?

Comment 34

[Ryan VanderMeulen [:RyanVM]]

(In reply to Preeti Raghunath(:Preeti) from comment #33)
> Ryan
> 
> Anything on master will automatically be on 1.3 right? At least till 12/9.
> SO removing 1.3?

Correct. That said, people often set blocking status on already-landed bugs so it's clear even if it were backed out or something.