New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 846917 link

Starred by 6 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Cannot drag file to Linux Files until opened

Project Member Reported by tbuck...@chromium.org, May 25 2018

Issue description

Chrome Version: 68.0.3438.0
OS Version: 10715.0.0

What steps will reproduce the problem?
1. Optional: start Crostini by launching Terminal
2. Open Files.app
3. Drag a file from Downloads to Linux Files

Expected: Linux Files connects to the container and copies the file over.
Actual: File fails to copy until after you have opened Linux Files the first time.

 

Comment 1 by vapier@chromium.org, May 29 2018

Components: OS>Systems>Containers
Currently if you drag and hold for 2s, it will start the container and you can then drop.  I am investigating about making the container start immediately.  This change should be simple enough.

When starting immediately, you would still have to wait until the container is started and mounted.  This typically takes a few seconds for me.  It would be a bit tricky to allow immediate drop which then mounts the container and then copies the file once the container is started and mounted since the current code relies on getting volume information for the destination prior to starting the copy.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df70b8889254a236a74e141c84cd41fb1c607b4b

commit df70b8889254a236a74e141c84cd41fb1c607b4b
Author: Joel Hockey <joelhockey@chromium.org>
Date: Tue Jun 12 05:06:42 2018

Refactor crostini mounting code into test.mountCrostini to make it reusable

Bug: 846917
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ifd4b52472de613ed5ef1d75218ce0cf02b25fb1d
Reviewed-on: https://chromium-review.googlesource.com/1096576
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566320}
[modify] https://crrev.com/df70b8889254a236a74e141c84cd41fb1c607b4b/ui/file_manager/file_manager/test/crostini.js
[modify] https://crrev.com/df70b8889254a236a74e141c84cd41fb1c607b4b/ui/file_manager/file_manager/test/js/chrome_file_manager.js
[modify] https://crrev.com/df70b8889254a236a74e141c84cd41fb1c607b4b/ui/file_manager/file_manager/test/js/test_util.js

Labels: M-69
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28744569f6d88d923529366ac2e18bbdda830680

commit 28744569f6d88d923529366ac2e18bbdda830680
Author: Joel Hockey <joelhockey@chromium.org>
Date: Wed Jun 13 02:28:07 2018

FilesApp mount crostini immediately for drag and drop

Currently FileTransferController starts a 2s timer to change to
a directory if you drag and then hover over a root.  This happens
on the dragenter event.  The dragleave event will cancel the timer
if it has not already fired.

For crostini, we want to change to the directory immediately.

Test fires dragenter and then dragleave events immediately after each
other.  Usually this would cause no change if the events are
received within 2s of each other.  We test for crostini that even
if these events are fired right after each other, we still change
directory.

Bug: 846917
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I4f0f463ff1c4bc164328a2764ff235ee399d6e4c
Reviewed-on: https://chromium-review.googlesource.com/1096801
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566693}
[modify] https://crrev.com/28744569f6d88d923529366ac2e18bbdda830680/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/28744569f6d88d923529366ac2e18bbdda830680/chrome/browser/chromeos/extensions/file_manager/private_api_strings.cc
[modify] https://crrev.com/28744569f6d88d923529366ac2e18bbdda830680/ui/file_manager/file_manager/foreground/js/empty_folder_controller.js
[modify] https://crrev.com/28744569f6d88d923529366ac2e18bbdda830680/ui/file_manager/file_manager/foreground/js/file_transfer_controller.js
[modify] https://crrev.com/28744569f6d88d923529366ac2e18bbdda830680/ui/file_manager/file_manager/test/crostini.js
[modify] https://crrev.com/28744569f6d88d923529366ac2e18bbdda830680/ui/file_manager/file_manager/test/js/chrome_file_manager.js
[modify] https://crrev.com/28744569f6d88d923529366ac2e18bbdda830680/ui/file_manager/file_manager/test/js/strings.js
[modify] https://crrev.com/28744569f6d88d923529366ac2e18bbdda830680/ui/file_manager/file_manager/test/js/test_util.js

Cc: tbuck...@chromium.org
Tom, I've now got it that as soon as you drag over 'Linux Files' it will start/mount container if not already running.  However you can't drop the files right away - you have to wait a few seconds for the mounting to complete.

Let me know if this fix is OK.
Cc: weifangsun@chromium.org
Status: Started (was: Assigned)
Any updates to share here? Is the current implementation OK?
Cc: sbroslawsky@chromium.org mcirimele@chromium.org
+mcirimele, sbroslawsky

Sorry for the delay. While this is an improvement, I worry that users will still expect it to copy the file if they immediately release it on top of Linux Files the first time.

