757664 - Make the files attribute of HTMLInputElement writable

Status: RESOLVED DUPLICATE of bug 1384030

(Core :: DOM: Core & HTML, defect)

Description

[thakis]

Attachment: Patch

The files attribute of the input element is currently marked readonly
[1], to protect from `myInput.files = "/etc/passwd"; myForm.submit()`.
Since its type is now FileList and not string, that's no longer
necessary.

Making the attribute writable would allow setting the files property
of an input element to dataTransfer.files from a drop handler. For
example, I would like to use this to create a larger drop-target for a
file input. Here's one request for this functionality:
http://stackoverflow.com/questions/8006715/drag-drop-files-into-standard-html-file-input

1: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#the-input-element



---

whatwg thread: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-May/036140.html
webkit change: https://bugs.webkit.org/show_bug.cgi?id=87154


---

I'm happy to add a test too, but https://developer.mozilla.org/en/Mozilla_automated_testing listed so many options that I couldn't decide on one. Please let me know what the best test type is for this change, if any.

Comment 1

[thakis]

Comment on attachment 626246 [details]
Patch

`hg log` suggests that sicking might be a good reviewer for this.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 626246 [details]
Patch

You should update the uuid of nsIDOMHTMLInputElement and write some tests.

Comment 3

[thakis]

Attachment: patch

Thanks for the comment! I suppose uuids always need to be updated when an interface changes. I just created an arbitrary new uuid, is that ok?

Can you point me to the directory of the existing input element tests? As mentioned above, I couldn't find them.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

The relevant test location would probably be http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/ . There are other tests there you can use for inspiration. Those are "plain" Mochitests: https://developer.mozilla.org/en/Mozilla_automated_testing#Mochitest_%28M%29

See https://developer.mozilla.org/en/Mochitest#Running_select_tests for how to run them.

Comment 5

[thakis]

Attachment: patch

Now with test. I haven't run the test yet (still waiting for my build), but it's probably good enough that you can tell me if this is basically the right way to go.

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 628547 [details] [diff] [review]
patch

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

The patch looks good but the tests need a bit of love.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1310,3 @@
>  }
>  
>  nsresult

NS_IMETHODIMP

::: content/html/content/test/forms/test_input_files_writable.html
@@ +18,5 @@
> +var input2 = document.forms[0].input2;
> +
> +var myFile = ["/foo"];
> +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +input1.mozSetFileNameArray(myFile, myFile.length);

I thought MozSetFileNameArray() was ignoring files not part of the file system. Is /foo in your machine? Or is that some kind of magic file we are using?

The method I use is what is in content/html/content/test/forms/test_required_attribute.html (checkInputRequiredValidityForFile method).

@@ +21,5 @@
> +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +input1.mozSetFileNameArray(myFile, myFile.length);
> +netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> +
> +input2.files = input1.files;

Could you test that this is not throwing?

@@ +23,5 @@
> +netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> +
> +input2.files = input1.files;
> +
> +ok(input2.files.length === 1, "files attribute should be writable");

is(input2.files.length, 1, "...");

Could you also check that input2.files.length is equal to 0 before the set.
Also, could you check that .value was returning "" and is now returning the file name?

Comment 7

[thakis]

Attachment: patch

Comment 8

[thakis]

> ::: content/html/content/src/nsHTMLInputElement.cpp
> @@ +1310,3 @@
> >  }
> >  
> >  nsresult
> 
> NS_IMETHODIMP

Done.

> 
> ::: content/html/content/test/forms/test_input_files_writable.html
> @@ +18,5 @@
> > +var input2 = document.forms[0].input2;
> > +
> > +var myFile = ["/foo"];
> > +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > +input1.mozSetFileNameArray(myFile, myFile.length);
> 
> I thought MozSetFileNameArray() was ignoring files not part of the file
> system. Is /foo in your machine? Or is that some kind of magic file we are
> using?

I found this in some other test. I can't find where I found it though. I don't have /foo on my machine, yet still this seems to work.

> @@ +21,5 @@
> > +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > +input1.mozSetFileNameArray(myFile, myFile.length);
> > +netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> > +
> > +input2.files = input1.files;
> 
> Could you test that this is not throwing?

I verified that mochitest implicitly complains about every thrown exception, so that's already done.

