Patch in a bit.

This refactors the input element to store an array of nsIDOMFiles and provides an implementation of a "fake" DOMFile that has no backing nsIFile (and no name). We could do a lot of different things here. We could make nsDOMFakeFile just the File API's blob (the FakeFile basically is a file with no name and no backing nsIFile). We also have a lot of different tests that rely on setting a file input element's value to some nonexistent file which this breaks (since we won't be able to create an nsIFile for it).

(In reply to comment #1) > Created attachment 462190 [details] [diff] [review] > Kill nsIFileControlElement Perhaps add nsHTMLInputElement::FromContent, like at <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLOptionElement.h#57>.

Comment on attachment 462190 [details] [diff] [review] Kill nsIFileControlElement >@@ -275,24 +275,24 @@ nsFileControlFrame::CreateAnonymousConte > // Mark the element to be native anonymous before setting any attributes. > mTextContent->SetNativeAnonymous(); > > mTextContent->SetAttr(kNameSpaceID_None, nsGkAtoms::type, > NS_LITERAL_STRING("text"), PR_FALSE); > > nsCOMPtr<nsIDOMHTMLInputElement> textControl = do_QueryInterface(mTextContent); > if (textControl) { >- nsCOMPtr<nsIFileControlElement> fileControl = do_QueryInterface(mContent); >- if (fileControl) { >- // Initialize value when we create the content in case the value was set >- // before we got here >- nsAutoString value; >- fileControl->GetDisplayFileName(value); >- textControl->SetValue(value); >- } >+ nsRefPtr<nsHTMLInputElement> fileControl = >+ static_cast<nsHTMLInputElement*>(textControl.get()); >+ >+ // Initialize value when we create the content in case the value was set >+ // before we got here >+ nsAutoString value; >+ fileControl->GetDisplayFileName(value); >+ textControl->SetValue(value); You probably want to keep *some* type of check that mContent is the right type here. Generally what we do is check that IsHTML() returns true and Tag() returns nsGkAtoms::input. This is what we do elsewhere before casting to specific element subclasses. You can then cast mContent directly. Should be safer and faster. If you want, you can also remove the QI on mTextContent and cast that directly to nsHTMLInputElement since we just created it and thus know that it's of the right type. >@@ -437,17 +437,19 @@ nsFileControlFrame::CaptureMouseListener > > NS_ASSERTION(mFrame, "We should have been unregistered"); > if (!ShouldProcessMouseClick(aMouseEvent)) > return NS_OK; > > // Get parent nsIDOMWindowInternal object. > nsIContent* content = mFrame->GetContent(); > nsCOMPtr<nsIDOMNSHTMLInputElement> inputElem = do_QueryInterface(content); >- nsCOMPtr<nsIFileControlElement> fileControl = do_QueryInterface(content); >+ nsRefPtr<nsHTMLInputElement> fileControl = >+ static_cast<nsHTMLInputElement*>(inputElem.get()); >+ > if (!content || !inputElem || !fileControl) > return NS_ERROR_FAILURE; Same here, you should add a check to make sure that the cast is safe. Check content->IsHTML and content->Tag. After null-checking content of course. >@@ -534,17 +536,19 @@ nsFileControlFrame::BrowseMouseListener: > > NS_ASSERTION(mFrame, "We should have been unregistered"); > if (!ShouldProcessMouseClick(aMouseEvent)) > return NS_OK; > > // Get parent nsIDOMWindowInternal object. > nsIContent* content = mFrame->GetContent(); > nsCOMPtr<nsIDOMNSHTMLInputElement> inputElem = do_QueryInterface(content); >- nsCOMPtr<nsIFileControlElement> fileControl = do_QueryInterface(content); >+ nsRefPtr<nsHTMLInputElement> fileControl = >+ static_cast<nsHTMLInputElement*>(inputElem.get()); >+ > if (!content || !inputElem || !fileControl) > return NS_ERROR_FAILURE; And here > nsFileControlFrame::GetFormProperty(nsIAtom* aName, nsAString& aValue) const > { > aValue.Truncate(); // initialize out param > > if (nsGkAtoms::value == aName) { >- nsCOMPtr<nsIFileControlElement> fileControl = >+ nsCOMPtr<nsIDOMHTMLInputElement> inputElem = > do_QueryInterface(mContent); >+ nsRefPtr<nsHTMLInputElement> fileControl = >+ static_cast<nsHTMLInputElement*>(inputElem.get()); >+ And preferably use IsHTML/Tag checks here too. Hmm.. feel free to add a function like: IsHTML(nsIAtom* aTag) { return IsHTML() && Tag() == aTag; } to nsIContent. You could also add a static nsHTMLInputElement* FromContent(nsIContent* aContent) { return aContent && aContent->IsHTML(nsGkAtoms::input)) ? static_cast<nsHTMLInputElement*>(aContent) : nsnull; } to nsHTMLInputElement. We have similar functions in other element classes. r=me with that fixed. For what it's worth, I don't think this needs sr, but feel free to request one if you want.

Comment on attachment 462192 [details] [diff] [review] Refactor Will this work if you attempt to read from the fake-file twice at the same time? It doesn't appear to. I.e. if you do: xhr1 = new XMLHttpRequest(); xhr1.open("POST", url1); xhr1.send(myfile); xhr2 = new XMLHttpRequest(); xhr2.open("POST", url2); xhr2.send(myfile); or img1.src = myfile.url; img2.src = myfile.url; where myfile is a fake-file. In nsXMLHttpRequest.cpp, instead of modifying the nsIDOMFile-specific code, make nsDOMFile and nsDOMFakeFile implement nsIXHRSendable and remove the file-specific code from nsXMLHttpRequest.cpp minusing based on that it looks like multiple reads will fail. Please rerequest if I'm misunderestimating the code :)

(In reply to comment #5) > minusing based on that it looks like multiple reads will fail. Please rerequest > if I'm misunderestimating the code :) This passes the buck to the callback function. That should probably be documented ;-)

That is, the callback receives an XPCOM object (the nsISupports*) and has to provide an input stream from that. Thus the callback has to handle the "hard" part of this.

Comment on attachment 462192 [details] [diff] [review] Refactor I talked to Kyle and I now see how this is intended to work. However since we, at least for now, only need memory backed files, we should just rename nsDOMFakeFile to nsDOMMemoryFile or some such, and put all the logic in that class. No need for callbacks and such yet.

Comment on attachment 462192 [details] [diff] [review] Refactor in nsDOMFileReader, you should not use the GetURL method and load from that url. Calling GetURL has some side effects since we have to keep the file alive for the lifetime of the calling page. One solution would be to extract a channel directly and call AsyncOpen on that. But I'm not sure if we have memory-backed channels :(

(In reply to comment #9) > One solution would be to extract a channel directly and call AsyncOpen on that. > But I'm not sure if we have memory-backed channels :( After chatting with sicking, the better solution is to register a separate moz-filedata uri for internal use and release it when we're done.

This adds nsHTMLInputElement::FromContent and uses that to do the checking. Carrying forward r+.

Comments about simplification and GetUrl addressed.

Is this supposed to be a blocker? I'm not sure it's worth approving these changes now unless they are supposed to block.

We're going to need this for the fennec camera work.

Missed the suggestion to implement nsIXHRSendable.

I just copied and pasted the relevant hunks, getting the earlier version to apply again (or finding a rev it applied to) didn't seem worth it.

Comment on attachment 463232 [details] [diff] [review] Patch V2.1 Actually, looking at this a bit more closely, I lost a piece somewhere along the way.

I'd lost the bit about not using GetUrl directly in the FileReader. That's in here now.

Comment on attachment 464499 [details] [diff] [review] Patch V2.2 We might want to give the camera "files" some reasonable name. Such as "generated.jpg" or some such. > nsDOMFile::GetAsDataURL(nsAString &aResult) > { > aResult.AssignLiteral("data:"); > > nsresult rv; >- nsCOMPtr<nsIMIMEService> mimeService = >- do_GetService(NS_MIMESERVICE_CONTRACTID, &rv); >- NS_ENSURE_SUCCESS(rv, rv); >+ if (!mContentType.Length()) { >+ nsCOMPtr<nsIMIMEService> mimeService = >+ do_GetService(NS_MIMESERVICE_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); > >- nsCAutoString contentType; >- rv = mimeService->GetTypeFromFile(mFile, contentType); >- if (NS_SUCCEEDED(rv)) { >- AppendUTF8toUTF16(contentType, aResult); >+ nsCAutoString contentType; >+ rv = mimeService->GetTypeFromFile(mFile, contentType); >+ if (NS_SUCCEEDED(rv)) { >+ AppendUTF8toUTF16(contentType, aResult); >+ } else { >+ aResult.AppendLiteral("application/octet-stream"); >+ } > } else { >- aResult.AppendLiteral("application/octet-stream"); >+ aResult.Append(mContentType); > } Might be worth assigning the resulting type to mContentType here. I.e. do something like: if (mContentType.IsEmpty()) { ... assign to mContentType ... } aResult.Append(mContentType); Or even better, can you call GetType(some_temporary_string); and have GetType cache in mContentType? >+nsDOMFile::GetSendInfo(nsIInputStream** aBody, >+ nsACString& aContentType, >+ nsACString& aCharset) >+{ >+ nsresult rv; >+ >+ nsCOMPtr<nsIInputStream> stream; >+ rv = this->GetInternalStream(getter_AddRefs(stream)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = this->GetType(aContentType); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = this->GuessCharset(stream, aCharset); >+ NS_ENSURE_SUCCESS(rv, rv); Don't set a charset at all. Better let the server do the guessing, especially since many times it'll be a binary file without charset at all. >diff --git a/content/base/src/nsFileDataProtocolHandler.cpp b/content/base/src/nsFileDataProtocolHandler.cpp >-nsFileDataProtocolHandler::AddFileDataEntry(nsACString& aUri, nsIFile* aFile, >+nsFileDataProtocolHandler::AddFileDataEntry(nsACString& aUri, >+ nsIDOMFile* aFile, > nsIPrincipal* aPrincipal) Fix tabs. >diff --git a/content/base/src/nsFileDataProtocolHandler.h b/content/base/src/nsFileDataProtocolHandler.h >@@ -57,7 +57,8 @@ public: > virtual ~nsFileDataProtocolHandler() {} > > // Methods for managing uri->file mapping >- static void AddFileDataEntry(nsACString& aUri, nsIFile* aFile, >+ static void AddFileDataEntry(nsACString& aUri, >+ nsIDOMFile* aFile, Fix tabs. (You might want to check the whole diff for tabs). >diff --git a/content/base/test/test_bug345339.html b/content/base/test/test_bug345339.html >--- a/content/base/test/test_bug345339.html >+++ b/content/base/test/test_bug345339.html >@@ -32,7 +32,7 @@ function afterLoad() { > iframeDoc.getElementById("hidden").value = "gecko"; > > netscape.security.PrivilegeManager.enablePrivilege("UniversalFileRead"); >- iframeDoc.getElementById("file").value = "dummyfile.txt"; >+// iframeDoc.getElementById("file").value = "test_bug345339.html"; > > /* Reload the page */ > $("testframe").setAttribute("onload", "afterReload()"); >@@ -55,8 +55,8 @@ function afterReload() { > is(iframeDoc.getElementById("hidden").value, "gecko", > "hidden field value preserved"); > netscape.security.PrivilegeManager.enablePrivilege("UniversalFileRead"); >- is(iframeDoc.getElementById("file").value, "dummyfile.txt", >- "file field value preserved"); >+// is(iframeDoc.getElementById("file").value, "test_bug345339.txt", >+// "file field value preserved"); Why these changes? >diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp >--- a/content/html/content/src/nsHTMLInputElement.cpp >+++ b/content/html/content/src/nsHTMLInputElement.cpp >@@ -183,12 +183,14 @@ class nsHTMLInputElementState : public n > mValue = aValue; > } > >- const nsTArray<nsString>& GetFilenames() { >- return mFilenames; >+ void GetFiles(nsCOMArray<nsIDOMFile> aFiles) { >+ aFiles.Clear(); >+ aFiles.AppendObjects(mFiles); > } Why not just return a reference to mFiles, like the old function does? >@@ -697,12 +701,38 @@ nsHTMLInputElement::MozSetFileNameArray( > return NS_ERROR_DOM_SECURITY_ERR; > } > >- nsTArray<nsString> fileNames(aLength); >+ nsCOMArray<nsIDOMFile> files; > for (PRUint32 i = 0; i < aLength; ++i) { >- fileNames.AppendElement(aFileNames[i]); >+ nsCOMPtr<nsIFile> file; >+ // XXXkhuey this should be an ASCII only case folding. >+ if (StringBeginsWith(nsDependentString(aFileNames[i]), >+ NS_LITERAL_STRING("file:"), >+ nsCaseInsensitiveStringComparator())) { >+ // Converts the URL string into the corresponding nsIFile if possible >+ // A local file will be created if the URL string begins with file:// >+ NS_GetFileFromURLSpec(NS_ConvertUTF16toUTF8(aFileNames[i]), >+ getter_AddRefs(file)); >+ } >+ >+ if (!file) { >+ // this is no "file://", try as local file >+ nsCOMPtr<nsILocalFile> localFile; >+ NS_NewLocalFile(nsDependentString(aFileNames[i]), >+ PR_FALSE, getter_AddRefs(localFile)); >+ file = do_QueryInterface(localFile); >+ } >+ >+ if (file) { >+ nsCOMPtr<nsIDOMFile> domFile = >+ do_QueryObject(new nsDOMFile(file, GetOwnerDoc())); >+ files.AppendObject(domFile); No need for do_QueryObject here. nsCOMPtr<nsIDOMFile> domFile = new nsDOMFile... should work fine. >@@ -724,7 +754,8 @@ nsHTMLInputElement::SetUserInput(const n > > if (mType == NS_FORM_INPUT_FILE) > { >- SetSingleFileName(aValue); >+ return NS_ERROR_NOT_IMPLEMENTED; >+ //SetSingleFile(aValue); > } else { > SetValueInternal(aValue, PR_TRUE, PR_TRUE); > } This doesn't seem finished. Looking very good in general. Please attach an interdiff for next iteration though. I ended up just reviewing the whole thing here since I wasn't sure if the interdiff corresponded to the latest patch version or not.

(In reply to comment #19) > Comment on attachment 464499 [details] [diff] [review] > Patch V2.2 > > We might want to give the camera "files" some reasonable name. Such as > "generated.jpg" or some such. Yeah, we probably do. > >diff --git a/content/base/test/test_bug345339.html b/content/base/test/test_bug345339.html > >--- a/content/base/test/test_bug345339.html > >+++ b/content/base/test/test_bug345339.html > >@@ -32,7 +32,7 @@ function afterLoad() { > > iframeDoc.getElementById("hidden").value = "gecko"; > > > > netscape.security.PrivilegeManager.enablePrivilege("UniversalFileRead"); > >- iframeDoc.getElementById("file").value = "dummyfile.txt"; > >+// iframeDoc.getElementById("file").value = "test_bug345339.html"; > > > > /* Reload the page */ > > $("testframe").setAttribute("onload", "afterReload()"); > >@@ -55,8 +55,8 @@ function afterReload() { > > is(iframeDoc.getElementById("hidden").value, "gecko", > > "hidden field value preserved"); > > netscape.security.PrivilegeManager.enablePrivilege("UniversalFileRead"); > >- is(iframeDoc.getElementById("file").value, "dummyfile.txt", > >- "file field value preserved"); > >+// is(iframeDoc.getElementById("file").value, "test_bug345339.txt", > >+// "file field value preserved"); > > Why these changes? So the issue here is that in nsHTMLInputElement::MozSetFileNameArray (which is what everything else calls eventually) trying to create a nonexistent file will throw (because we're not going to be able to create any sort of DOMFile for it to store internally. Idk what we want to do here (probably should just change the tests that do this to only do files that exist.)

Still unresolved issues are naming the file and what to do about tests that set .value to a nonexistent file.

(In reply to comment #20) > > Why these changes? > > So the issue here is that in nsHTMLInputElement::MozSetFileNameArray (which is > what everything else calls eventually) trying to create a nonexistent file will > throw (because we're not going to be able to create any sort of DOMFile for it > to store internally. Idk what we want to do here (probably should just change > the tests that do this to only do files that exist.) Yeah, I think we should change the tests to only use existing files. However I also think we shouldn't make MozSetFileNameArray or setting .value throw if there are non-existing filenames. Just ignore those names instead.

Comment on attachment 464815 [details] [diff] [review] Interdiff V2.2 - V2.3 You're doing nsCOMArray<nsIDOMFile> files = GetFiles(); a lot. However this copies the contents of the array, in at least many cases unnecessarily. Just do nsCOMArray<nsIDOMFile>& files = GetFiles(); To be on the safe side you could even make GetFiles return a const nsCOMArray& and add |const| to callers accordingly. r=me with that and fixing the tests as discussed in previous comment.

(In reply to comment #23) > Comment on attachment 464815 [details] [diff] [review] > Interdiff V2.2 - V2.3 > > You're doing > > nsCOMArray<nsIDOMFile> files = GetFiles(); > > a lot. However this copies the contents of the array, in at least many cases > unnecessarily. Just do > > nsCOMArray<nsIDOMFile>& files = GetFiles(); Mmm good point.

Rebased to tip, carrying forward r+

So an issue I ran into when fixing the last of the test failures is that with this patch the file names are no longer rendered. This is because layout calls GetDisplayFilename which in turn calls GetMozFullPath on all of the DOMFiles. GetMozFullPath returns nothing though because apparently layout doesn't have UniversalFileRead capability. I'm not sure how to proceed here.

This passes all tests except as noted in the previous comment.

Can you cast the files to nsDOMFile and call some internal method there that doesn't perform the security check? We'd have to make sure never to stick anything that isn't an nsDOMFile into the array in nsHTMLInputElement. In fact, maybe it'd be better to make nsHTMLInputElement hold an nsTArray<nsDOMFile> rather than nsCOMArray<nsIDOMFile>.

Yeah, we can do that. I'll work up another patch on top of the above.

This adds another method to get the internal file name without security checks. We could add this to the base object (nsDOMFile) rather than nsIDOMFile but that seems messier to me.

Comment on attachment 469132 [details] [diff] [review] Add another method to let layout in the side door. I still think it would be cleaner to convert the internal code to use nsDOMFiles instead of putting [noscript] attributes on the interface. DeCOMtamination FTW! But I'm totally fine with this too.

So, that fixes layout, but not print preview. When the static doc gets created for print preview there are no files in it. I tried this but it didn't work, when GetDisplayFileName gets called on the static document there is nothing there, which makes no sense to me.

So the problem ended up being that the first patch botches the deCOMtamination and asks the anonymous text control for the filename.

Last attachment is as pushed. http://hg.mozilla.org/mozilla-central/rev/af1365b24066

We should document for extension authors that <input type="file"> will ignore anything of the form document.getElementById("random input type=file").value="file that doesn't exist"). This isn't relevant to webpages because we don't let them set the value to anything interesting anyways.

Backed out due to leaks.

The patch leaks when creation of a channel fails (the file doesn't exist). Easy enough to fix, and glad that our test suite catches it. Repushed: http://hg.mozilla.org/mozilla-central/rev/8d846fde08cb

Comment on attachment 472265 [details] [diff] [review] : Refactor <input> implementation to deal with files that are not on the disk. a=blocking-fennec >+ var file = Components.classes["@mozilla.org/file/directory_service;1"] >+ .getService(Components.interfaces.nsIProperties) >+ .get("TmpD", Components.interfaces.nsILocalFile); >+ file.append("346337_test1.file"); >+ file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666); >+ filePath1 = file.path; >+ file = Components.classes["@mozilla.org/file/directory_service;1"] >+ .getService(Components.interfaces.nsIProperties) >+ .get("TmpD", Components.interfaces.nsILocalFile); >+ file.append("346337_test2.file"); >+ file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666); You never clean these files up do you?

(In reply to comment #40) > Comment on attachment 472265 [details] [diff] [review] > : Refactor <input> implementation to deal with files that are not on the disk. > a=blocking-fennec > > >+ var file = Components.classes["@mozilla.org/file/directory_service;1"] > >+ .getService(Components.interfaces.nsIProperties) > >+ .get("TmpD", Components.interfaces.nsILocalFile); > >+ file.append("346337_test1.file"); > >+ file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666); > >+ filePath1 = file.path; > >+ file = Components.classes["@mozilla.org/file/directory_service;1"] > >+ .getService(Components.interfaces.nsIProperties) > >+ .get("TmpD", Components.interfaces.nsILocalFile); > >+ file.append("346337_test2.file"); > >+ file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666); > You never clean these files up do you? No, doesn't look like it. File a followup?

(In reply to comment #40) >(From update of attachment 472265 [details] [diff] [review]) >>+ file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666); >You never clean these files up do you? In fact the tests don't seem to require the files to exist.

(In reply to comment #42) > (In reply to comment #40) > >(From update of attachment 472265 [details] [diff] [review]) > >>+ file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666); > >You never clean these files up do you? > In fact the tests don't seem to require the files to exist. The DOM does. You can't do document.getElementById("random input type=file element").value = "File that doesn't exist" anymore. See comments 20 and 22.

This almost certainly caused bug 594964.

Added the note as indicated in comment #37.