Please add a File constructor

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: costan, Assigned: baku)

Tracking

({dev-doc-needed})

17 Branch
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Reporter

Description

7 years ago
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.
Reporter

Comment 1

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

public-webapps@w3.org 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
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...
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Sounds like a plan.  On the bright side, WebIDL-ifying it should not be too much pain, I hope.
WebIDL-ifying it is not really a pain if we don't want to de-virtualize anything.  But I do ;-)

Comment 6

6 years ago
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.
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.
Assignee

Comment 8

6 years ago
Posted patch constructor.patch (obsolete) — Splinter Review
http://dev.w3.org/2006/webapi/FileAPI/#file
[Constructor(Blob fileBits, [EnsureUTF16] DOMString fileName)]

Only: new File(blob, DOMString name) is supported.
Attachment #815880 - Flags: review?(khuey)
Assignee

Comment 10

6 years ago
(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);
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.
That seems like it allows creating name-less files.  Is that expected?
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...
Assignee

Comment 14

6 years ago
Posted patch constructor.patch (obsolete) — Splinter Review
Attachment #815880 - Attachment is obsolete: true
Attachment #815880 - Flags: review?(khuey)
Attachment #816199 - Flags: review?(khuey)
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.
Assignee: nobody → amarchesini
blocking-b2g: --- → koi+
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.
Assignee

Comment 17

6 years ago
(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?
Flags: needinfo?(jonas)
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).
Flags: needinfo?(jonas)
Renom since bug 920905 has been renomed as well.
blocking-b2g: koi+ → koi?
Assignee

Comment 20

6 years ago
Posted patch constructor.patch (obsolete) — Splinter Review
Attachment #816199 - Attachment is obsolete: true
Attachment #816199 - Flags: review?(khuey)
Attachment #823944 - Flags: review?(jonas)
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.
Attachment #823944 - Flags: review?(jonas) → review-
Moving to 1.3, please track for the release.
blocking-b2g: koi? → 1.3?
Assignee

Comment 23

6 years ago
Posted patch constructor.patch (obsolete) — Splinter Review
Attachment #823944 - Attachment is obsolete: true
Attachment #824141 - Flags: review?(jonas)
Assignee

Comment 24

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

This should be a follow up.
Assignee

Comment 25

6 years ago
Posted patch constructor.patch (obsolete) — Splinter Review
Attachment #824141 - Attachment is obsolete: true
Attachment #824141 - Flags: review?(jonas)
Attachment #824145 - Flags: review?(jonas)
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.
Attachment #824145 - Flags: review?(jonas) → review+
Assignee

Comment 27

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

Can the name be an empty string? I don't think so.
Flags: needinfo?(jonas)
Assignee

Comment 28

6 years ago
Posted patch constructor.patch (obsolete) — Splinter Review
Attachment #824145 - Attachment is obsolete: true
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"], "");
Flags: needinfo?(jonas)
Assignee

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9640f9fda733
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Ryan

Anything on master will automatically be on 1.3 right? At least till 12/9. SO removing 1.3?
blocking-b2g: 1.3? → -
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.