Bug 31090 - [Chromium] Add DownloadURL format to Chromium clipboard.
Summary: [Chromium] Add DownloadURL format to Chromium clipboard.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-03 15:36 PST by Jian Li
Modified: 2009-12-16 10:34 PST (History)
5 users (show)

See Also:


Attachments
Proposed Patch (2.91 KB, patch)
2009-11-03 15:48 PST, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (2.91 KB, patch)
2009-11-04 17:38 PST, Jian Li
dimich: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (4.26 KB, patch)
2009-12-10 16:18 PST, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (4.39 KB, patch)
2009-12-10 16:25 PST, Jian Li
dimich: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (5.82 KB, patch)
2009-12-10 17:47 PST, Jian Li
dimich: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-11-03 15:36:56 PST
Add DownloadURL format to Chromium clipboard in order to support dragging a virtual file out of the browser.
Comment 1 Tony Chang 2009-11-03 15:42:46 PST
Out of curiosity, what is an example flow that is currently missing?  ctrl+drag of a link to the desktop?
Comment 2 Jian Li 2009-11-03 15:48:59 PST
Created attachment 42432 [details]
Proposed Patch
Comment 3 Jian Li 2009-11-03 15:53:39 PST
(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.
Comment 4 Tony Chang 2009-11-03 16:09:05 PST
(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?
Comment 5 Jian Li 2009-11-03 16:21:27 PST
(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.
Comment 6 Tony Chang 2009-11-03 16:34:26 PST
(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?
Comment 7 Dmitry Titov 2009-11-03 22:10:52 PST
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.
Comment 8 Tony Chang 2009-11-04 10:21:01 PST
(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.
Comment 9 Dmitry Titov 2009-11-04 17:32:04 PST
> 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.
Comment 10 Jian Li 2009-11-04 17:38:39 PST
Created attachment 42533 [details]
Proposed Patch

Updated the patch to account for the chromium side review feedback about the variable name.
Comment 11 Tony Chang 2009-11-04 17:42:03 PST
(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.
Comment 12 Dmitry Titov 2009-11-04 17:51:53 PST
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.
Comment 13 Jian Li 2009-12-10 16:18:00 PST
Created attachment 44647 [details]
Proposed Patch
Comment 14 WebKit Review Bot 2009-12-10 16:18:17 PST
style-queue ran check-webkit-style on attachment 44647 [details] without any errors.
Comment 15 Jian Li 2009-12-10 16:25:45 PST
Created attachment 44648 [details]
Proposed Patch

Updated the patch to add the spec link to ChangeLog.
Comment 16 WebKit Review Bot 2009-12-10 16:28:41 PST
style-queue ran check-webkit-style on attachment 44648 [details] without any errors.
Comment 17 Dmitry Titov 2009-12-10 17:32:27 PST
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...
Comment 18 Jian Li 2009-12-10 17:47:10 PST
Created attachment 44653 [details]
Proposed Patch

All fixed. Thanks.
Comment 19 WebKit Review Bot 2009-12-10 17:50:29 PST
style-queue ran check-webkit-style on attachment 44653 [details] without any errors.
Comment 20 Jian Li 2009-12-16 10:34:33 PST
Committed as http://trac.webkit.org/changeset/52160.