Items in the Downloads page should be loaded like a normal OS file when dragg...

Items in the Downloads page should be loaded like a normal OS file when dragged to a Tab content area. We need the URL to be set to the file path in the IDataObject so that WebDropData can properly handle the file.

BUG=9266
TEST=Drag an item with a supported MIME type from the Downloads page to a Tab content area. It should load within the browser. Drag an item with an unsupported MIME type and it should not open it.

[Evan Stade, 2009-08-07 02:47:12]

Paul, could you take a look?

http://codereview.chromium.org/164119/diff/5/1005
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/164119/diff/5/1005#newcode251
Line 251: download->file_name().ToWStringHack());
this can just be .value() instead of .ToWStringHack() since we are inside a
windows-only block

[pierre.lafayette, 2009-08-07 03:34:32]

Never liked those *Hack functions.

[Paul Godavari, 2009-08-07 20:52:00]

Would you explain the use case this patch solves (provide an example of why we'd
want this)?

When I tested your patch, I was able to drag a download to a tab contents page
which then re-downloaded it leaving me with a second copy of the original
download. That seems very wrong.

[pierre.lafayette, 2009-08-07 21:05:24]

Sorry I wasn't clear in the TEST section. Try dragging an image you've
downloaded or a text file etc. It should open in the browser. The bug is
http://crbug.com/9266.

[Paul Godavari, 2009-08-07 21:27:04]

That use case sounds reasonable to me, but the side effect of your change is
re-downloading exe and other file types when dragged onto the tab contents. Can
you make this change work for only file types that can be displayed / loaded?

[pierre.lafayette, 2009-08-07 23:35:28]

Okay, Lei pointed me to mime_util. Now only supported types will have the
additional URL information.

[Paul Godavari, 2009-08-08 01:18:32]

LGTM, thanks for the mime fix.

Just a few nits to fix, listed below.

I noticed a new bug exposed by this patch, which is that if the TabContents
can't load the dragged file, the drag cursor should reflect that the drop won't
be allowed. As it is, we still show the 'accept' drag cursor. Can you file a
separate bug to fix that (it doesn't have to be addressed in this patch)?

http://codereview.chromium.org/164119/diff/13/1015
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/164119/diff/13/1015#newcode16
Line 16: #include "net/base/mime_util.h"
nit: #includes are in alphabetical order by path component, so this needs to be
just before the skia include.

http://codereview.chromium.org/164119/diff/13/1015#newcode249
Line 249: std::string mime_type = download->mime_type();
nit: Chrome style uses variable declarations in the smallest scope possible, so
'mime_type' should be declared just after SetFilename().

http://codereview.chromium.org/164119/diff/13/1015#newcode259
Line 259: // Add URL so that we can load supported files when dragged
Nit: Comments should use as much of the 80 column limit as possible.

http://codereview.chromium.org/164119/diff/13/1015#newcode260
Line 260: // to Tab content areas.
nit: 'TabContents' not 'Tab contents'.

[pierre.lafayette, 2009-08-08 02:33:23]

Done. I've created the second bug at http://crbug.com/18818.

[pierre.lafayette, 2009-08-08 02:41:09]

Do you think it's a bug that dragging a local file with unsupported MIME type
from Windows to the TabContents downloads the file?

[pierre.lafayette, 2009-08-09 17:27:50]

We could only add the URL to the data when the file is of a certain type. Is
there an enum or something that lists files that can be opened within the
browser?

2009/8/7 <paul@chromium.org>

> That use case sounds reasonable to me, but the side effect of your
> change is re-downloading exe and other file types when dragged onto the
> tab contents. Can you make this change work for only file types that can
> be displayed / loaded?
>
>
> http://codereview.chromium.org/164119
>



-- 
Pierre.