Support drag and drop of files with filesystem: URL

Support drag and drop of files with filesystem: URL

Filter filesystem files in RenderViewHostImpl::OnStartDragging.

Create isolated filesystem for each filesystem file in RenderViewHostImpl::DragTargetDragEnter.

Add file_system_files to content::DropData.

Wire information from blink::WebDragData to ui::OSExchangeData.
  blink::WebDragData -[drop_data_builder.cc]-> content::DropData -[web_contents_view_aura.cc]-> ui::OSExchangeData

Wire information from ui::OSExchangeData to blink::WebDragData.
  ui::OSExchangeData -[web_contents_view_aura.cc]-> content::DropData -[render_view_impl.cc]-> blink::WebDragData

BUG=126766
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265616

[kinuko, 2014-04-21 03:31:38]

(Also +cc tzik@ to share contexts with slightly wider ppl)

https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1400:
filtered_data.file_system_files.push_back(drop_data.file_system_files[i]);
This is where we start drag operation, is it right?  I have a feeling that it
would be cleaner to create an Isolated filesystem here and pass files with
Isolated filesystem ids (rather than receiving filesystem URLs and creating
Isolated filesystems there), though currently Isolated filesystem's lifetime
management is tied to the process that created it and we may need to implement
correct ref-counting during drag-and-drop. (Though-- it might be ok to just let
it go if the sender side's window is closed? Native drag-and-drop somewhat works
like it)
Wdyt?

[kinuko, 2014-04-21 05:05:36]

On 2014/04/21 03:31:38, kinuko wrote:
> (Also +cc tzik@ to share contexts with slightly wider ppl)
> 
>
https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
> content/browser/renderer_host/render_view_host_impl.cc:1400:
> filtered_data.file_system_files.push_back(drop_data.file_system_files[i]);
> This is where we start drag operation, is it right?  I have a feeling that it
> would be cleaner to create an Isolated filesystem here and pass files with
> Isolated filesystem ids (rather than receiving filesystem URLs and creating
> Isolated filesystems there), though currently Isolated filesystem's lifetime
> management is tied to the process that created it and we may need to implement
> correct ref-counting during drag-and-drop. (Though-- it might be ok to just
let
> it go if the sender side's window is closed? Native drag-and-drop somewhat
works
> like it)
> Wdyt?

Um, wait, I might be missing some contexts... would it need to be passed across
the browser process boundary?

[kinuko, 2014-04-21 06:00:48]

On 2014/04/21 05:05:36, kinuko wrote:
> On 2014/04/21 03:31:38, kinuko wrote:
> > (Also +cc tzik@ to share contexts with slightly wider ppl)
> > 
> >
>
https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
> > File content/browser/renderer_host/render_view_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
> > content/browser/renderer_host/render_view_host_impl.cc:1400:
> > filtered_data.file_system_files.push_back(drop_data.file_system_files[i]);
> > This is where we start drag operation, is it right?  I have a feeling that
it
> > would be cleaner to create an Isolated filesystem here and pass files with
> > Isolated filesystem ids (rather than receiving filesystem URLs and creating
> > Isolated filesystems there), though currently Isolated filesystem's lifetime
> > management is tied to the process that created it and we may need to
implement
> > correct ref-counting during drag-and-drop. (Though-- it might be ok to just
> let
> > it go if the sender side's window is closed? Native drag-and-drop somewhat
> works
> > like it)
> > Wdyt?
> 
> Um, wait, I might be missing some contexts... would it need to be passed
across
> the browser process boundary?

(Per offline chat) the browser process should be always same, and since the
current file handling is similar to what we do for local files (i.e. so we
always trust what clipboard says) I assume we should be ok with this design.

[kinuko, 2014-04-21 06:08:27]

Ok, from filesystem pov I think this lgtm. Thanks!

https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:792:
fileapi::FileSystemURL file_system_url =
Could we have a brief comment to note that this URL's origin part could be
different from the receiver's one if it's from another site?

[hashimoto, 2014-04-21 08:12:45]

https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/242643004/diff/30001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:792:
fileapi::FileSystemURL file_system_url =
On 2014/04/21 06:08:27, kinuko wrote:
> Could we have a brief comment to note that this URL's origin part could be
> different from the receiver's one if it's from another site?

Done.

[hashimoto, 2014-04-21 08:38:33]

jam@, could you take a look at this change?

[jam, 2014-04-22 17:11:27]

lgtm

https://codereview.chromium.org/242643004/diff/60001/content/public/common/dr...
File content/public/common/drop_data.cc (right):

https://codereview.chromium.org/242643004/diff/60001/content/public/common/dr...
content/public/common/drop_data.cc:9:
DropData::FileSystemFileInfo::FileSystemFileInfo() : size(0) {}
nit: i think you can inline this and the dtor and clang won't complain?

[hashimoto, 2014-04-23 03:42:37]

https://codereview.chromium.org/242643004/diff/60001/content/public/common/dr...
File content/public/common/drop_data.cc (right):

https://codereview.chromium.org/242643004/diff/60001/content/public/common/dr...
content/public/common/drop_data.cc:9:
DropData::FileSystemFileInfo::FileSystemFileInfo() : size(0) {}
On 2014/04/22 17:11:28, jam wrote:
> nit: i think you can inline this and the dtor and clang won't complain?

I thought it would because GURL is not POD.
However, it seems it's OK as a tryjob with ctor/dtor inlined for
linux_chromium_chromeos_clang_dbg passed without failures.
Done.

[hashimoto, 2014-04-23 03:42:41]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-04-23 03:43:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/242643004/100001

[commit-bot: I haz the power, 2014-04-23 09:38:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-23 09:38:17]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[hashimoto, 2014-04-23 09:48:04]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-04-23 09:49:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/242643004/100001

[commit-bot: I haz the power, 2014-04-23 12:19:51]

Change committed as 265616