Include file contents in drag exchange on aura

Include file contents in drag exchange on aura

This makes dragging an image to a folder save the contents, rather than
just saving a .url file.

R=sky@chromium.org
BUG=280238
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221840

[scottmg, 2013-09-05 00:00:55]

oops, jochen is ooo, +sky.

[sky, 2013-09-05 15:42:02]

Is it possible to write tests for this?

https://codereview.chromium.org/23591026/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/23591026/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_view_aura.cc:239: void
PrepareDragForFileContents(const DropData& drop_data,
Add a description.

https://codereview.chromium.org/23591026/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_view_aura.cc:241: static const int
kMaxFilenameLength = 255;  // FAT and NTFS
Isn't this win specific? Also, isn't there an OS constant we shuld be using?

https://codereview.chromium.org/23591026/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_view_aura.cc:253: 0,
kMaxFilenameLength - extension.size()));
What if extension.size() > kMaxFilenameLength?

[scottmg, 2013-09-05 20:01:54]

On 2013/09/05 15:42:02, sky wrote:
> Is it possible to write tests for this?

I guess in interactive_ui_tests as it needs to drag out of chrome to the desktop
or a folder. I'll try to do that.

> 
>
https://codereview.chromium.org/23591026/diff/1/content/browser/web_contents/...
> File content/browser/web_contents/web_contents_view_aura.cc (right):
> 
>
https://codereview.chromium.org/23591026/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_view_aura.cc:239: void
> PrepareDragForFileContents(const DropData& drop_data,
> Add a description.
> 
>
https://codereview.chromium.org/23591026/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_view_aura.cc:241: static const int
> kMaxFilenameLength = 255;  // FAT and NTFS
> Isn't this win specific? Also, isn't there an OS constant we shuld be using?
> 
>
https://codereview.chromium.org/23591026/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_view_aura.cc:253: 0,
> kMaxFilenameLength - extension.size()));
> What if extension.size() > kMaxFilenameLength?

Yeah. :( I ported this from the _win version, but it seems a bit complex for
what it's doing. I'll try to simplify.

[sky, 2013-09-05 20:41:46]

Ugh. IF you need an interactive test don't bother. Maybe the code could be
restructured to make it more testable? Don't feel you have to do that, just
grumbling.

[scottmg, 2013-09-06 21:35:35]

On 2013/09/05 20:41:46, sky wrote:
> Ugh. IF you need an interactive test don't bother. Maybe the code could be
> restructured to make it more testable? Don't feel you have to do that, just
> grumbling.

I looked at adding a test to content_unittests, but the drag client is mocked
out so the code doesn't get here.

For the max length, the old code didn't really make any sense: it was limiting
the file *name* length, but the real limit is on the entire path length, so we
can't avoid making it too long at this point.

In particular, if we actually exercised that code path and truncated (limiting
it to MAX_PATH) then the drop would still fail because
c:\desktop\<long_filename_that's_MAX_PATH_long> would exceed the limit, so
Windows fails the drop anyway. Since we don't know anything about where the drop
will be, we can't usefully limit the path name, except by choosing some
arbitrary quite short value. But that seems annoying too as you might lose
useful information in the file name.

The failure case is that the drop silently fails and no file is saved, which
isn't great but not catastrophic either.

[sky, 2013-09-06 22:17:03]

On Fri, Sep 6, 2013 at 2:35 PM,  <scottmg@chromium.org> wrote:
> On 2013/09/05 20:41:46, sky wrote:
>>
>> Ugh. IF you need an interactive test don't bother. Maybe the code could be
>> restructured to make it more testable? Don't feel you have to do that,
>> just
>> grumbling.
>
>
> I looked at adding a test to content_unittests, but the drag client is
> mocked
> out so the code doesn't get here.
>
> For the max length, the old code didn't really make any sense: it was
> limiting
> the file *name* length, but the real limit is on the entire path length, so
> we
> can't avoid making it too long at this point.

My worry was more overflow.

  -Scott

> In particular, if we actually exercised that code path and truncated
> (limiting
> it to MAX_PATH) then the drop would still fail because
> c:\desktop\<long_filename_that's_MAX_PATH_long> would exceed the limit, so
> Windows fails the drop anyway. Since we don't know anything about where the
> drop
> will be, we can't usefully limit the path name, except by choosing some
> arbitrary quite short value. But that seems annoying too as you might lose
> useful information in the file name.
>
> The failure case is that the drop silently fails and no file is saved, which
> isn't great but not catastrophic either.
>
> https://codereview.chromium.org/23591026/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-09-06 22:17:08]

LGTM

[commit-bot: I haz the power, 2013-09-06 22:18:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/23591026/19001

[commit-bot: I haz the power, 2013-09-07 00:55:39]

Change committed as 221840