Have a dedicated /Downloads/.Trash

On a device, MyFiles/Downloads is a separate volume to MyFiles which is
bind mounted. As a result, moving a file from MyFiles/Downloads/ into
MyFiles/.Trash is a copy operation rather than move.

To fix this, we will have a dedicated MyFiles/Downloads/.Trash for items
under Downloads which guarantees that all deletes will stay fast.

Bug: 953310
Change-Id: Ib1bc6d9da2aec45e1d4b43eae25cb02b230b117f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2469645Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819682}

ui/file_manager/file_manager/background/js/trash.js

@@ -21,6 +21,24 @@ class TrashDirs {
   }
 }
 
+/**
+ * Configuration for where Trash is stored in a volume.
+ */
+class TrashConfig {
+  /**
+   * @param {VolumeManagerCommon.RootType} rootType
+   * @param {string} topDir Top directory of volume. Must end with a slash to
+   *     make comparisons simpler.
+   * @param {string} trashDir Trash directory. Must end with a slash to make
+   *     comparisons simpler.
+   */
+  constructor(rootType, topDir, trashDir) {
+    this.rootType = rootType;
+    this.topDir = topDir;
+    this.trashDir = trashDir;
+  }
+}
+
 /**
  * Result from calling Trash.removeFileOrDirectory().
  */