> @@ +23,5 @@
> > +netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> > +
> > +input2.files = input1.files;
> > +
> > +ok(input2.files.length === 1, "files attribute should be writable");
> 
> is(input2.files.length, 1, "...");
> 
> Could you also check that input2.files.length is equal to 0 before the set.
> Also, could you check that .value was returning "" and is now returning the
> file name?

Done.

Comment 9

[thakis]

Attachment: patch

Comment 10

[thakis]

(I verified that the test fails without the code change but passes with it.)

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 628895 [details] [diff] [review]
patch

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

r=me with the requested changes.

Though, this needs a super-review from Jonas.

::: content/html/content/test/forms/test_input_files_writable.html
@@ +34,5 @@
> +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +input1.mozSetFileNameArray(myFile, myFile.length);
> +
> +input2.files = input1.files;
> +is(input2.files.length, 1, "files attribute should be writable");

I would really prefer something like that:
caught = false;
try {
  input2.files = input1.files;
} catch(e) {
  caught = true;
}
is(caught, false, "files attribute should be writable");
is(input2.files.length, 1, "setting the files attribute should have updated it");

Comment 12

[thakis]

> ::: content/html/content/test/forms/test_input_files_writable.html
> @@ +34,5 @@
> > +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > +input1.mozSetFileNameArray(myFile, myFile.length);
> > +
> > +input2.files = input1.files;
> > +is(input2.files.length, 1, "files attribute should be writable");
> 
> I would really prefer something like that:
> caught = false;
> try {
>   input2.files = input1.files;
> } catch(e) {
>   caught = true;
> }
> is(caught, false, "files attribute should be writable");
> is(input2.files.length, 1, "setting the files attribute should have updated
> it");

What's the advantage of doing that? If the assignment throws, mochitest will fail the test anway, and it's 6 lines less as is.

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to thakis from comment #12)
> > ::: content/html/content/test/forms/test_input_files_writable.html
> > @@ +34,5 @@
> > > +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > > +input1.mozSetFileNameArray(myFile, myFile.length);
> > > +
> > > +input2.files = input1.files;
> > > +is(input2.files.length, 1, "files attribute should be writable");
> > 
> > I would really prefer something like that:
> > caught = false;
> > try {
> >   input2.files = input1.files;
> > } catch(e) {
> >   caught = true;
> > }
> > is(caught, false, "files attribute should be writable");
> > is(input2.files.length, 1, "setting the files attribute should have updated
> > it");
> 
> What's the advantage of doing that? If the assignment throws, mochitest will
> fail the test anway, and it's 6 lines less as is.

1. This is something that should fail without the patch and doesn't fail with the patch.
2. Explicit check is always better than implicit check.
3. If something goes wrong, in one blink a developer would be able to understand what is happening.

Comment 14

[thakis]

Attachment: patch

With the requested test change.

Comment 15

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

We need to think about the security implications of this and how it interacts with session restore before landing.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 629233 [details] [diff] [review]
patch

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

r=me. The patch looks fine. Thank you! :)

I will let Jonas taking the decision regarding whether we want that change to happen.
We might also need a security review if the change is pushed to m-c.

Comment 17

[Mounir Lamouri (:mounir)]

I guess we can mark this bug as confirmed for the moment, keeping the option of marking it WONTFIX if we don't want that feature.

Comment 18

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

I think we need to integrate with session-storage *where we can*. I.e. I don't think we should take arbitrary blobs and store them in session storage. However if someone takes a file-list, removes a couple of files, and then re-inserts it, then that should work with session-storage.

I don't see how the patch as-is would work given that we have no API for creating FileLists. We should make it possible to set any array containing Files as the value for .files.

We need to make sure to update the rendered text when .files is set. I'm not sure if the current patch does that or not.

Comment 19

[thakis]

My use case is to install an html5 drop handler, which gets a file list. So there's no need to create FileLists.

Supporting an array of Files can be done in a follow-up; I'm not interested in working on that part.

The current patch updates the rendered text.

Session-storage integration should work just as well as it does for files added through the file picker dialog.

Comment 20

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

I'd rather not land a half-baked patch, that's generally not how we do things in Gecko. If we want to do this, we should do it right.

Comment 21

[thakis]

I don't see how this is half-baked. It's functional, tested, does what it's supposed to do, and is better than what's currently in. It's forward-compatible with supporting arrays of Files if someone wants to implement that.

Comment 22

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

For one it means that pages can't do feature detection by assigning into a .files property. Instead you have to wait for a drop event before figuring out the platform capabilities.

Comment 23

[thakis]

True, but is that worse than not being able to do this at all?

Comment 24

[emmanuel.joubaud]

What thakis proposes is the way it works in Chrome, who doesn't support creating FileList either.
And the use case is huge : there is no other way to use drag n drop in a form without AJAX (which means server-side complexity, temp file...)

Comment 25

[Mounir Lamouri (:mounir)]

Sorry, wrong bug...

Comment 26

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

Comment on attachment 629233 [details] [diff] [review]
patch

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

I'll let mounir make a decision here. I still think that we should implement the ability to do

inputElement.files = [blob1, blob2];

That should be pretty easy to implement once bug 825196 is fixed so that we can use webidl. Using WebIDL would also mean that passing a FileList would JustWork(tm) if I understand things correctly.

Maybe something Andrea is interested in taking on since he's been doing a lot of work on HTMLInputElement lately.

Comment 27

[Mounir Lamouri (:mounir)]

Comment on attachment 629233 [details] [diff] [review]
patch

Marking sr- to make clear the patch is fine but we want to hold the landing for the moment.

Comment 28

[Mounir Lamouri (:mounir)]

So Jonas, if I understand correctly, a way to fix that is to add a Constructor to FileList that takes an array of File and make input.fileList r/w. Is that correct?

I agree that we could probably make this happen after bug 825196 anyway.

Comment 29

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

No, that wouldn't be enough to let the author do:

myInputElement.files = [blob1, blob2];

Comment 30

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

I don't actually know what types we should use here, and what operations we should permit. I think we do want to allow the following:

element.files = otherElement.files;
element.files = [blob1, blob2];
element.files = [blob1, file1];

We technically could also permit:

element.files.push(blob1);
element.files.splice(0, 1);

But what's tricky is that if we permit the last two, those should update the UI. This means that we couldn't use a plain old js Array.

I don't know what mechanisms WebIDL provides here that would help us. Sounds like a question for bz, Anne, Cameron and public-script-coord.

Comment 31

[Mounir Lamouri (:mounir)]

Anne, could you give your feedback on comment 30?

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Sounds like a WebIDL Array (Blob[])?

Comment 33

[Anne (:annevk)]

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

Comment 34

[Anne (:annevk)]

So what I think the conclusion will be is that the setter takes a sequence<Blob> (with special filename extracting semantics for File) and getter always returns a FileList. If we can implement that today without IDL that'd be a good way forward.

Comment 35

[Mounir Lamouri (:mounir)]

(In reply to Anne (:annevk) from comment #34)
> So what I think the conclusion will be is that the setter takes a
> sequence<Blob> (with special filename extracting semantics for File) and
> getter always returns a FileList. If we can implement that today without IDL
> that'd be a good way forward.

According to the WebIDL specification, we MUST NOT use sequence<> for an attribute. Does that mean we shouldn't use sequence<> for input.files?

Comment 36

[Anne (:annevk)]

Well, what we're really doing here is treat the setting and getting as completely different from an IDL perspective. As if you had `FileList setFiles(sequence<Blob>)` and `FileList getFiles()`. In that case it seems fine to use sequence.

Comment 37

[Boris Zbarsky [:bzbarsky]]

I think we should fix (or extend) WebIDL to allow declaring different types for the getter and setter of an attribute.  This has come up numerous times now, and there's no reason I can see for not allowing it.

Comment 38

[Mounir Lamouri (:mounir)]

I guess we will be able to fix this bug when bug 862800 will be fixed.

Comment 39

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #37)
> I think we should fix (or extend) WebIDL to allow declaring different types
> for the getter and setter of an attribute.  This has come up numerous times
> now, and there's no reason I can see for not allowing it.

I think that is the right approach here.  Olli didn't seem to like it in https://www.w3.org/Bugs/Public/show_bug.cgi?id=20407, but I think it makes enough sense.

Comment 40

[Boris Zbarsky [:bzbarsky]]

I posted http://lists.w3.org/Archives/Public/public-script-coord/2013AprJun/0153.html just to make sure people don't have objections.

Comment 41

[Hixie (not reading bugmail)]

Looks like WebKit implemented this: https://bugs.webkit.org/show_bug.cgi?id=87154

Is there an intent here to match WebKit exactly, or to do something similar, or something else?

Comment 42

[Boris Zbarsky [:bzbarsky]]

When we floated this idea (see comment 40), people thought this was a terrible API match for JS, so at the moment there are no plans to do anything here, I believe.

Comment 43

[Boris Zbarsky [:bzbarsky]]

More precisely, taking random non-FileList objects was considered bad API.

Comment 44

[Boris Zbarsky [:bzbarsky]]

So apparently WebKit only allows actual FileList objects, and actually changes the filelist pointer in the file input to the new object.  See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22682#c6

Comment 45

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

Could we please simply implement a setter which takes any array-like object and then take all File or Blob objects in the array-like and construct a new FileList containing only those.

I.e. I'd really like all of:

input.files = otherInput.files;
input.files = dropEvent.files;
input.files = [file1, file2];

to work.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> Could we please simply implement a setter which takes any array-like object 

See comment 43?

Comment 47

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

I don't see that the webkit approach could be better than my proposal though. Do you have links to the objections?

Comment 48

[Boris Zbarsky [:bzbarsky]]

See the link in comment 40 and the following thread.

Comment 49

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

Based on that thread I'm going to stand by my proposal.

The only real counterproposal was to introduce more array types, i.e. |new FileList([...])| which has been equally frowned upon elsewhere.

I just sent this proposal to the list:

myFileInput.files returns a simple raw JS Array
This array can be modified using normal JS Array functions, like any other Array can.
myFileInput.files is settable to any other raw JS Array, i.e.
myFileInput.files = ["a", 2, myFile3] works.
x = [...]; myFileInput.files = x; myFileInput.files === x; always returns true
Submitting ignores any non-Files in the Array

When exactly the UI is updated is somewhat tricky. We can likely use
Object.observe which would lead to updating at end-of-task I think.

Comment 50

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

http://lists.w3.org/Archives/Public/public-script-coord/2013OctDec/0083.html

Comment 51

[Victor Costan]

Have you decided on how to make the files attribute writable?

I'm asking because, according to the comment below, it seems that the File API spec will become RC soon, so the window of opportunity for adding a FileList constructor is going to close soon.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23853#c18

I'm not advocating for (or against) a FileList constructor. I'm bringing this up because I would like a method for changing the files attribute.

Thank you!

Comment 52

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

My current proposal is here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682

Comment 53

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

Either way this should not affect the FileAPI spec.

Comment 54

[gotennn]

So it's been 9-10 months since last comment and nothing has been done, although a working path has been submitted here. The same functionality, as thakis suggested, would work for me and make Firefox behave same way as Chrome or Safari.

Comment 56

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

My recommendation, based on the discussion in [1] is to implement the following here:

Make our current DOMFileList implementation implement the JS-iterable protocol (this might already be the case?)

When the .files setter is run, treat the passed in value as an JS-iterable value. If it's not an iterable value, throw a TypeError exception.

Iterate the list and try to convert each value to a DOM Blob. If any value is not a DOM Blob, throw an TypeError exception.

Create a new DOMFileList, and populate it with the Blobs.

Make the getter return the newly created DOMFileList.

The official "decision" hasn't been made in [1] yet. But it definitely feels like this is the direction it's heading. The main unknown is if we'll keep returning a DOMFileList, or if we can return a frozen array, from the getter. But that's something we can worry about separately I think.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682

Comment 57

[Boris Zbarsky [:bzbarsky]]

> this might already be the case?

Yes, since it has an indexed getter we stick an iterator on it already.

> When the .files setter is run, treat the passed in value as an JS-iterable value.

OK.  So basically, we want to either use the "any" type or add support for having attributes whose get and set types are different, right?

If we move to returning a frozen array, things are a bit simpler impl-wise, because then we can just make this a sequence type for both get and set.  So if we think we'll end up there it might be better to just use "any" for now and suck up the pain of doing manual iteration in C++...

Comment 58

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

(In reply to Not doing reviews right now from comment #57)
> OK.  So basically, we want to either use the "any" type or add support for
> having attributes whose get and set types are different, right?

Yes.

> If we move to returning a frozen array, things are a bit simpler impl-wise,
> because then we can just make this a sequence type for both get and set.

I would think so yes. That happening is probably mainly blocked on getting [1] fixed in WebIDL. I don't know what the timeframe on that is.

Alternatively we could just try to return a frozen array right away, and see what feedback we get. That might help move [1] along. We can always switch back to returning a DOMFileList if the feedback is too negative.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682

Comment 59

[Boris Zbarsky [:bzbarsky]]

> That happening is probably mainly blocked on getting [1] fixed in WebIDL

Getting what part of it fixed, exactly?

> Alternatively we could just try to return a frozen array right away

If we think we can do that without an obvious backwards compat problem, that seems like the way to go to me, in terms of simplicity of implementation on our end.

Comment 60

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

(In reply to Not doing reviews right now from comment #59)
> > That happening is probably mainly blocked on getting [1] fixed in WebIDL
> 
> Getting what part of it fixed, exactly?

Checking in a patch to the WebIDL spec which changes it to define that "attribute sequence<X> attrName" creates a getter which returns a frozen Array and a setter which accepts an iterable. And getting at least one other browser vendor to say that this looks good.

> > Alternatively we could just try to return a frozen array right away
> 
> If we think we can do that without an obvious backwards compat problem, that
> seems like the way to go to me, in terms of simplicity of implementation on
> our end.

Clearly there are theoretical problems with this. Someone might modify the DOMFileList prototype and add functions, and then expect myinputElement.files to have those functions.

But I suspect this is very rare. Other than possibly adding functions from the Array prototype, which would be fine.

Even more theoretical would be someone depending on myinputElement.files *not* having things from the Array prototype on it. But this seems even more theoretical.

I think chances are good enough that it's worth trying.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> Checking in a patch to the WebIDL spec which changes it to define that "attribute
> sequence<X> attrName" creates a getter which returns a frozen Array and a setter which
> accepts an iterable.

To be clear, this is a convenience; this behavior can already be defined in prose, yes?

> Clearly there are theoretical problems with this.

The main question is non-theoretical: is someone using .item() on FileLists?

Comment 62

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

(In reply to Not doing reviews right now from comment #61)
> > Checking in a patch to the WebIDL spec which changes it to define that "attribute
> > sequence<X> attrName" creates a getter which returns a frozen Array and a setter which
> > accepts an iterable.
> 
> To be clear, this is a convenience; this behavior can already be defined in
> prose, yes?

Yes. The purpose of doing the WebIDL check is to see if browser vendors think that the pattern is a good pattern.

> > Clearly there are theoretical problems with this.
> 
> The main question is non-theoretical: is someone using .item() on FileLists?

Doh! Right. I forgot about that. I guess that's something we could actually get telemetry on.

A simple search did turn up a few cases on stackoverflow though, which is concerning.

So I guess we'd need to use a subclass of Array which has a .item function. But that seems to make this more complicated. So maybe the right thing to do is to just use 'attribute any' for now.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move CreateNewFileInstance over into File code so it can be used from other callsites

Comment 64

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Allow assigning an iterable to HTMLInputElement.files and have it replace the DOMFileList it's holding with a new one

Jonas, three things I want feedback on:

1) The behavior when null is passed.
2) The behavior when assigning on a non-file input.
3) The behavior when upgrading blobs, and in particular how that part differs from what MozSetFileNameArray does.  See also bug 1146116.

Comment 65

[Andrea Marchesini [:baku]]

Comment on attachment 8590993 [details] [diff] [review]
part 1.  Move CreateNewFileInstance over into File code so it can be used from other callsites

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

::: dom/base/File.cpp
@@ +344,5 @@
> +  nsRefPtr<MultipartFileImpl> impl =
> +    new MultipartFileImpl(blobImpls, filename, contentType);
> +
> +  nsRefPtr<File> file = new File(GetParentObject(), impl);
> +  return file.forget();  

extra space.

Comment 66

[Andrea Marchesini [:baku]]

Comment on attachment 8590994 [details] [diff] [review]
part 2.  Allow assigning an iterable to HTMLInputElement.files and have it replace the DOMFileList it's holding with a new one

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

lgtm. Have you checked if we have some web-platform-tests about HTMLInputElement.files getters/setters?

::: dom/html/HTMLInputElement.cpp
@@ +2780,5 @@
> +    }
> +
> +    nsRefPtr<File> blob;
> +    nsresult rv =
> +      UnwrapObject<prototypes::id::Blob, mozilla::dom::File>(&temp.toObject(),

Any reason why you don't use UNWRAP_OBJECT(Blob, &tmp.toObject(), blob)?

Comment 67

[Boris Zbarsky [:bzbarsky]]

> Any reason why you don't use UNWRAP_OBJECT(Blob, &tmp.toObject(), blob)?

Er, no.  Mostly I copy/pasted that part from codegen for a sequence-typed attribute.  ;)  Fixed.

Comment 68

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

Comment on attachment 8590994 [details] [diff] [review]
part 2.  Allow assigning an iterable to HTMLInputElement.files and have it replace the DOMFileList it's holding with a new one

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

Is this just stalled on me? It definitely looks good to me.

Comment 69

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

> 1) The behavior when null is passed.

