Closed Bug 503598 Opened 15 years ago Closed 15 years ago

Support file handling with drag and drop

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: matinm1, Assigned: matinm1)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 9 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: 

Web developers need an easy means of accessing local files, outside of the traditional (and cumbersome) <input type="file"> method. The current plan for HTML5 is to expose a .files attribute on a dataTransfer object. This enables web sites to allow users to drag and drop multiple local files into a web page, with every file being picked up, assembled into an array, and accessible as an attribute of a dataTransfer object, all without needing to explicitly prompt the user for privilege changes.

Example:
function dodrop(event) //specified as the ondrop handler
{
  var dt = event.dataTransfer;
  var files = dt.files;
  //manipulate files as desired
  var fileName1 = files[0].fileName;
  var fileSize2 = files[1].fileSize;
}

Reproducible: Always
Assignee: nobody → mmovassate
Attachment #387982 - Flags: superreview?(jonas)
Attachment #387982 - Flags: review?
Attachment #387982 - Flags: review? → review?(enndeakin)
Comment on attachment 387982 [details] [diff] [review]
Proposed patch for file drag n' drop

Drive-by comments (not enough to warrant generating another patch) below:

> [scriptable, uuid(E4970BA1-9567-455C-8B4E-4607D7E741BB)]
> interface nsIDOMDataTransfer : nsISupports

Nit:  need to rev the uuid.  (You are changing the interface.)

Also, you're adding a lot of header files for this to nsDOMDataTransfer.cpp.  Do you need all of them?  (nsIEditor.h is one I don't see the need for.)
re: Alex's quibbles:

All fixed. Thanks for pointing these out!
Attachment #387982 - Attachment is obsolete: true
Attachment #387982 - Flags: superreview?(jonas)
Attachment #387982 - Flags: review?(enndeakin)
Attachment #387991 - Flags: superreview?(jonas)
Attachment #387991 - Flags: review?(enndeakin)
Attachment #387991 - Attachment is obsolete: true
Attachment #387992 - Flags: superreview?(jonas)
Attachment #387992 - Flags: review?(enndeakin)
Attachment #387991 - Flags: superreview?(jonas)
Attachment #387991 - Flags: review?(enndeakin)
Comment on attachment 387982 [details] [diff] [review]
Proposed patch for file drag n' drop

>diff --git a/content/events/src/nsDOMDataTransfer.cpp b/content/events/src/nsDOMDataTransfer.cpp
>+#include "nsCOMPtr.h"
>+
>+#include "nsIControllers.h"
>+#include "nsFocusManager.h"
>+#include "nsPIDOMWindow.h"
>+#include "nsContentCID.h"
>+#include "nsIComponentManager.h"
>+#include "nsIDOMHTMLFormElement.h"
>+#include "nsIDOMEventTarget.h"
>+#include "nsIFormControl.h"
>+#include "nsIEventStateManager.h"
>+#include "nsIScriptSecurityManager.h"
>+#include "nsIPrivateDOMEvent.h"
>+#include "nsIEditor.h"
>+
>+#include "nsIMIMEService.h"
>+#include "nsCExternalHandlerService.h"
>+#include "nsIFile.h"
>+#include "nsILocalFile.h"
>+#include "nsIFileStreams.h"
>+#include "nsNetUtil.h"
>+#include "nsDOMFile.h"

Are all these really needed?


