[Chromium] Add DownloadURL format to Chromium clipboard.

[Jian Li]

Add DownloadURL format to Chromium clipboard in order to support dragging a virtual file out of the browser.

[Tony Chang]

Out of curiosity, what is an example flow that is currently missing?  ctrl+drag of a link to the desktop?

[Jian Li]

Created attachment 42432 [details]
Proposed Patch

[Jian Li]

(In reply to comment #1)
> Out of curiosity, what is an example flow that is currently missing?  ctrl+drag
> of a link to the desktop?

One scenario is to support dragging an email attachment to the desktop. The web
application has to call event.dataTransfer.setData() to add the specific format
in order to mean to download the url, instead of copying the url value.

[Tony Chang]

(In reply to comment #3)
> One scenario is to support dragging an email attachment to the desktop. The web
> application has to call event.dataTransfer.setData() to add the specific format
> in order to mean to download the url, instead of copying the url value.

I see, so this can only be set via JS?

Should we introduce a new URL member? What if JS wants to set both downloadurl and a regular url?

[Jian Li]

(In reply to comment #4)
> (In reply to comment #3)
> > One scenario is to support dragging an email attachment to the desktop. The web
> > application has to call event.dataTransfer.setData() to add the specific format
> > in order to mean to download the url, instead of copying the url value.
> 
> I see, so this can only be set via JS?

Yes, no additional UI.
> 
> Should we introduce a new URL member? What if JS wants to set both downloadurl
> and a regular url?

No, because DownloadUrl takes higher priority. I do not think it make sense if we do both the download and url value copy.

[Tony Chang]

(In reply to comment #5)
> (In reply to comment #4)
> > Should we introduce a new URL member? What if JS wants to set both downloadurl
> > and a regular url?
> 
> No, because DownloadUrl takes higher priority. I do not think it make sense if
> we do both the download and url value copy.

Doesn't the drop target make that decision?  E.g., if you drag something from a web page and you drop it on a external application, it might not know how to handle a downloadurl, but it might know how to handle a regular URL.  Am I misunderstanding how downloadurl is going to be used?

[Dmitry Titov]

I think this was discussed on whatwg.. The idea is not that the drop target outside of browser will know how to handle DownloadUrl. The idea is to let the page to tell the browser to replace the DownloadUrl format with a platform-dependent file format, so that the OS file management app (Explorer or Desktop on Windows or Finder on Mac) could pull a file (via download done by the browser) and put it in the drop location. 
So when you drag-drop something from the page to the desktop for example, the page will add DownloadUrl format to dataTransfer object, and in return, the browser will add a platform-dependent streaming file format to the clipboard.

[Tony Chang]

(In reply to comment #7)
> I think this was discussed on whatwg.. The idea is not that the drop target
> outside of browser will know how to handle DownloadUrl. The idea is to let the
> page to tell the browser to replace the DownloadUrl format with a
> platform-dependent file format, so that the OS file management app (Explorer or
> Desktop on Windows or Finder on Mac) could pull a file (via download done by
> the browser) and put it in the drop location. 
> So when you drag-drop something from the page to the desktop for example, the
> page will add DownloadUrl format to dataTransfer object, and in return, the
> browser will add a platform-dependent streaming file format to the clipboard.

I see, so the OS's data object will contain only the streaming format.  It seems like it would be better to provide both the streaming format and a URL because the drop target might not know how to handle the streaming format.

For example you might have an external bookmark manager that doesn't know how to accept a streaming file format, but does understand URLs.  As the site owner, you might want to put a downloadurl that is to be used for streaming but a different url for an html page that embeds the content (ie, embeds a movie or pdf file).

I thought we also had a bug about being able to download the file if you drag a link and drop while holding ctrl.  I can't seem to get any other browser to do this, so maybe I'm dreaming.

[Dmitry Titov]

> I see, so the OS's data object will contain only the streaming format.  It
> seems like it would be better to provide both the streaming format and a URL
> because the drop target might not know how to handle the streaming format.

That is all under control of the page, since it can populate the dataTransfer object with any formats.  However, perhaps it's not always beneficial to set both Url and DownloadUrl because many drop targets actually know how to deal with both - Windows Desktop will create a shortcut or download the file. For example, Email app would only add DownloadUrl in case of attachment since storing a link to it is not

I am not certain if you say the solution should be different. If so, please speak up! Otherwise, I don't see a reason not to review the patch.

[Jian Li]

Created attachment 42533 [details]
Proposed Patch

Updated the patch to account for the chromium side review feedback about the variable name.

[Tony Chang]

(In reply to comment #9)
> > I see, so the OS's data object will contain only the streaming format.  It
> > seems like it would be better to provide both the streaming format and a URL
> > because the drop target might not know how to handle the streaming format.
> 
> That is all under control of the page, since it can populate the dataTransfer
> object with any formats.  However, perhaps it's not always beneficial to set
> both Url and DownloadUrl because many drop targets actually know how to deal
> with both - Windows Desktop will create a shortcut or download the file. For
> example, Email app would only add DownloadUrl in case of attachment since
> storing a link to it is not

Right, I'm saying with this patch, it's not possible for the page to set a url
and a download url at the same time.  In fact, if you were to do a 
  event.dataTransfer.setData('downloadurl', 'http://www.foo.com/bar');
  event.dataTransfer.setData('url', 'http://www.foo.com/other');

the drop might initiate a download on www.foo.com/other, which seems to be
wrong.  I'm just saying that ChromiumDataObject needs a new KURL, not a bool. 
I'm not suggesting we should set the URL automatically when a downloadurl is
set.

[Dmitry Titov]

Comment on attachment 42533 [details]
Proposed Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        Bug 31090 - [Chromium] Add DownloadURL format to Chromium clipboard.
> +        https://bugs.webkit.org/show_bug.cgi?id=31090

I think ChangeLog could contain more info not only on what is done but also
why.
A link to a spec or mail list discussion is good to have, especially since the bug itself does not have it.

Also, need to say why no test.

> +        bool needToDownloadUrl;

Better name (or a comment) would be good here. It is unclear if the url itself should be downloaded?
Maybe 'useUrlToDownloadData'?

> +        ChromiumDataObject() : needToDownloadUrl(false) {}

Also should be set to false in ChromiumDataObject::clear()

> diff --git a/WebCore/platform/chromium/ClipboardChromium.cpp b/WebCore/platform/chromium/ClipboardChromium.cpp
>  // We provide the IE clipboard types (URL and Text), and the clipboard types specified in the WHATWG Web Applications 1.0 draft
>  // see http://www.whatwg.org/specs/web-apps/current-work/ Section 6.3.5.3

This comment is not true anymore, should be updated.

Also, I think the suggestion from Tony is good. Please consider KURL instead of bool.

r- for now, but close.

[Jian Li]

Created attachment 44647 [details]
Proposed Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44647 [details] without any errors.

[Jian Li]

Created attachment 44648 [details]
Proposed Patch

Updated the patch to add the spec link to ChangeLog.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44648 [details] without any errors.

[Dmitry Titov]

Comment on attachment 44648 [details]
Proposed Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        The proposal to whatwg can be find here:

find -> found

> diff --git a/WebCore/platform/chromium/ChromiumDataObject.h b/WebCore/platform/chromium/ChromiumDataObject.h
> +        KURL downloadURL;

The ChromiumDataObject::clear(0 and friends should get to know the downloadURL too...

[Jian Li]

Created attachment 44653 [details]
Proposed Patch

All fixed. Thanks.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44653 [details] without any errors.

[Jian Li]

Committed as http://trac.webkit.org/changeset/52160.