I think the cleanest thing to do would be to clear the list. IIRC we return null when the <input> doesn't contain any files.

> 2) The behavior when assigning on a non-file input.

Probably throw.

> 3) The behavior when upgrading blobs, and in particular how that part
> differs from what MozSetFileNameArray does.  See also bug 1146116.

I don't know how bug 1146116 will turn out. But I hope really really hard to not have to upgrade Blobs to Files here. I guess it's a module owner decision, but I hope I made my opinion clear.

Comment 70

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

Did you mean bug 1162658 by any chance?

Comment 71

[Boris Zbarsky [:bzbarsky]]

> Is this just stalled on me?

It's mostly stalled on sorting out the blob-upgrading bits.

> I think the cleanest thing to do would be to clear the list.

Great.  That's what I'm doing.

> Probably throw.

Can do.

> Did you mean bug 1162658 by any chance?

No, I meant bug 1146116.  That's what introduced the current MozSetFileNameArray behavior.  It's mostly relevant in that it's an existing "upgrade to File" path we have here, except it always clones the File, since it's assuming it's called from chrome.  And the real question is what setting .files from chrome should do....

Comment 72

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

Honestly I don't care much about what happens in the case when chrome calls the .files setter. Though of course if it's called on a chrome <input> we should just run the setter as normal.