>@@ -98,26 +121,28 @@ nsDOMDataTransfer::nsDOMDataTransfer(PRU
>   CacheExternalFormats();
> }
> 
> nsDOMDataTransfer::nsDOMDataTransfer(PRUint32 aEventType,
>                                      const PRUint32 aEffectAllowed,
>                                      PRBool aIsExternal,
>                                      PRBool aUserCancelled,
>                                      nsTArray<nsTArray<TransferItem> >& aItems,
>+				     nsDOMFileList* aFiles,

Fix whitespace, I think you have tabs in there.

>+nsDOMDataTransfer::PopulateFilesList()
>+{
>+  mFiles = new nsDOMFileList();

Actually, I think we should reuse the existing file-list here if one exists. So that in case someone has already grabbed a reference to the list that object still works. Also only really makes sense to populate the list if someone has asked for the .files list.

So I'd say let GetFiles() instantiate mFiles, and then call PopulateFilesList. And let PopulateFilesList bail early if mFiles is still null.

>+  PRUint32 count = mItems.Length();
>+  for (PRUint32 i = 0; i < count; i++) {
>+    nsCOMPtr<nsIVariant> ivariant;

Remove the 'i' from the variable name. Way too many things would start with 'i' if we prefixed everything holding onto an interface with 'i'.

>+    nsresult rv = MozGetDataAt(NS_ConvertUTF8toUTF16(kFileMime), i, getter_AddRefs(ivariant));
>+    NS_ENSURE_SUCCESS(rv, rv);

PopulateFilesList returns a PRBool and you're here returning an nsresult. Same thing in other places in this function.

>+    if (!ivariant)
>+      continue;

Is it expected that MozGetDataAt will return null here?

>+    nsCOMPtr<nsISupports> supports;
>+    rv = ivariant->GetAsISupports(getter_AddRefs(supports));
>+    NS_ENSURE_SUCCESS(rv, rv);

GetAsISupports will return an error value if for example a string is stored. In that case you just want to 'continue' rather than bailing completely.

You should probably make sure to test the case when the datatransfer contains both files and non-file data.

>+    if (!supports)
>+      continue;
>+
>+    nsCOMPtr<nsIFile> file = do_QueryInterface(supports);

You need to check that this QI succeeds. Something other than a file could be stored.

>diff --git a/content/events/src/nsDOMDataTransfer.h b/content/events/src/nsDOMDataTransfer.h
>@@ -42,16 +42,24 @@
> #include "nsTArray.h"
> #include "nsIVariant.h"
> #include "nsIPrincipal.h"
> #include "nsIDOMDataTransfer.h"
> #include "nsIDragService.h"
> #include "nsIDOMElement.h"
> #include "nsCycleCollectionParticipant.h"
> 
>+#include "nsIMIMEService.h"
>+#include "nsCExternalHandlerService.h"
>+#include "nsIFile.h"
>+#include "nsILocalFile.h"
>+#include "nsIFileStreams.h"
>+#include "nsNetUtil.h"
>+#include "nsDOMFile.h"

Are all of these really needed?

>@@ -92,16 +100,17 @@ protected:
> 
>   // this constructor is used only by the Clone method to copy the fields as
>   // needed to a new data transfer.
>   nsDOMDataTransfer(PRUint32 aEventType,
>                     const PRUint32 aEffectAllowed,
>                     PRBool aIsExternal,
>                     PRBool aUserCancelled,
>                     nsTArray<nsTArray<TransferItem> >& aItems,
>+		    nsDOMFileList* aFiles,

Fix whitespace. Don't use tabs.


>diff --git a/content/events/test/test_dragstart.html b/content/events/test/test_dragstart.html
>--- a/content/events/test/test_dragstart.html
>+++ b/content/events/test/test_dragstart.html
>@@ -12,16 +12,18 @@
>  
> <script>
> 
> SimpleTest.waitForExplicitFinish();
> 
> var gDragInfo;
> var gDataTransfer = null;
> var gExtraDragTests = 0;
>+var fileIndex = 0;
>+var testFileName = "pic";
> 
> function runTests()
> {
>   // first, create a selection and try dragging it
>   var draggable = $("draggable");
>   window.getSelection().selectAllChildren(draggable);
>   synthesizeMouse(draggable, 6, 6, { type: "mousedown" });
>   synthesizeMouse(draggable, 14, 14, { type: "mousemove" });
>@@ -385,16 +387,104 @@ function test_DataTransfer(dt)
>    "linkMove", "copyLink", "all", "uninitialized", "none"].forEach(
>     function (i) {
>       dt.effectAllowed = i;
>       is(dt.dropEffect, "move", "dropEffect not modified by effectAllowed set to " + i);
>       is(dt.effectAllowed, i == "" || i == "other" || i == "moveCopy" ? "link" : i,
>          "effectAllowed set to " + i);
>     }
>   );
>+
>+  //Test the .files attribute on data transfers
>+  try {
>+    is(dt.files.length, 0, ".files should have zero length on an empty datatransfer");
>+    is(dt.files[0], null, ".files should return null on index accesses");
>+
>+    dt.mozSetDataAt("text/plain", "Additional text alongside files", 0);
>+    is(dt.files.length, 0, ".files should have zero length on a datatransfer with no files");
>+    is(dt.files[0], null, ".files should return null on index accesses that don't contain files");
>+    dt.mozClearDataAt("text/plain", 0);
>+	
>+    [1,2,5,20,50].forEach(
>+	function (i) {

Don't use 'i' here as that's something usually used when iterating between two numbers. Call it numFiles or something.

>+    [1,2,5,20,50].forEach(
>+	function (i) {
>+	  //Test .files returns only files, and nothing else
>+
>+	  appendNFiles(dt, i);
>+	  dt.mozSetDataAt("text/plain", "Additional text alongside files", i);
>+	  is(dt.files.length, i, ".files has incorrect length when plain-text is present");
>+	  dt.mozClearDataAt("text/plain", i);
>+	  cleanUpAllFiles(dt);
>+
>+	  appendNFiles(dt, i);
>+	  dt.mozSetDataAt("text/html", "<em>Additional html text alongisde files</em>", i);
>+	  is(dt.files.length, i, ".files has incorrect length when HTML text is present");
>+	  dt.mozClearDataAt("text/html", i);
>+	  cleanUpAllFiles(dt);
>+
>+	  appendNFiles(dt, i);
>+	  dt.mozSetDataAt("text/uri-list", "http://www.mozilla.org/", i);
>+	  is(dt.files.length, i, ".files has incorrect length when URL text is present");
>+	  dt.mozClearDataAt("text/uri-list", i);
>+	  cleanUpAllFiles(dt);
>+	}

Try mixing file and non-file data. You're currently always appending the non-file data at the end. Looking at the code it doesn't appear that that will work as described above.

>+function appendNFiles(dt, num)
>+{
>+  while (fileIndex < num) {
>+    fileIndex++;
>+    var nsifile = Components.classes["@mozilla.org/file/local;1"]
>+			    .createInstance(Components.interfaces.nsILocalFile);
>+    nsifile.initWithPath(getCwdPath() + "/" + testFileName + fileIndex);
>+    nsifile.create(0, 0);
>+    dt.mozSetDataAt("application/x-moz-file", nsifile, fileIndex - 1);
>+  }
>+}
>+
>+function cleanUpAllFiles(dt)
>+{
>+  for (var i = 1; i <= fileIndex; i++) {
>+    var nsifile = Components.classes["@mozilla.org/file/local;1"]
>+			    .createInstance(Components.interfaces.nsILocalFile);
>+    nsifile.initWithPath(getCwdPath() + "/" + testFileName + i);
>+    nsifile.remove(false);
>+    dt.mozClearDataAt("application/x-moz-file", 0);
>+  }
>+  fileIndex = 0;
>+}
>+
>+function getCwdPath()
>+{
>+  return Components.classes["@mozilla.org/file/directory_service;1"].getService(Components.interfaces.nsIProperties).get("CurWorkD", Components.interfaces.nsILocalFile).path;
> }

I don't know that this is always a safe place to write files. The test would also fail if a file with your name happen to already exist. Check what the existing inputelement.files tests do and do that instead.


>diff --git a/dom/interfaces/events/nsIDOMDataTransfer.idl b/dom/interfaces/events/nsIDOMDataTransfer.idl

>+interface nsIDOMFileList;
> 
> [scriptable, uuid(E4970BA1-9567-455C-8B4E-4607D7E741BB)]
> interface nsIDOMDataTransfer : nsISupports

Update the iid.

Looks good otherwise. Please attach a new patch once you have the above comments fixed.
Attachment #387982 - Attachment is obsolete: false
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 387992 [details] [diff] [review]
Proposed patch, now with even less superfluousness!

Marking sr- per previous comments
Attachment #387992 - Flags: superreview?(jonas)
Attachment #387992 - Flags: superreview-
Attachment #387992 - Flags: review?(enndeakin)
Attached patch Proposed patch for file dnd v4 (obsolete) — Splinter Review
Per Jonas's input:

-Reusing existing filesList reference, instead of instantiating new ones
-PopulateFilesList() helper function now returns nsresult, instead of PRBool
-More robust error-checking in assuring proper query-interfacing
-Whitespace/ ickiness removed
-Test harness writes and manipulates files to the temp directory
-Test harness mixes non-file data with files in the dataTransfer's internal mItems list
-Files created during testing now have varying amounts of data in them, instead of simply being empty

Woo!
Attachment #387982 - Attachment is obsolete: true
Attachment #387992 - Attachment is obsolete: true
Attachment #388389 - Flags: superreview?(jonas)
Attachment #388389 - Flags: review?(enndeakin)
Comment on attachment 388389 [details] [diff] [review]
Proposed patch for file dnd v4

>diff --git a/content/events/src/nsDOMDataTransfer.cpp b/content/events/src/nsDOMDataTransfer.cpp

>+nsDOMDataTransfer::GetFiles(nsIDOMFileList** aFileList)
>+{
>+  if (!mFiles) {
>+    mFiles = new nsDOMFileList();
>+    nsresult rv = PopulateFilesList();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

You need to initialize *aFileList to nsnull in case you take the early return here. Generally for getters like this you should always null out out-argument to ensure that it doesn't contain garbage which the caller then might call AddRef on or some such.

>@@ -485,17 +504,17 @@ nsDOMDataTransfer::AddElement(nsIDOMElem
> }
> 
> nsresult
> nsDOMDataTransfer::Clone(PRUint32 aEventType, PRBool aUserCancelled,
>                          nsIDOMDataTransfer** aNewDataTransfer)
> {
>   nsDOMDataTransfer* newDataTransfer =
>     new nsDOMDataTransfer(aEventType, mEffectAllowed, mIsExternal, aUserCancelled,
>-                          mItems, mDragImage, mDragImageX, mDragImageY);
>+                          mItems, mFiles, mDragImage, mDragImageX, mDragImageY);
>   NS_ENSURE_TRUE(newDataTransfer, NS_ERROR_OUT_OF_MEMORY);

You can't share the file-list here since the two dataTransfers can contain different files. So just let the new data-transfer initialize the file-list lazily if needed.

I think this means that you also don't need to add the new argument to the dataTransfer constructor.


>@@ -811,8 +834,40 @@ nsDOMDataTransfer::FillInExternalDragDat
>       variant->SetAsAString(str);
>     }
>     else {
>       variant->SetAsISupports(data);
>     }
>     aItem.mData = variant;
>   }
> }
>+
>+nsresult
>+nsDOMDataTransfer::PopulateFilesList()
>+{
>+  if(!mFiles)
>+    return NS_OK;
>+
>+  mFiles->Clear();
>+  PRUint32 count = mItems.Length();
>+
>+  for (PRUint32 i = 0; i < count; i++) {
>+    nsCOMPtr<nsIVariant> variant;
>+    nsresult rv = MozGetDataAt(NS_ConvertUTF8toUTF16(kFileMime), i, getter_AddRefs(variant));

Hmm.. just looked at the code for MozGetDataAt. Are you sure that your code will work even when the user doesn't have UniversalXPConnect rights? Have you tested simply dragging files to a normal (not mochitest) page and seen that you can access the data?

Also, do you need to deal kFilePromiseMime in addition to kFileMime here? Not really sure how that works.

>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if(!variant)
>+      continue;
>+
>+    nsCOMPtr<nsISupports> supports;
>+    rv = variant->GetAsISupports(getter_AddRefs(supports));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIFile> file = do_QueryInterface(supports);
>+    NS_ASSERTION(file, "Expecting nsISupports to permit query interface for nsIFile.");

Change the assertion to something like:

"Expecting file data". Query interface is always from nsISupports so no need to mention that.

>+    nsRefPtr<nsDOMFile> domFile = new nsDOMFile(file);

Check that the memory allocation succeeds here. Something like
NS_ENSURE_TRUE(domFile, NS_ERROR_OUT_OF_MEMORY) should work.

>diff --git a/content/events/src/nsDOMDataTransfer.h b/content/events/src/nsDOMDataTransfer.h

>+  // array of files, containing only the files present in the dataTransfer
>+  nsCOMPtr<nsDOMFileList> mFiles;

Actually, you should never stick a concrete class (such as nsDOMFileList) in an nsCOMPtr. nsCOMPtr is made for interfaces (like nsIDOMFileList). Use nsRefPtr here instead.

>diff --git a/content/events/test/test_dragstart.html b/content/events/test/test_dragstart.html
>--- a/content/events/test/test_dragstart.html
>+++ b/content/events/test/test_dragstart.html
>@@ -12,16 +12,19 @@
>  
> <script>
> 
> SimpleTest.waitForExplicitFinish();
> 
> var gDragInfo;
> var gDataTransfer = null;
> var gExtraDragTests = 0;
>+var fileIndex = 0;
>+var testFileName = "bug503598testfile";
>+var fileTypes = [];
> 
> function runTests()
> {
>   // first, create a selection and try dragging it
>   var draggable = $("draggable");
>   window.getSelection().selectAllChildren(draggable);
>   synthesizeMouse(draggable, 6, 6, { type: "mousedown" });
>   synthesizeMouse(draggable, 14, 14, { type: "mousemove" });
>@@ -385,16 +388,166 @@ function test_DataTransfer(dt)
>    "linkMove", "copyLink", "all", "uninitialized", "none"].forEach(
>     function (i) {
>       dt.effectAllowed = i;
>       is(dt.dropEffect, "move", "dropEffect not modified by effectAllowed set to " + i);
>       is(dt.effectAllowed, i == "" || i == "other" || i == "moveCopy" ? "link" : i,
>          "effectAllowed set to " + i);
>     }
>   );
>+
>+  //Test the .files attribute on data transfers
>+  try {
>+    var typeDataPairs = [["text/plain", "Additional text alongside files"],
>+			 ["text/html", "<em>Additional html text alongside files</em>"],
>+			 ["text/uri-list", "http://www.mozilla.org/"]];
>+
>+    //Make sure files list is initially empty
>+    is(dt.files.length, 0, ".files should have zero length on an empty datatransfer");
>+    is(dt.files[0], null, ".files should return null on index accesses");

Should say ".files should return null on out-of-bounds index access".

Still need to review the tests.
Attached patch Proposed patch for file dnd v5 (obsolete) — Splinter Review
All excellent input, thanks again Jonas!

Fixed in this version of the patch's patch:
-Ensuring getter doesn't retrieve garbage by initializing aFilesList to nsnull
-Removed fileList argument in the constructor (you're right; there's no reason to pass the argument explicitly in, let's just allow clones to lazily initialize the fileList)
-Ensuring memory allocation doesn't crash and burn
-nsRefPtr vs. nsCOMPtr distinction duly noted :-)
-Beautified error/test output

Regarding the MozGetDataAt point you brought up: my code certainly works within a normal page. I've attached a sample HTML file containing relevant dataTransfer/.files code demonstrating this feature. Feel free to try it out for yourself.

I see no compelling reason to deal with kFilePromiseMime data types here. Admittedly, this seems counter-intuitive, as nsITransferable.idl notes that kFilePromiseMimes are "a dataless flavor used to interact with the OS during file drags" [1]. While it may seem pertinent, as far as I know - and I may be mistaken, so it's worth gathering Neil's input on this - kFileMime is the only data type ever touched during drag n' drop of local files.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/public/nsITransferable.idl#71
Attachment #388389 - Attachment is obsolete: true
Attachment #388411 - Flags: superreview?(jonas)
Attachment #388411 - Flags: review?(enndeakin)
Attachment #388389 - Flags: superreview?(jonas)
Attachment #388389 - Flags: review?(enndeakin)
kFilePromiseMime is only used when dragging from Mozilla to other applications or the desktop. It's used to indicate that the file referred to isn't yet available but will be when the drop occurs. When dragging from a file, it naturally isn't needed.
Does the copy-paste code use DataTransfers? If so, what happens if you copy an image in one window and paste it in another?
(In reply to comment #12)
> Does the copy-paste code use DataTransfers?

No, that's bug 407983.
So what happens if you drag an image from one window to another? Do we create a file-object and use kFileMime by the time we fire the DROP event?

Orthogonally, when do we fire NS_DRAGDROP_DRAGDROP events? Is it the same thing as a NS_DRAGDROP_DROP event, only it's used for XUL backwards compat?
Are you expecting dataTransfer.files to only return something on drop, or during all drag events? Content cannot access the file info during other events, so the cached list will end up being empty.
It seems safest to only expose it during "drop" events. Otherwise the page could steal file data just because the user happens to drag a file from an app on one side of the browser window to another.

I'll check with the spec people what the intended behavior is.
(In reply to comment #16)
> It seems safest to only expose it during "drop" events. Otherwise the page
> could steal file data just because the user happens to drag a file from an app
> on one side of the browser window to another.

In our implementation, assuming no caching occurs, it will just return nothing as the security check will fail.

Having it only available seems safer.

Once concern with the .files api though, is that is doesn't provide a means of knowing if a file is being dragged at all, without checking for our application/x-moz-file type.
Btw, you never answered my second paragraph in comment 14.

(In reply to comment #17)
> Once concern with the .files api though, is that is doesn't provide a means of
> knowing if a file is being dragged at all, without checking for our
> application/x-moz-file type.

Indeed. Two possible solutions are:

1. Always populate the .files list, but make .item() throw if the files haven't been dropped yet. That way you can check .files.length

2. Have a second property like .containsFiles which is set appropriately during drag events.
I talked with Hixie about this. Given that you during drag event currently can't get any information about what data is stored in the DataTransfer, I don't think we need to special-case information about possible contained files here.

Eventually the spec will add an API to DataTransfer that allows the page to check for existence of data types without getting to the actual data, that API will then be possible to use to check for existence of files.


So the only remaining questions I have are the ones in comment 14. And I need to further review the tests. Other than that I think the patch looks good.
(In reply to comment #19)
> Eventually the spec will add an API to DataTransfer that allows the page to
> check for existence of data types without getting to the actual data, that API
> will then be possible to use to check for existence of files.

The spec does have such an API: dataTransfer.types, modeled after the one Safari implemented, and we implemented as well.
Oh, indeed, so we just need to define what type files are supposed to be. I must have misunderstood Hixie. Will poke him again.
Comment on attachment 388411 [details] [diff] [review]
Proposed patch for file dnd v5

>+    [1,2,5,10].forEach(
>+	function (numFiles) {
>+	  //Test .files returns only files, and nothing else
>+	  typeDataPairs.forEach(
>+	    function (typeDataPair) {
>+	      var type = typeDataPair[0];
>+	      var data = typeDataPair[1];
>+
>+	      //Test non-files at the beginning of the data list
>+	      appendNDatums(dt, type, data, numFiles);
>+	      appendNFiles(dt, numFiles);
>+	      is(dt.files.length, numFiles,
>+		".files has incorrect length with "+type+" present at the beginning of dataTransfer");
>+	      cleanUpData(dt);
>+
>+	      //Test non-files at the middle of the data list
>+	      var half = Math.floor(numFiles/2);
>+	      appendNFiles(dt, half);
>+	      appendNDatums(dt, type, data, numFiles);
>+	      appendNFiles(dt, numFiles - half);
>+	      is(dt.files.length, numFiles,
>+		".files has incorrect length with "+type+" present in the middle of dataTransfer");
>+	      cleanUpData(dt);


Add "datums" before and after the files here as well, just to add a bit more complexity. I.e.

datums
files
datums
files
datums

Other than that looks great.

We need to figure out what to return for DataTransfer.types, but lets deal with that once the spec says what should be returned.

Neil: Would be great if we can get this reviewed pretty soon so that this can go in before the 1.9.2 branch at the end of the month.
Attachment #388411 - Flags: superreview?(jonas) → superreview+
Neil: The part I was most uncertain about was the questions in comment 14
Attached patch Proposed patch for file dnd v6 (obsolete) — Splinter Review
Cool. New version of the patch attached, containing a couple more complex test cases.
Attachment #388411 - Attachment is obsolete: true
Attachment #390268 - Flags: superreview?(jonas)
Attachment #390268 - Flags: review?(enndeakin)
Attachment #388411 - Flags: review?(enndeakin)
Comment on attachment 390268 [details] [diff] [review]
Proposed patch for file dnd v6

What I mean is that during a dragover event, for example, a chrome event listener could retrieve 'files', which would cache the files, which a later content caller would end up retrieving. We should be explicit and only check for the drop/dragdrop events during GetFiles. It's ok to not have them available to chrome callers as well.

If you did, you could also remove the other calls to PopulateFilesList, as datatransfers are readonly during a drop.

It would mean changing the test though, but using synthesizeDrop from EventUtils.js would help.


>+nsresult
>+nsDOMDataTransfer::PopulateFilesList()
>+{
>+  if(!mFiles)

space after 'if', as well as couple other places in this method.


>+
>+    nsCOMPtr<nsISupports> supports;
>+    rv = variant->GetAsISupports(getter_AddRefs(supports));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIFile> file = do_QueryInterface(supports);
>+    NS_ASSERTION(file, "Expecting data to be in file format.");

This shouldn't be an assertion, since someone could have just put the wrong data here. In general, I don't think we should propgate most of the errors out here; instead we should just return an empty list or a list without that item.


>   /**
>+   * Holds a list of all the local files available on the current data transfer.
>+   * A dataTransfer containing no files will return an empty list, and an
>+   * invalid index access on the resulting file list will return null. 
>+   */

'invalid index access' doesn't seem to apply here, as it's just a property. I think you may be referring to this calling MozGetDataAt but that is an implementation detail that shouldn't be referred to here.

Also either say 'available on this data transfer' or just end the sentence after 'available'.
(In reply to comment #25)

Thanks Neil! A few points to clarify, though:

> If you did, you could also remove the other calls to PopulateFilesList, as
> datatransfers are readonly during a drop.

Doesn't the DataTransfer hold an internal boolean (mReadOnly) that can restrict modification via methods like MozSetDataAt and MozClearDataAt? If so, I'm not sure I understand why other calls to PopulateFilesList() would need to be removed. Couldn't we just set mReadOnly to true on a drop?
 
> This shouldn't be an assertion, since someone could have just put the wrong
> data here. In general, I don't think we should propgate most of the errors out
> here; instead we should just return an empty list or a list without that item.

Correct me if I make any misguided assumptions here.

The call to MozGetDataAt several lines above contains an argument that specifies the expected format of the data item we're retrieving. If the entry at the given index is *not* a file, then MozGetDataAt returns null, in which case the entry is skipped and the next item in the dataTransfer list is checked. As far as I know, if MozGetDataAt returns a non-null value with kFileMime as the first argument, then MozGetDataAt is assuring us that we've retrieved a value representing a file. Thus, an error here - where the value won't QI to a file - would imply an error in the implementation of MozGetDataAt, which is bad news. Hence raising an assertion.

> 'invalid index access' doesn't seem to apply here, as it's just a property. I
> think you may be referring to this calling MozGetDataAt but that is an
> implementation detail that shouldn't be referred to here.

What I'm referring to is an invalid index access on the property itself, since the property is a list. For instance:

	//User drags in a single file, so dt.files.length is 1;
	...
	var imaginaryFile = dt.files[1];	//imaginaryFile will be null

This is the behavior described in the current spec, so I figured it was worth describing in the interface file.
(In reply to comment #26)

> Doesn't the DataTransfer hold an internal boolean (mReadOnly) that can restrict
> modification via methods like MozSetDataAt and MozClearDataAt? If so, I'm not
> sure I understand why other calls to PopulateFilesList() would need to be
> removed. Couldn't we just set mReadOnly to true on a drop?

mReadOnly is already set to true on all events except dragstart/draggesture.

> I was referring to changing the code to only allow dataTransfer.files to return
> anything on drop, in which case modifying the data wouldn't be possible.
> As far as I know, if MozGetDataAt returns a non-null value with
> kFileMime as the first argument, then MozGetDataAt is assuring us that we've
> retrieved a value representing a file.

MozGetDataAt doesn't do this, no. MozGetDataAt returns whatever object happened to be stored there. For drags from other applications this will never happen. But some extension could theoretically stuff some other value there, possibly due to a bug in that extension's code.

Assertions are intended for when really bad things happen.
> What I'm referring to is an invalid index access on the property itself, since
> the property is a list.

Ah, OK that makes sense then.
(In reply to comment #27)
 
> MozGetDataAt doesn't do this, no. MozGetDataAt returns whatever object happened
> to be stored there. For drags from other applications this will never happen.
> But some extension could theoretically stuff some other value there, possibly
> due to a bug in that extension's code.

Hmm. Strange, because if I drag in some plain-text data, and then call MozGetDataAt("application/x-moz-file", 0), the return value is null. On the other hand, when I call MozGetDataAt("text/plain", 0), the plaintext data gets properly returned. Is this not the expected behavior?
If a privileged caller does MozSetDataAt("application/x-moz-file", document)
Attached patch Proposed patch for file dnd v7 (obsolete) — Splinter Review
Fixed a few issues. More important ones:
1. No more strict assertions regarding misuse of MozSetDataAt
2. PopulateFilesList only proceeds with retrieving a file list if the data transfer's event type is a NS_DRAGDROP_DRAGDROP or a NS_DRAGDROP_DROP. Does this look fine, Neil?

Introduced an issue:
1. For the time being, the test_dragstart MochiTest does not test the .files attribute. I've already spoken to Jonas about this, and we agreed that rolling out a version of the drag n' drop patch held a higher priority than fixing minor quibbles with the MochiTest. I've also already pinged Neil about this, and the only change that needs to be made for the test to run smoothly is a proper implementation of synthesizeDrop() in EventUtils.js that manages to initialize a dataTransfer with eventType = "drop".

Once that small change is made, everything should be good to go.
Attachment #390268 - Attachment is obsolete: true
Attachment #391290 - Flags: superreview?(jonas)
Attachment #391290 - Flags: review?(enndeakin)
Attachment #390268 - Flags: superreview?(jonas)
Attachment #390268 - Flags: review?(enndeakin)
Comment on attachment 391290 [details] [diff] [review]
Proposed patch for file dnd v7

>+nsDOMDataTransfer::PopulateFilesList()
>+{
>+  if (!mFiles)
>+    return NS_OK;
>+
>+  mFiles->Clear();
>+  PRUint32 count = mItems.Length();
>+
>+  if (mEventType != NS_DRAGDROP_DROP && mEventType != NS_DRAGDROP_DRAGDROP)
>+    return NS_OK;
>+
>+  for (PRUint32 i = 0; i < count; i++) {
>+    nsCOMPtr<nsIVariant> variant;
>+    nsresult rv = MozGetDataAt(NS_ConvertUTF8toUTF16(kFileMime), i, getter_AddRefs(variant));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (!variant)
>+      continue;
>+
>+    nsCOMPtr<nsISupports> supports;
>+    rv = variant->GetAsISupports(getter_AddRefs(supports));
>+    NS_ENSURE_SUCCESS(rv, rv);

Given that we want to deal with MozGetDataAt returning non-file, you should deal with GetAsISupports returning a non-success value by |continue|ing rather than returning an error. So change the above NS_ENSURE_SUCCESS to

if (NS_FAILED(rv)) {
  continue;
}


It would actually be good if you tested this too, but calling MozSetDataAt to store a string with the file format or some such.

sr=me with that fixed.
Attachment #391290 - Flags: superreview?(jonas) → superreview+
Attached patch Proposed patch for file dnd v8 (obsolete) — Splinter Review
Awesome, just attached a new version of the patch with less error propagation. Just ran some informal tests, and found that a call to dt.mozSetDataAt("application/x-moz-file", "plain text") reflects changes in mozItemCount but not in the files attribute. No exceptions thrown or anything. I'm assuming this is the behavior we want.
Attachment #391290 - Attachment is obsolete: true
Attachment #391451 - Flags: superreview?(jonas)
Attachment #391451 - Flags: review?(enndeakin)
Attachment #391290 - Flags: review?(enndeakin)
Comment on attachment 391451 [details] [diff] [review]
Proposed patch for file dnd v8

Any word on getting review on this by the freeze?
Attachment #391451 - Flags: superreview?(jonas) → superreview+
Comment on attachment 391451 [details] [diff] [review]
Proposed patch for file dnd v8

> NS_IMETHODIMP
> nsDOMDataTransfer::MozClearDataAt(const nsAString& aFormat, PRUint32 aIndex)
...
>+  nsresult rv = PopulateFilesList();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

The data is readonly during a drop so no need for this.


> void
> nsDOMDataTransfer::ClearAll()
> {
>   mItems.Clear();
>+  PopulateFilesList();
> }

Same here.


> nsresult
> nsDOMDataTransfer::SetDataWithPrincipal(const nsAString& aFormat,
>                                         nsIVariant* aData,
>                                         PRUint32 aIndex,
>                                         nsIPrincipal* aPrincipal)
...
>+  nsresult rv = PopulateFilesList();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

And here as well.

>+
>+nsresult
>+nsDOMDataTransfer::PopulateFilesList()
>+{
>+  if (!mFiles)
>+    return NS_OK;
>+
>+  mFiles->Clear();
>+  PRUint32 count = mItems.Length();
>+
>+  if (mEventType != NS_DRAGDROP_DROP && mEventType != NS_DRAGDROP_DRAGDROP)
>+    return NS_OK;

You could just put this in GetFiles. In fact, a separate method isn't really necessary.


OK, so there isn't really a way to test this. We should probably just not check in the test.
Attached patch Proposed patch for file dnd v9 (obsolete) — Splinter Review
Well, that significantly simplifies the patch. :-) All calls to PopulateFilesList() removed, so that the file retrieval code is consolidated in the getter. I'm not marking my previous version of the patch obsolete, just in case some error is found in my most recent implementation and Jonas decides it's best to go with the solution that works but is slightly clunkier.
Attachment #391637 - Flags: superreview?(jonas)
Attachment #391637 - Flags: review?(enndeakin)
Comment on attachment 391637 [details] [diff] [review]
Proposed patch for file dnd v9

>+  if (!mFiles) {
>+    mFiles = new nsDOMFileList();
>+

Just add a null check here, and this looks good!
Attachment #391637 - Flags: review?(enndeakin) → review+
Excellent. Quick change made.
Attachment #391637 - Attachment is obsolete: true
Attachment #391650 - Flags: superreview?(jonas)
Attachment #391650 - Flags: review?(enndeakin)
Attachment #391637 - Flags: superreview?(jonas)
Attachment #391650 - Flags: review?(enndeakin) → review+
Attachment #391451 - Flags: review?(enndeakin)
Comment on attachment 391650 [details] [diff] [review]
Proposed patch for file dnd v10

We really need to get tests working asap though :(
Attachment #391650 - Flags: superreview?(jonas) → superreview+
fwiw, that was Jonas using my computer.
Patch has been checked in today. See here: http://hg.mozilla.org/mozilla-central/rev/0a8bed5ebe1a .

Bug isn't quite closed yet, because I've still got to figure out how to properly synthesize mouse drops. Stay tuned for more info!
in-testsuite flag is just for that! See EventUtils's synthesizeMouse[Drag|Drop] for synthesizing mouse drags at: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
My apologies, I should've been more descriptive.

The version of synthesizeDrop in EventUtils.js throws an exception because it uses outdated calls to initDragEvent. Updating those will properly synthesize a drag, but not quite a drop, since it sets the dataTransfer's eventType field to "dragstart" instead of "drop." Because the .files attribute only returns a populated list in a dataTransfer with eventType == "drop" (to prevent security issues related to the chrome making fallacious calls to mozSetDataAt), this version of synthesizeDrop will fail to attach any meaningful file data to the dataTransfer. I needed to figure out how to synthesize a true "drop" (i.e. setting the eventType field of the dataTransfer to "drop"), which I wasn't able to do in time for the code freeze.

In addition, Neil claims we're not going to be able to test this, which is slightly troublesome. :-/
Attachment #391451 - Attachment is obsolete: true
Comment on attachment 388413 [details]
Sample code to experiment with file dnd.

><html>
><head>
><h3>Drag any number of files into the magical box below: </h3>
><script>
>
>function dodrop(event)
>{
>  var dt = event.dataTransfer;
>  var files = dt.files;
>
>  var count = files.length;
>  output("File Count: " + count + "\n");
>
>  for (var i = 0; i < count; i++) {
>    output(" File " + i + ":\n");
>    var types = dt.mozTypesAt(i);
>    for (var t = 0; t < types.length; t++) {
>      output("  " + types[t] + ": ");
>      try {
>        var data = files[i];
>        output("(" + (typeof data) + ") : <" + data + " > " + data.fileName + " " + data.fileSize + "\n");
>      } catch (ex) {
>        output("<<error>>\n");
>        dump(ex);
>      }
>    }
>  }
>}
>
>function output(text)
>{
>  document.getElementById("output").textContent += text;
>  dump(text);
>}
>
></script>
></head>
><body>
>
><div id="output" style="min-height: 100px; white-space: pre; border: 1px solid black;"
>     ondragenter="document.getElementById('output').textContent = ''; event.stopPropagation(); event.preventDefault();"
>     ondragover="event.stopPropagation(); event.preventDefault();"
>     ondrop="event.stopPropagation(); event.preventDefault(); dodrop(event);">
>
></body>
></html>
Keywords: dev-doc-needed
Sheppy: Is there a reason to use mozTypesAt in the example? It's mozilla-only and doesn't seem to add any value to the example. (Also, I think you're using it incorrectly, though not sure about that).
Something as simple as:

for (var i = 0; i < count; i++) {
  output("(" + (typeof data) + ") : <" + data + " > " + data.fileName + " " +
         data.fileSize + "\n");
}

should do the trick I'd think
Err.. that should of course be:

for (var i = 0; i < files.length; i++) {
  output("(" + (typeof files[i]) + ") : <" + files[i] + " > " +
         files[i].fileName + " " + files[i].fileSize + "\n");
}
Aaand then I forgot that the names of the properties has changed, so it's now.

for (var i = 0; i < files.length; i++) {
  output("(" + (typeof files[i]) + ") : <" + files[i] + " > " +
         files[i].name + " " + files[i].size + "\n");
}
That's used because I copied and pasted the code from one of the tests attached to this bug. I'll update the sample.
Sample has been replaced. Also did some style cleanup and added notes that the moz* methods are Gecko-specific.

https://developer.mozilla.org/En/DragDrop/DataTransfer
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.