Is there some signal we can send to Files.app so that it starts mounting when Crostini is started instead of when the user first interacts with "Linux files"? Or is there some way we can let the user know to wait to drop the file?
Hi there,

A few thoughts for context. 
1. The default operation associated with dragging a file to another root or folder is for it to move, not copy. There are plenty of cases where we can't do that right now so no worries for now but Crostini should consider whether this is something to allow in the future. I would love for it to always move on default. The user can use a keyboard modifier (believe it's SHIFT) to change from move to copy. 

2. I like Tom's suggestion of mounting Crostini without waiting for the user to interact with "Linux files". This must be similar to what we do for other drives, like Google Drive?

3. If we can't do that, how about this? Rather than having the user adopt to the wait time, can we "fake" the operation and just delay it? We make it looks like we allow the drop (chip with green plus sign for ocpy), and give feedback that it's copying, even though some of that time is spent waiting for the mount? 
Labels: -Pri-1 -M-69 M-70 Pri-2
Moving to M70. Also moving to P2 since the worst case is that the file isn't copied (which is annoying, but not destructive).

Will mount SSHFS as soon as container starts up.

@mcirimele/sbroslawsky, let's discuss copy vs move.
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/047d1ba3bcb48861f937b1518d75213979a14488

commit 047d1ba3bcb48861f937b1518d75213979a14488
Author: Joel Hockey <joelhockey@chromium.org>
Date: Thu Aug 02 04:42:15 2018

Track calls to GetContainerSshKeys in FakeConciergeClient.

This will be used in a followup CL for CrostiniManager unit tests.

Bug: 846917
Change-Id: I4938a4bf75b986adb4a2dc283ffac7d0509e1c5c
Reviewed-on: https://chromium-review.googlesource.com/1158319
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580068}
[modify] https://crrev.com/047d1ba3bcb48861f937b1518d75213979a14488/chromeos/dbus/fake_concierge_client.cc
[modify] https://crrev.com/047d1ba3bcb48861f937b1518d75213979a14488/chromeos/dbus/fake_concierge_client.h

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/64ab6403e0d694c98d4da8a2dd961907e1ddf056

commit 64ab6403e0d694c98d4da8a2dd961907e1ddf056
Author: Joel Hockey <joelhockey@chromium.org>
Date: Thu Aug 02 06:47:16 2018

CrOS crostini: sshfs mount immediately when container starts

Perform sshfs mount to termina/penguin immediately when container
starts.  Mounting code has moved from the chrome.fileManagerPrivate
extension functions into CrostiniManager.

This change will make FilesApp more responsive to users who are
likely to use terminal or some other crostini app before using FilesApp.

Bug: 846917
Change-Id: I9fc246e45e54390e7a75f15a5571a24218cf2d5c
Reviewed-on: https://chromium-review.googlesource.com/1158317
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580094}
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/chromeos/crostini/crostini_manager.cc
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/chromeos/crostini/crostini_manager.h
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/chromeos/crostini/crostini_manager_unittest.cc
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/chromeos/extensions/file_manager/private_api_misc.h
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/chromeos/file_manager/path_util.cc
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/chromeos/file_manager/path_util.h
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/ui/views/crostini/crostini_installer_view.cc
[modify] https://crrev.com/64ab6403e0d694c98d4da8a2dd961907e1ddf056/chrome/browser/ui/views/crostini/crostini_installer_view.h

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4

commit 1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4
Author: Christos Froussios <cfroussios@chromium.org>
Date: Thu Aug 02 12:40:08 2018

Revert "CrOS crostini: sshfs mount immediately when container starts"

This reverts commit 64ab6403e0d694c98d4da8a2dd961907e1ddf056.

Reason for revert: Suspected of making 
CrostiniInstallerViewBrowserTest.InstallFlow very flaky on 
linux-chromeos-rel

Original change's description:
> CrOS crostini: sshfs mount immediately when container starts
> 
> Perform sshfs mount to termina/penguin immediately when container
> starts.  Mounting code has moved from the chrome.fileManagerPrivate
> extension functions into CrostiniManager.
> 
> This change will make FilesApp more responsive to users who are
> likely to use terminal or some other crostini app before using FilesApp.
> 
> Bug: 846917
> Change-Id: I9fc246e45e54390e7a75f15a5571a24218cf2d5c
> Reviewed-on: https://chromium-review.googlesource.com/1158317
> Reviewed-by: Nicholas Verne <nverne@chromium.org>
> Commit-Queue: Joel Hockey <joelhockey@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#580094}

TBR=joelhockey@chromium.org,nverne@chromium.org

