819373 - Handle drag and drop of a download link

Status: NEW

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P5)

Description

[Tom S]

When you drag a link we the download attribute onto the desktop, we want to download this instead of just creating a link to the file. Bug 570164 is quite similar.

Comment 1

[Tom S]

I think this might actually be pretty easy. We already support something called x-moz-file-promise. And we even have documentation for it :)
https://developer.mozilla.org/en-US/docs/DragDrop/Recommended_Drag_Types#Dragging_files_to_an_operating_system_folder

Comment 2

[Tom S]

You can drag and drop files in Firefox on Linux, which makes this kind of painful to test and implement for me. But this should be relatively easy.

Comment 3

[Tom S]

*You can't

Comment 4

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

Well, at least image dnd from file system works on linux.

Comment 5

[Neil Deakin]

Only text and urls are supported for dropping on gtk currently.

Comment 6

[Tom S]

Attachment: WIP patch

Now that I got drag and drop working on linux, I could actually implement this. I still have to iron out some of the details, but this works.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 813271 [details] [diff] [review]
WIP patch

>+  static void GetDownloadLink(nsIContent *aContent, nsIURI *aLinkURI,

Clearly bogus return value.  And needs to document aLinkURI, no?

And maybe call it IsDownloadLink?

>+++ b/content/base/src/nsContentAreaDragDrop.cpp

Someone familiar with this code should review it, please.

>+++ b/content/base/src/nsContentUtils.cpp
>+  // Check if there is an download attribute.

s/an/a/

>+  if (!aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::download, aFileName))

Curlies around if body, please.

>+++ b/widget/gtk/nsDragService.cpp

Changes here are ok, but not obviously related...

Comment 8

[Tom S]

Arguably having to pass the aLinkURI is a pretty bad API, but I am not sure how abstract that between the two cases. Maybe we should always use href?

Comment 9

[[e-mail]]

Comment on attachment 813271 [details] [diff] [review]
WIP patch

>+      if (!nsContentUtils::GetDownloadLink(aDragNode, hrefURI, fileName)) {
>+        if (!mImage)
>+          return NS_OK;
This is very ugly, and possibly prone to error if someone needs to tweak the logic.
I tried to think of several approaches, but you probably don't want to accidentally download an image linked to a download that doesn't specify a file name, in which case I ended up with this approach:
if (mIsAnchor && nsContentUtils::GetDownloadLink(...)) {
  // this is a download, so promise to download it
} else if (mImage) {
  // promise the image
}
if (!fileName.IsEmpty()) {
  // we found something to download
}
Note that you don't now need the outer if (mImage || mIsAnchor) test.

>+      if (fileName.IsEmpty()) {
>+        nsAutoCString fileNameC;
>+        imgUrl->GetFileName(fileNameC);
Which URL is this? I don't see a relevant one handy. You might want the file from mUrlString, but I suppose you'd have to use string manipulation to extract it.

>diff --git a/widget/gtk/nsDragService.cpp b/widget/gtk/nsDragService.cpp
[Unnecessary change?]

Comment 10

[Andy Mercer]

Could we not just duplicate what was used in Chromium to finish this?

Comment 11

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.