@@ -43,32 +61,37 @@ class TrashItem {
 class Trash {
   constructor() {
     /**
-     * Store /.Trash/files and /.Trash/info to avoid repeated lookup
-     * @private {?TrashDirs}
+     * Store TrashDirs to avoid repeated lookup.
+     * @private {!Object<string, !TrashDirs>}
+     * @const
      */
-    this.trashDirs_;
+    this.trashDirs_ = {};
   }
 
   /**
-   * Only move to trash if feature is on, and entry is in MyFiles, but not
-   * already in /.Trash.
+   * Only move to trash if feature is on, and entry is in one of the supported
+   * volumes, but not already in the trash.
    *
    * @param {!VolumeManager} volumeManager
    * @param {!Entry} entry The entry to remove.
-   * @return {boolean} True if item should be moved to trash, else false if item
-   *     should be permanently deleted.
+   * @return {?TrashConfig} Valid TrashConfig if item should be moved to trash,
+   *     else null if item should be permanently deleted.
    * @private
    */
   shouldMoveToTrash_(volumeManager, entry) {
-    if (loadTimeData.getBoolean('FILES_TRASH_ENABLED')) {
-      const info = volumeManager.getLocationInfo(entry);
-      const entryInTrash =
-          entry.fullPath === '/.Trash' || entry.fullPath.startsWith('/.Trash/');
-      return !!info &&
-          info.rootType === VolumeManagerCommon.RootType.DOWNLOADS &&
-          !entryInTrash;
+    const info = volumeManager.getLocationInfo(entry);
+    if (!loadTimeData.getBoolean('FILES_TRASH_ENABLED') || !info) {
+      return null;
     }
-    return false;
+    const fullPathSlash = entry.fullPath + '/';
+    for (const config of Trash.CONFIG) {
+      const entryInVolume = fullPathSlash.startsWith(config.topDir);
+      if (config.rootType === info.rootType && entryInVolume) {
+        const entryInTrash = fullPathSlash.startsWith(config.trashDir);
+        return entryInTrash ? null : config;
+      }
+    }
+    return null;
   }
 
   /**
@@ -83,11 +106,13 @@ class Trash {
    *     is removed, rejects with DOMError.
    */
   removeFileOrDirectory(volumeManager, entry, permanentlyDelete) {
-    if (!permanentlyDelete && this.shouldMoveToTrash_(volumeManager, entry)) {
-      return this.trashLocalFileOrDirectory_(volumeManager, entry);
-    } else {
-      return this.permanentlyDeleteFileOrDirectory_(entry);
+    if (!permanentlyDelete) {
+      const config = this.shouldMoveToTrash_(volumeManager, entry);
+      if (config) {
+        return this.trashFileOrDirectory_(entry, config);
+      }
     }
+    return this.permanentlyDeleteFileOrDirectory_(entry);
   }
 
   /**
@@ -108,30 +133,32 @@ class Trash {
   }
 
   /**
-   * Get /.Trash/files and /.Trash/info directories.
+   * Get trash files and info directories.
    *
-   * @param {!VolumeManager} volumeManager
+   * @param {!Entry} entry The entry to remove.
+   * @param {!TrashConfig} config
    * @return {!Promise<!TrashDirs>} Promise which resolves with trash dirs.
    * @private
    */
-  getTrashDirs_(volumeManager) {
-    if (this.trashDirs_) {
-      return Promise.resolve(this.trashDirs_);
+  async getTrashDirs_(entry, config) {
+    const key = `${config.rootType}-${config.topDir}`;
+    let trashDirs = this.trashDirs_[key];
+    if (trashDirs) {
+      return trashDirs;
     }
 
-    const downloads = volumeManager.getCurrentProfileVolumeInfo(
-        VolumeManagerCommon.VolumeType.DOWNLOADS);
-    const root = downloads.fileSystem.root;
-    return new Promise((resolve, reject) => {
-      root.getDirectory('.Trash', {create: true}, (trashRoot) => {
-        trashRoot.getDirectory('files', {create: true}, (trashFiles) => {
-          trashRoot.getDirectory('info', {create: true}, trashInfo => {
-            this.trashDirs_ = new TrashDirs(trashFiles, trashInfo);
-            resolve(this.trashDirs_);
-          }, reject);
-        }, reject);
-      }, reject);
-    });
+    let trashRoot = entry.filesystem.root;
+    const parts = config.trashDir.split('/');
+    for (const part of parts) {
+      if (part) {
+        trashRoot = await this.getDirectory_(trashRoot, part);
+      }
+    }
+    const trashFiles = await this.getDirectory_(trashRoot, 'files');
+    const trashInfo = await this.getDirectory_(trashRoot, 'info');
+    trashDirs = new TrashDirs(trashFiles, trashInfo);
+    this.trashDirs_[key] = trashDirs;
+    return trashDirs;
   }
 
   /**
@@ -164,6 +191,21 @@ class Trash {
     return this.moveTo_(tmpFile, trashInfoDir, finalName);
   }
 
+  /**
+   * Promise wrapper for FileSystemDirectoryEntry.getDirectory().
+   *
+   * @param {!DirectoryEntry} dirEntry current directory.
+   * @param {string} path name of directory within dirEntry.
+   * @return {!Promise<!DirectoryEntry>} Promise which resolves with
+   *     <dirEntry>/<path>.
+   * @private
+   */
+  getDirectory_(dirEntry, path) {
+    return new Promise((resolve, reject) => {
+      dirEntry.getDirectory(path, {create: true}, resolve, reject);
+    });
+  }
+
   /**
    * Promise wrapper for FileSystemEntry.moveTo().
    *
@@ -181,16 +223,15 @@ class Trash {
   }
 
   /**
-   * Move a file or a directory in the local DOWNLOADS volume to
-   * the trash.
+   * Move a file or a directory to the trash.
    *
-   * @param {!VolumeManager} volumeManager
    * @param {!Entry} entry The entry to remove.
+   * @param {!TrashConfig} config trash config for entry.
    * @return {!Promise<!TrashItem>}
    * @private
    */
-  async trashLocalFileOrDirectory_(volumeManager, entry) {
-    const trashDirs = await this.getTrashDirs_(volumeManager);
+  async trashFileOrDirectory_(entry, config) {
+    const trashDirs = await this.getTrashDirs_(entry, config);
     const name =
         await fileOperationUtil.deduplicatePath(trashDirs.files, entry.name);
 
@@ -226,13 +267,8 @@ class Trash {
 
     // Move to last directory in path, making sure dirs are created if needed.
     let dir = trashItem.filesEntry.filesystem.root;
-    const cd = (directory, path) => {
-      return new Promise((resolve, reject) => {
-        directory.getDirectory(path, {create: true}, resolve, reject);
-      });
-    };
     for (let i = 0; i < parts.length - 1; i++) {
-      dir = await cd(dir, parts[i]);
+      dir = await this.getDirectory_(dir, parts[i]);
     }
 
     // Restore filesEntry first, then remove its trash infoEntry.
@@ -246,3 +282,19 @@ class Trash {
     await this.permanentlyDeleteFileOrDirectory_(trashItem.infoEntry);
   }
 }
+
+/**
+ * Volumes supported for Trash, and location of Trash dir. Items will be
+ * searched in order.
+ *
+ * @type {!Array<!TrashConfig>}
+ */
+Trash.CONFIG = [
+  // MyFiles/Downloads is a separate volume on a physical device, and doing a
+  // move from MyFiles/Downloads/<path> to MyFiles/.Trash actually does a
+  // copy across volumes, so we have a dedicated MyFiles/Downloads/.Trash.
+  new TrashConfig(
+      VolumeManagerCommon.RootType.DOWNLOADS, '/Downloads/',
+      '/Downloads/.Trash/'),
+  new TrashConfig(VolumeManagerCommon.RootType.DOWNLOADS, '/', '/.Trash/'),
+];

ui/file_manager/file_manager/background/js/trash_unittest.js

@@ -9,7 +9,9 @@ let volumeManager;
 function setUp() {
   // Mock LoadTimeData strings.
   window.loadTimeData = {
-    data: {},
+    data: {
+      'FILES_TRASH_ENABLED': true,
+    },
     getBoolean: function(key) {
       return window.loadTimeData.data[key];
     },
@@ -24,7 +26,7 @@ function setUp() {
  * we correctly either permanently delete, or move to trash.
  *
  * @suppress {accessControls} Access private functions
- * permanentlyDeleteFileOrDirectory_() and trashLocalFileOrDirectory_().
+ * permanentlyDeleteFileOrDirectory_() and trashFileOrDirectory_().
  */
 function checkRemoveFileOrDirectory(
     filesTrashEnabled, rootType, path, deletePermanently,
@@ -35,21 +37,21 @@ function checkRemoveFileOrDirectory(
   const f = MockFileEntry.create(volumeInfo.fileSystem, path);
 
   const trash = new Trash();
-  // Detect whether permanentlyDelete..., or trashLocal... is called.
+  // Detect whether permanentlyDelete..., or trash... is called.
   let permanentlyDeleteCalled = false;
-  let trashLocalCalled = false;
+  let trashCalled = false;
   trash.permanentlyDeleteFileOrDirectory_ = () => {
     permanentlyDeleteCalled = true;
     return Promise.resolve();
   };
-  trash.trashLocalFileOrDirectory_ = (volumeManager, entry) => {
-    trashLocalCalled = true;
+  trash.trashFileOrDirectory_ = (volumeManager, entry) => {
+    trashCalled = true;
     return Promise.resolve();
   };
 
   trash.removeFileOrDirectory(volumeManager, f, deletePermanently);
   assertEquals(expectPermanentlyDelete, permanentlyDeleteCalled);
-  assertEquals(!expectPermanentlyDelete, trashLocalCalled);
+  assertEquals(!expectPermanentlyDelete, trashCalled);
 }
 
 /**
@@ -104,12 +106,11 @@ async function testPermanentlyDeleteFileOrDirectory(done) {
 }
 
 /**
- * Test trashLocalFileOrDirectory_().
- *
- * @suppress {accessControls} Access trashLocalFileOrDirectory_().
+ * Test trash in MyFiles.
  */
-async function testTrashLocalFileOrDirectory(done) {
+async function testMyFilesTrash(done) {
   const trash = new Trash();
+  const deletePermanently = false;
   const downloads = volumeManager.getCurrentProfileVolumeInfo(
       VolumeManagerCommon.VolumeType.DOWNLOADS);
   const fs = downloads.fileSystem;
@@ -123,7 +124,7 @@ async function testTrashLocalFileOrDirectory(done) {
   // /.Trash/info.
   assertEquals(5, Object.keys(fs.entries).length);
   assertTrue(!!fs.entries['/dir/file1']);
-  await trash.trashLocalFileOrDirectory_(volumeManager, file1);
+  await trash.removeFileOrDirectory(volumeManager, file1, deletePermanently);
   assertFalse(!!fs.entries['/dir/file1']);
   assertTrue(fs.entries['/.Trash/files'].isDirectory);
   assertTrue(fs.entries['/.Trash/info'].isDirectory);
@@ -137,7 +138,7 @@ async function testTrashLocalFileOrDirectory(done) {
 
   // Trashed dir should also move children files into /.Trash/files.
   assertTrue(!!fs.entries['/dir']);
-  await trash.trashLocalFileOrDirectory_(volumeManager, dir);
+  await trash.removeFileOrDirectory(volumeManager, dir, deletePermanently);
   assertFalse(!!fs.entries['/dir']);
   assertFalse(!!fs.entries['/dir/file2']);
   assertFalse(!!fs.entries['/dir/file3']);
@@ -158,11 +159,63 @@ async function testTrashLocalFileOrDirectory(done) {
   done();
 }
 
+/**
+ * Test that Downloads has its own /Downloads/.Trash since it is a separate
+ * mount on a device and we don't want move to trash to be a copy operation.
+ *
+ * @suppress {accessControls} Access shouldMoveToTrash_().
+ */
+async function testDownloadsHasOwnTrash(done) {
+  const trash = new Trash();
+  const deletePermanently = false;
+  const downloads = volumeManager.getCurrentProfileVolumeInfo(
+      VolumeManagerCommon.VolumeType.DOWNLOADS);
+  const fs = downloads.fileSystem;
+  const file1 = MockFileEntry.create(fs, '/file1', null, new Blob(['f1']));
+  const dir2 = MockDirectoryEntry.create(fs, '/Downloads');
+  const file2 =
+      MockFileEntry.create(fs, '/Downloads/file2', null, new Blob(['f1']));
+  assertEquals(4, Object.keys(fs.entries).length);
+
+  // Move /file1 to trash.
+  await trash.removeFileOrDirectory(volumeManager, file1, deletePermanently);
+  assertTrue(fs.entries['/.Trash'].isDirectory);
+  assertTrue(fs.entries['/.Trash/files'].isDirectory);
+  assertTrue(fs.entries['/.Trash/info'].isDirectory);
+  assertTrue(fs.entries['/.Trash/files/file1'].isFile);
+  assertTrue(fs.entries['/.Trash/info/file1.trashinfo'].isFile);
+  assertEquals(8, Object.keys(fs.entries).length);
+
+  // Move /files2 (in Downloads to trash.
+  await trash.removeFileOrDirectory(volumeManager, file2, deletePermanently);
+  assertTrue(fs.entries['/Downloads/.Trash'].isDirectory);
+  assertTrue(fs.entries['/Downloads/.Trash/files'].isDirectory);
+  assertTrue(fs.entries['/Downloads/.Trash/info'].isDirectory);
+  assertTrue(fs.entries['/Downloads/.Trash/files/file2'].isFile);
+  assertTrue(fs.entries['/Downloads/.Trash/info/file2.trashinfo'].isFile);
+  assertEquals(12, Object.keys(fs.entries).length);
+
+  // Delete /Downloads/.Trash/files/file2.
+  const file2Trashed = fs.entries['/Downloads/.Trash/files/file2'];
+  assertFalse(!!trash.shouldMoveToTrash_(volumeManager, file2Trashed));
+  await trash.removeFileOrDirectory(
+      volumeManager, file2Trashed, deletePermanently);
+  assertEquals(11, Object.keys(fs.entries).length);
+
+  // Delete /Downloads/.Trash.
+  const downloadsTrash = fs.entries['/Downloads/.Trash'];
+  assertFalse(!!trash.shouldMoveToTrash_(volumeManager, downloadsTrash));
+  await trash.removeFileOrDirectory(
+      volumeManager, downloadsTrash, deletePermanently);
+  assertEquals(7, Object.keys(fs.entries).length);
+
+  done();
+}
+
 /**
  * Test restore().
  */
 async function testRestore(done) {
-  window.loadTimeData.data['FILES_TRASH_ENABLED'] = true;
   const trash = new Trash();
   const deletePermanently = false;
   const downloads = volumeManager.getCurrentProfileVolumeInfo(

ui/file_manager/integration_tests/file_manager/trash.js

@@ -33,24 +33,20 @@ testcase.trashMoveToTrash = async () => {
 
   // Wait for menu item to appear.
   await remoteCall.waitForElement(
-      appId, '#gear-menu-toggle-hidden-files:not([disabled])');
-
-  // Wait for menu item to appear.
-  await remoteCall.waitForElement(
-      appId, '#gear-menu-toggle-hidden-files:not([checked])');
+      appId, '#gear-menu-toggle-hidden-files:not([disabled]):not([checked])');
 
   // Click the menu item.
   await remoteCall.callRemoteTestUtil(
       'fakeMouseClick', appId, ['#gear-menu-toggle-hidden-files']);
 
-  // Navigate to /My files/.Trash/files.
-  await navigateWithDirectoryTree(appId, '/My files/.Trash/files');
+  // Navigate to /My files/Downloads/.Trash/files.
+  await navigateWithDirectoryTree(appId, '/My files/Downloads/.Trash/files');
 
   // Ensure hello.txt exists.
   await remoteCall.waitForElement(appId, '#file-list [file-name="hello.txt"]');
 
   // Navigate to /My files/.Trash/files.
-  await navigateWithDirectoryTree(appId, '/My files/.Trash/info');
+  await navigateWithDirectoryTree(appId, '/My files/Downloads/.Trash/info');
 
   // Ensure hello.txt.trashinfo exists.
   await remoteCall.waitForElement(