Change-Id: Ib00e4417faaa482d62f03bed708dd46b1e0f7902
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 846917,  870289 
Reviewed-on: https://chromium-review.googlesource.com/1160541
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580145}
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/chromeos/crostini/crostini_manager.cc
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/chromeos/crostini/crostini_manager.h
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/chromeos/crostini/crostini_manager_unittest.cc
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/chromeos/extensions/file_manager/private_api_misc.h
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/chromeos/file_manager/path_util.cc
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/chromeos/file_manager/path_util.h
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/ui/views/crostini/crostini_installer_view.cc
[modify] https://crrev.com/1c3b10d1bacd74ecfc6cfb77e4e93b9f1fddc5c4/chrome/browser/ui/views/crostini/crostini_installer_view.h

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/114391232f315969b5f1c5cd16ee670e4a8a56a0

commit 114391232f315969b5f1c5cd16ee670e4a8a56a0
Author: Joel Hockey <joelhockey@chromium.org>
Date: Tue Aug 07 01:56:46 2018

Call SetCrostiniUIAllowedForTesting in crostini browser tests to fix dcheck

Bug: 846917
Change-Id: Ica3471cb3931a7e9368c386aca012674242e60cc
Reviewed-on: https://chromium-review.googlesource.com/1164146
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581091}
[modify] https://crrev.com/114391232f315969b5f1c5cd16ee670e4a8a56a0/chrome/browser/ui/views/crostini/crostini_browser_test_util.cc
[modify] https://crrev.com/114391232f315969b5f1c5cd16ee670e4a8a56a0/chrome/browser/ui/views/crostini/crostini_browser_test_util.h

Project Member

Comment 16 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b785be60dcb4387d89d725db914d2b70be02dc92

commit b785be60dcb4387d89d725db914d2b70be02dc92
Author: Joel Hockey <joelhockey@chromium.org>
Date: Wed Aug 08 02:51:31 2018

Reland "CrOS crostini: sshfs mount immediately when container starts"

This is a reland of 64ab6403e0d694c98d4da8a2dd961907e1ddf056

Updates:
* Fixed CrostiniInstallerView::SetupResult to not change
  kErrorOffline value.
* VolumeManager::AddSshfsCrostiniVolume ignores if crostini is
  already mounted.
* CrostiniInstallerViewBrowserTest is using a
  WaitingDiskMountManagerObserver to manage the run loop and
  ensure test runs all the way through new mounting code.
* Added a custom mount point manager to FakeCrosDisksClient
  to fake the sshfs mounting in CrostiniInstallerViewBrowserTest.

Original change's description:
> CrOS crostini: sshfs mount immediately when container starts
>
> Perform sshfs mount to termina/penguin immediately when container
> starts.  Mounting code has moved from the chrome.fileManagerPrivate
> extension functions into CrostiniManager.
>
> This change will make FilesApp more responsive to users who are
> likely to use terminal or some other crostini app before using FilesApp.
>
> Bug: 846917
> Change-Id: I9fc246e45e54390e7a75f15a5571a24218cf2d5c
> Reviewed-on: https://chromium-review.googlesource.com/1158317
> Reviewed-by: Nicholas Verne <nverne@chromium.org>
> Commit-Queue: Joel Hockey <joelhockey@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#580094}

Bug: 846917
Change-Id: I3677f88fc17031a2beaa6c82fc731a3c4545f4a8
Reviewed-on: https://chromium-review.googlesource.com/1161761
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581450}
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/chromeos/crostini/crostini_manager.cc
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/chromeos/crostini/crostini_manager.h
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/chromeos/crostini/crostini_manager_unittest.cc
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/chromeos/extensions/file_manager/private_api_misc.h
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/chromeos/file_manager/path_util.cc
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/chromeos/file_manager/path_util.h
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/chromeos/file_manager/volume_manager.cc
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/ui/views/crostini/crostini_installer_view.cc
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/ui/views/crostini/crostini_installer_view.h
[modify] https://crrev.com/b785be60dcb4387d89d725db914d2b70be02dc92/chrome/browser/ui/views/crostini/crostini_installer_view_browsertest.cc

Blocking: 880175
Joel, is this fixed already?
There have been 2 improvements:
1/ When doing drag and drop, there is no longer any delay before we trigger the container to start up.  However, there is still the delay of the container starting.  So you must hover over the target until the container starts before you can successfully drop.

2/ We now mount the container as soon as the container starts such as using the terminal or any other linux app.  This means it is more likely that when a user does drag and drop, we will have already mounted linux files.

The full solution requires modification to the files app so that it can receive a drop target on a fake item and then wait until the mount completes before doing the actual copy.  I don't expect to make a change like this any time soon.
OK. It doesn't sound like this is blocking issue 880175, is that right?
Blocking: -880175
Labels: -Pri-2 -M-70 Pri-3
Most common flows have been fixed, lowering priority for remaining work.

Sign in to add a comment