While working on bug 192728 (which is about porting FF's download manager drag&drop) I found that FF's download drag&drop doesn't work on Linux. From bug 192728 comment 8: "I couldn't find a program on Linux that serves as a valid drop target. I tried Nautilus (Gnome), Thunar (XFce), Dolphin (KDE 4), Konqueror (KDE 4), ROX Filer. Works fine on WinXP. Same in FF. Possible hint: ROX Filer says "Sorry - I need a target of type text/uri-list or XdndDirectSave0"." From bug 192728 comment 10: "I also found why d&d didn't work on Linux (the ROX error message was really helpful!). Implementing text/uri-list and text/plain in addition is the key. That makes it work in all applications listed in comment 8. I even tried SciTE and KWrite (both text editors) successfully. <http://mdn.beonex.com/En/DragDrop/Recommended_Drag_Types> (Note the following under Dragging Files: "If possible, you may also include the file URL of the file using both the text/uri-list and/or text/plain types. These types should be added last so that the more specific application/x-moz-file type has higher priority.") <http://www.rfc-editor.org/rfc/rfc2483.txt> (Notes that CRLF is required. Something that ROX will happily tell you, too.)" Effectively you just need to replace this: var dt = aEvent.dataTransfer; dt.mozSetDataAt("application/x-moz-file", f, 0); dt.effectAllowed = "copyMove"; by this: var url = Services.io.newFileURI(f).spec; var dt = aEvent.dataTransfer; dt.mozSetDataAt("application/x-moz-file", f, 0); dt.setData("text/uri-list", url + "\r\n"); dt.setData("text/plain", url + "\n"); dt.effectAllowed = "copyMove"; I'll leave creating a patch and driving this through to you guys since you'll probably require a test and I have no intention to go through the review hassle. FF has many more devs than SM and I already did the analysis + solution. Have fun.

This is rather old bug, still valid and mentioned fix still works. Attaching patch, please have a look. I'm not sure who to ask for review, feel free to reassign. Part is in browser code and part in toolkit. It might have to be splitted.

Comment on attachment 658045 [details] [diff] [review] d&d patch 1 I'm definitely not the right reviewer here. Punting to Marco, maybe he can review this.

Comment on attachment 658045 [details] [diff] [review] d&d patch 1 Review of attachment 658045 [details] [diff] [review]: ----------------------------------------------------------------- There are various traling spaces in the patch that should be removed, please have a look for them. ::: toolkit/mozapps/downloads/content/downloads.js @@ +726,5 @@ > > var dt = aEvent.dataTransfer; > dt.mozSetDataAt("application/x-moz-file", f, 0); > + var url = Services.io.newFileURI(f).spec; > + dt.setData("text/uri-list", url + "\r\n"); I think the spec refers to CRLF in the sense of "where line termination is needed it should be a CRLF", though that's quite unclear. Also HTML5 dnd specs uses "separated by CRLF", not "terminated by CRLF", there's nothing to separate here. What I can say for sure is we never added newline terminators, and our documentation does the same, so if it works regardless I'd left them out. Same comment is valid for the other changes, so could you please verify if it works without the trailing terminators? @@ +747,5 @@ > var dt = aEvent.dataTransfer; > + // If dragged item is from our source, do not try to > + // redownload already downloaded file. > + if (dt.mozGetDataAt("application/x-moz-file", 0)) > + return; This looks like trying to fix a different bug... Care to explain? Eventually should be split apart to its own bug.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_bug_462172.xul can likely be updated to verify this fix

Added fix for test and removed CRLF. D&D is working without additional CRLF. > @@ +747,5 @@ > > var dt = aEvent.dataTransfer; > > + // If dragged item is from our source, do not try to > > + // redownload already downloaded file. > > + if (dt.mozGetDataAt("application/x-moz-file", 0)) > > + return; > > This looks like trying to fix a different bug... Care to explain? Eventually > should be split apart to its own bug. When you start to drag from download manager and you drop also on download manager file is "downloaded" from file://... again because Mozilla accepts drops of text/uri-list or text/plain.

Comment on attachment 666507 [details] [diff] [review] d&d patch 2 Review of attachment 666507 [details] [diff] [review]: ----------------------------------------------------------------- Could you please verify if DownloadsIndicatorView.onDrop needs the same kind of fix? It's managing DND on the Downloads panel (the panel opened from the new downloads button in Nightly), you fixed the creation in both the panel and the window, but drop only in the window. So I suspect it's still possible to drag & drop to self in the panel. ::: toolkit/mozapps/downloads/content/downloads.js @@ +747,5 @@ > var dt = aEvent.dataTransfer; > + // If dragged item is from our source, do not try to > + // redownload already downloaded file. > + if (dt.mozGetDataAt("application/x-moz-file", 0)) > + return; I wonder if there's some internal source that may allow drops here with x-moz-file but off-hand I can't think of any... So I think we can take this and if someone knocks at our door we will just add some more data to distinguish. ::: toolkit/mozapps/downloads/tests/chrome/test_bug_462172.xul @@ +66,5 @@ > state: Ci.nsIDownloadManager.DOWNLOAD_FINISHED} > ]; > > +function compareStrFunc(actualData, expectedData) { > + return expectedData == actualData; please use ===

Comment on attachment 666507 [details] [diff] [review] d&d patch 2 just feedback for now, waiting for answer to the previous comment

> Could you please verify if DownloadsIndicatorView.onDrop needs the same kind > of fix? It's managing DND on the Downloads panel (the panel opened from the > new downloads button in Nightly), you fixed the creation in both the panel > and the window, but drop only in the window. So I suspect it's still > possible to drag & drop to self in the panel. Your right, dragging to button from download manager triggers download again. I've attached fixed patch.

Comment on attachment 670733 [details] [diff] [review] d&d patch 3 Review of attachment 670733 [details] [diff] [review]: ----------------------------------------------------------------- It looks fine now, just a small comment. Thank you for looking into this. ::: browser/components/downloads/content/indicator.js @@ +517,5 @@ > + let dt = aEvent.dataTransfer; > + // If dragged item is from our source, do not try to > + // redownload already downloaded file. > + if (dt.mozGetDataAt("application/x-moz-file", 0)) > + return; I'd put your code at the beginning of the method, mostly cause we don't need name and url until later, so keep vars definition near their first use.

Patch fixed, thanks for review, copying review+ from previous patch.

In future please use checkin-needed keyword when requesting a checking (I'm adding it for you now)

Pushed to Try (all tests on Linux & Linux64, and one Mac platform just in case there are unintended non-Linux consequences): https://tbpl.mozilla.org/?tree=Try&rev=ec17db31c5b8