But other than that, an ideal solution might be that if the .files setter is called when the caller is chrome code, and the 'this' object is a content <input> element then:

* For any Blob/File object which is *not* same-origin with the <input> element, create a new Blob/File with the same contents and metadata (name/lastModified) which belongs to the same origin as the <input>.
* For any Blob/File object which *is* same-origin with the <input> element, just use the passed in object.

But it all depends on how much complexity we want to deal with. It might be fine to "do nothing" which I think means that if the page could access the .files property, but accessing .files[n].size might throw an exception. And then make it the responsibility of the chrome code to make sure to create an object in the right origin first.

What do we do for Element.appendChild? Does it throw if chrome tries to insert a chrome node into a content document?

Comment 73

[Boris Zbarsky [:bzbarsky]]

> but accessing .files[n].size might throw an exception

Right, and that causes problems.  See bug 1146116.

> What do we do for Element.appendChild?

We adopt the argument into the content document.  Then it behaves like any other content node.

Comment 74

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

(In reply to Not doing reviews right now from comment #73)
> > but accessing .files[n].size might throw an exception
> 
> Right, and that causes problems.  See bug 1146116.

One could argue that that's a bug in the chrome/addon code calling the .files setter. I.e. that that code should call MozSetFileArray, which takes care of this problem, or create new File/Blob objects in the correct scope.

Or we could make chrome code calling the .files setter on a content <input> always throw.

Or we could do something more complex as mentioned in comment 72.

I'm really fine either way here.

> > What do we do for Element.appendChild?
> 
> We adopt the argument into the content document.  Then it behaves like any
> other content node.

Makes sense. Sadly not something we can do here I'd think.

Comment 75

[Boris Zbarsky [:bzbarsky]]

I think we should get rid of MozSetFileArray once this setter exists, fwiw.

Comment 76

[Jed Davis [:jld] (on leave until 08-31)]

(In reply to Not doing reviews right now from comment #75)
> I think we should get rid of MozSetFileArray once this setter exists, fwiw.

Fine by me, for what it's worth; I never entirely liked using a mozPrefixed method for that, but I didn't have any better ideas.  I assume we have ways of dealing with any add-ons that already started using it.

Comment 77

[Boris Zbarsky [:bzbarsky]]

Not going to get to this...

That said, at this point this looks like a duplicate of bug 1384030. This bug was trying for something more ambitious, but the thing people really needed ended up happening htere.

Comment 78

[thakis]

I can't help but point out that the patch that landed in bug 1384030 3 years is identical to my patch on this bug here (https://bugzilla.mozilla.org/attachment.cgi?id=628895&action=edit) except that my patch had tests and was written 8 years ago.

It's cool y'all wanted to do something more ambitious, but the person who filed this bug and wrote the patch (me) didn't.

Comment 79

[Boris Zbarsky [:bzbarsky]]

It's not quite identical, but yes. The perfect was the enemy of the good once again.