Fix some more with flag MyFilesVolume enabled

Change EntryList method from children to use getUIChildren so it has
the same API as VolumeEntry.

Change util.isDescendantEntry to check getUIChildren for EntryList and
VolumeEntry if the filesystem of both entris are different. These 2
types contain entries with different filesystem thus we should check
those.

Add toEntryList helper just to be able to check that an entry has the
method getUIChildren without closure complaining that Entry and
FilesAppEntry don't have such attribute, this method is only available
in EntryList and VolumeEntry.

Change directory_tree EntryListItem to add the volume type attribute
to have it compatible with and without MyFilesVolume flag.

Change file_display.js selector to use volume-type-icon makes downloads
folder selector compatible with/without MyFilesVolume flag.

Change FileManager to check if MyFiles actually exist before assigning
to nextCurrentDirEntry, this happens if MyFiles is a volume and isn't
mounted. Also in this condition (MyFiles being a volume but not
mounted) change NavigationListModel to create a EntryList for MyFiles
so it can display Linux or Play files if they exist.

before this change.

Test: Enabled a few tests with MyFilesVolume flag that were broken
Bug: 873539
Change-Id: I66d2b95d5ea5897c2f2db558f46b0bcfcfd0c900
Reviewed-on: https://chromium-review.googlesource.com/c/1345708
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610505}

chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

@@ -612,20 +612,44 @@ WRAPPED_INSTANTIATE_TEST_CASE_P(
     FilesAppBrowserTest,
     ::testing::Values(
         EventCase("tabindexSearchBoxFocus"),
+        EventCase("tabindexSearchBoxFocus").EnableMyFilesVolume(),
         EventCase("tabindexFocus"),
+        EventCase("tabindexFocus").EnableMyFilesVolume(),
         EventCase("tabindexFocusDownloads"),
+        EventCase("tabindexFocusDownloads").EnableMyFilesVolume(),
         EventCase("tabindexFocusDownloads").InGuestMode(),
+        EventCase("tabindexFocusDownloads").InGuestMode().EnableMyFilesVolume(),
         EventCase("tabindexFocusDirectorySelected"),
+        EventCase("tabindexFocusDirectorySelected").EnableMyFilesVolume(),
         EventCase("tabindexOpenDialogDrive").WithBrowser().DisableDriveFs(),
         EventCase("tabindexOpenDialogDrive").WithBrowser().EnableDriveFs(),
+        EventCase("tabindexOpenDialogDrive")
+            .WithBrowser()
+            .EnableDriveFs()
+            .EnableMyFilesVolume(),
         EventCase("tabindexOpenDialogDownloads").WithBrowser(),
+        EventCase("tabindexOpenDialogDownloads")
+            .WithBrowser()
+            .EnableMyFilesVolume(),
         EventCase("tabindexOpenDialogDownloads").WithBrowser().InGuestMode(),
+        EventCase("tabindexOpenDialogDownloads")
+            .WithBrowser()
+            .InGuestMode()
+            .EnableMyFilesVolume(),
         EventCase("tabindexSaveFileDialogDrive").WithBrowser().DisableDriveFs(),
         EventCase("tabindexSaveFileDialogDrive").WithBrowser().EnableDriveFs(),
+        EventCase("tabindexSaveFileDialogDrive")
+            .WithBrowser()
+            .EnableDriveFs()
+            .EnableMyFilesVolume(),
         EventCase("tabindexSaveFileDialogDownloads").WithBrowser(),
         EventCase("tabindexSaveFileDialogDownloads")
             .WithBrowser()
-            .InGuestMode()));
+            .InGuestMode(),
+        EventCase("tabindexSaveFileDialogDownloads")
+            .WithBrowser()
+            .InGuestMode()
+            .EnableMyFilesVolume()));
 
 WRAPPED_INSTANTIATE_TEST_CASE_P(
     FileDialog, /* file_dialog.js */
@@ -776,13 +800,17 @@ WRAPPED_INSTANTIATE_TEST_CASE_P(
 WRAPPED_INSTANTIATE_TEST_CASE_P(
     Metadata, /* metadata.js */
     FilesAppBrowserTest,
-    ::testing::Values(TestCase("metadataDownloads"),
-                      TestCase("metadataDrive").DisableDriveFs(),
-                      TestCase("metadataDrive").EnableDriveFs(),
-                      TestCase("metadataTeamDrives").DisableDriveFs(),
-                      TestCase("metadataTeamDrives").EnableDriveFs(),
-                      TestCase("metadataLargeDrive").DisableDriveFs(),
-                      TestCase("metadataLargeDrive").EnableDriveFs()));
+    ::testing::Values(
+        TestCase("metadataDownloads"),
+        TestCase("metadataDrive").DisableDriveFs(),
+        TestCase("metadataDrive").EnableDriveFs(),
+        TestCase("metadataDrive").EnableDriveFs().EnableMyFilesVolume(),
+        TestCase("metadataTeamDrives").DisableDriveFs(),
+        TestCase("metadataTeamDrives").EnableDriveFs(),
+        TestCase("metadataTeamDrives").EnableDriveFs().EnableMyFilesVolume(),
+        TestCase("metadataLargeDrive").DisableDriveFs(),
+        TestCase("metadataLargeDrive").EnableDriveFs(),
+        TestCase("metadataLargeDrive").EnableDriveFs().EnableMyFilesVolume()));
 
 // Structure to describe an account info.
 struct TestAccountInfo {

ui/file_manager/file_manager/common/js/BUILD.gn

@@ -180,9 +180,11 @@ js_library("util") {
 
 js_unittest("util_unittest") {
   deps = [
+    ":files_app_entry_types",
     ":mock_entry",
+    ":unittest_util",
     ":util",
-    "//ui/file_manager/base/js:test_error_reporting",
+    "//ui/file_manager/file_manager/background/js:mock_volume_manager",
   ]
 }
 

ui/file_manager/file_manager/common/js/files_app_entry_types.js

@@ -280,7 +280,12 @@ class EntryList {
     this.type_name = 'EntryList';
   }
 
-  get children() {
+  /**
+   * @return {!Array<!Entry|!FilesAppEntry>} List of entries that are shown as
+   *     children of this Volume in the UI, but are not actually entries of the
+   *     Volume.  E.g. 'Play files' is shown as a child of 'My files'.
+   */
+  getUIChildren() {
     return this.children_;
   }
 
@@ -360,8 +365,9 @@ class EntryList {
    * @return {number} index of entry on this EntryList or -1 if not found.
    */
   findIndexByVolumeInfo(volumeInfo) {
-    return this.children.findIndex(
-        childEntry => childEntry.volumeInfo === volumeInfo);
+    return this.children_.findIndex(childEntry => {
+      return /** @type {VolumeEntry} */ (childEntry).volumeInfo === volumeInfo;
+    });
   }
 
   /**
@@ -371,11 +377,12 @@ class EntryList {
    * @return {boolean} if entry was removed.
    */
   removeByVolumeType(volumeType) {
-    const childIndex = this.children.findIndex(
-        childEntry => childEntry.volumeInfo &&
-            childEntry.volumeInfo.volumeType === volumeType);
+    const childIndex = this.children_.findIndex(childEntry => {
+      const volumeInfo = /** @type {VolumeEntry} */ (childEntry).volumeInfo;
+      return volumeInfo && volumeInfo.volumeType === volumeType;
+    });
     if (childIndex !== -1) {
-      this.children.splice(childIndex, 1);
+      this.children_.splice(childIndex, 1);
       return true;
     }
     return false;
@@ -388,10 +395,10 @@ class EntryList {
    * @return {boolean} if entry was removed.
    */
   removeByRootType(rootType) {
-    const childIndex =
-        this.children.findIndex(childEntry => childEntry.rootType === rootType);
+    const childIndex = this.children_.findIndex(
+        childEntry => childEntry.rootType === rootType);
     if (childIndex !== -1) {
-      this.children.splice(childIndex, 1);
+      this.children_.splice(childIndex, 1);
       return true;
     }
     return false;

ui/file_manager/file_manager/common/js/files_app_entry_types_unittest.js

@@ -38,13 +38,13 @@ function testEntryList(testReportCallback) {
   assertEquals('my_files', entryList.rootType);
   assertFalse(entryList.isNativeType);
   assertEquals(null, entryList.getNativeEntry());
-  assertEquals(0, entryList.children.length);
+  assertEquals(0, entryList.getUIChildren().length);
   assertTrue(entryList.isDirectory);
   assertFalse(entryList.isFile);
 
   entryList.addEntry(
       new EntryList('Child Entry', VolumeManagerCommon.RootType.MY_FILES));
-  assertEquals(1, entryList.children.length);
+  assertEquals(1, entryList.getUIChildren().length);
 
   const reader = entryList.createReader();
   // How many times the reader callback |accumulateResults| has been called?
@@ -92,12 +92,12 @@ function testEntryListGetParent(testReportCallback) {
 function testEntryListAddEntry() {
   const entryList =
       new EntryList('My files', VolumeManagerCommon.RootType.MY_FILES);
-  assertEquals(0, entryList.children.length);
+  assertEquals(0, entryList.getUIChildren().length);
 
   const childEntry = fakeVolumeEntry(VolumeManagerCommon.VolumeType.DOWNLOADS);
   entryList.addEntry(childEntry);
-  assertEquals(1, entryList.children.length);
-  assertEquals(childEntry, entryList.children[0]);
+  assertEquals(1, entryList.getUIChildren().length);
+  assertEquals(childEntry, entryList.getUIChildren()[0]);
 }
 
 /**
@@ -130,7 +130,7 @@ function testEntryFindIndex() {
   // Test removeByVolumeType.
   assertTrue(
       entryList.removeByVolumeType(VolumeManagerCommon.VolumeType.CROSTINI));
-  assertEquals(1, entryList.children.length);
+  assertEquals(1, entryList.getUIChildren().length);
   // Now crostini volume doesn't exist anymore, so should return False.
   assertFalse(
       entryList.removeByVolumeType(VolumeManagerCommon.VolumeType.CROSTINI));
@@ -138,7 +138,7 @@ function testEntryFindIndex() {
   // Test removeByRootType.
   entryList.addEntry(fakeEntry);
   assertTrue(entryList.removeByRootType(VolumeManagerCommon.RootType.CROSTINI));
-  assertEquals(1, entryList.children.length);
+  assertEquals(1, entryList.getUIChildren().length);
 }
 
 /**
@@ -473,7 +473,7 @@ function testEntryListAddEntrySetsPrefix() {
       new EntryList('My files', VolumeManagerCommon.RootType.MY_FILES);
 
   entryList.addEntry(volumeEntry);
-  assertEquals(1, entryList.children.length);
+  assertEquals(1, entryList.getUIChildren().length);
   // entryList is parent of volumeEntry so it should be its prefix.
   assertEquals(entryList, volumeEntry.volumeInfo.prefixEntry);
 }

ui/file_manager/file_manager/common/js/util.js

@@ -947,19 +947,32 @@ util.isChildEntry = function(entry, directory) {
 util.isDescendantEntry = function(ancestorEntry, childEntry) {
   if (!ancestorEntry.isDirectory)
     return false;
-  if (ancestorEntry instanceof EntryList) {
-    const entryList = /** @type {EntryList} */ (ancestorEntry);
-    return entryList.children.some(ancestorChild => {
+
+  // For EntryList and VolumeEntry they can contain entries from different
+  // files systems, so we should check its getUIChildren.
+  const entryList = util.toEntryList(ancestorEntry);
+  if (entryList.getUIChildren) {
+    // VolumeEntry has to check to root entry descendant entry.
+    const nativeEntry = entryList.getNativeEntry();
+    if (nativeEntry &&
+        util.isSameFileSystem(nativeEntry.filesystem, childEntry.filesystem)) {
+      return util.isDescendantEntry(
+          /** @type {!DirectoryEntry} */ (nativeEntry), childEntry);
+    }
+
+    return entryList.getUIChildren().some(ancestorChild => {
       if (util.isSameEntry(ancestorChild, childEntry))
         return true;
 
       // root entry might not be resolved yet.
-      const volumeEntry = ancestorChild.getNativeEntry();
+      const volumeEntry =
+          /** @type {DirectoryEntry} */ (ancestorChild.getNativeEntry());
       return volumeEntry &&
           (util.isSameEntry(volumeEntry, childEntry) ||
            util.isDescendantEntry(volumeEntry, childEntry));
     });
   }
+
   if (!util.isSameFileSystem(ancestorEntry.filesystem, childEntry.filesystem))
     return false;
   if (util.isSameEntry(ancestorEntry, childEntry))
@@ -1501,6 +1514,16 @@ util.toFilesAppEntry = function(entry) {
   return /** @type {FilesAppEntry} */ (entry);
 };
 
+/**
+ * Casts an Entry to a EntryList, to access a FilesAppEntry-specific
+ * property without Closure compiler complaining.
+ * @param {Entry|FilesAppEntry} entry
+ * @return {EntryList}
+ */
+util.toEntryList = function(entry) {
+  return /** @type {EntryList} */ (entry);
+};
+
 /**
  * Returns true if entry is FileSystemEntry or FileSystemDirectoryEntry, it
  * returns false if it's FakeEntry or any one of the FilesAppEntry types.

ui/file_manager/file_manager/common/js/util_unittest.js

@@ -5,6 +5,13 @@
 let fileSystem;
 
 function setUp() {
+  // Override VolumeInfo.prototype.resolveDisplayRoot to be sync.
+  VolumeInfoImpl.prototype.resolveDisplayRoot = function(successCallback) {
+    this.displayRoot_ = this.fileSystem_.root;
+    successCallback(this.displayRoot_);
+    return Promise.resolve(this.displayRoot_);
+  };
+
   fileSystem = new MockFileSystem('fake-volume');
   const filenames = [
     '/file_a.txt',
@@ -16,6 +23,10 @@ function setUp() {
     '/dir_a/dir_b/dir_c/file_g.txt',
   ];
   fileSystem.populate(filenames);
+
+  window.loadTimeData = {
+    getString: id => id,
+  };
 }
 
 function testReadEntriesRecursively(callback) {
@@ -65,3 +76,53 @@ function testReadEntriesRecursivelyLevel1(callback) {
       },
       () => {}, () => false, 1 /* opt_maxDepth */);
 }
+
+
+function testIsDescendantEntry() {
+  const root = fileSystem.root;
+  const folder = fileSystem.entries['/dir_a'];
+  const subFolder = fileSystem.entries['/dir_a/dir_b'];
+  const file = fileSystem.entries['/file_a.txt'];
+  const deepFile = fileSystem.entries['/dir_a/dir_b/dir_c/file_g.txt'];
+
+  const fakeEntry =
+      new FakeEntry('fake-entry-label', VolumeManagerCommon.RootType.CROSTINI);
+
+  const entryList =
+      new EntryList('entry-list-label', VolumeManagerCommon.RootType.MY_FILES);
+  entryList.addEntry(fakeEntry);
+
+  const volumeManager = new MockVolumeManager();
+  // Index 1 is Downloads.
+  assertEquals(
+      VolumeManagerCommon.VolumeType.DOWNLOADS,
+      volumeManager.volumeInfoList.item(1).volumeType);
+  const downloadsVolumeInfo = volumeManager.volumeInfoList.item(1);
+  const mockFs = /** @type {MockFileSystem} */ (downloadsVolumeInfo.fileSystem);
+  mockFs.populate(['/folder1/']);
+  const folder1 = downloadsVolumeInfo.fileSystem.entries['/folder1'];
+
+  const volumeEntry = new VolumeEntry(downloadsVolumeInfo);
+  volumeEntry.addEntry(fakeEntry);
+
+  // No descendants.
+  assertFalse(util.isDescendantEntry(file, file));
+  assertFalse(util.isDescendantEntry(root, root));
+  assertFalse(util.isDescendantEntry(deepFile, root));
+  assertFalse(util.isDescendantEntry(subFolder, root));
+  assertFalse(util.isDescendantEntry(fakeEntry, root));
+  assertFalse(util.isDescendantEntry(root, fakeEntry));
+  assertFalse(util.isDescendantEntry(fakeEntry, entryList));
+  assertFalse(util.isDescendantEntry(fakeEntry, volumeEntry));
+  assertFalse(util.isDescendantEntry(folder1, volumeEntry));
+
+  assertTrue(util.isDescendantEntry(root, file));
+  assertTrue(util.isDescendantEntry(root, subFolder));
+  assertTrue(util.isDescendantEntry(root, deepFile));
+  assertTrue(util.isDescendantEntry(root, folder));
+  assertTrue(util.isDescendantEntry(folder, subFolder));
+  assertTrue(util.isDescendantEntry(folder, deepFile));
+  assertTrue(util.isDescendantEntry(entryList, fakeEntry));
+  assertTrue(util.isDescendantEntry(volumeEntry, fakeEntry));
+  assertTrue(util.isDescendantEntry(volumeEntry, folder1));
+}

ui/file_manager/file_manager/foreground/js/file_manager.js

@@ -1418,7 +1418,7 @@ FileManager.prototype = /** @struct */ {
 
     // If there is no target select MyFiles by default.
     queue.run((callback) => {
-      if (!nextCurrentDirEntry)
+      if (!nextCurrentDirEntry && this.directoryTree.dataModel.myFilesModel_)
         nextCurrentDirEntry = this.directoryTree.dataModel.myFilesModel_.entry;
 
       callback();

ui/file_manager/file_manager/foreground/js/navigation_list_model.js

@@ -518,6 +518,15 @@ NavigationListModel.prototype.orderAndNestItems_ = function() {
             str('MY_FILES_ROOT_LABEL'), NavigationModelItemType.ENTRY_LIST,
             myFilesEntry);
         this.myFilesModel_ = myFilesModel;
+      } else {
+        // When MyFilesVolume isn't available we create a empty EntryList to be
+        // MyFiles to be able to display Linux or Play volumes. However we don't
+        // save it back to this.MyFilesModel_ so it's always re-created.
+        myFilesEntry = new EntryList(
+            str('MY_FILES_ROOT_LABEL'), VolumeManagerCommon.RootType.MY_FILES);
+        myFilesModel = new NavigationModelFakeItem(
+            myFilesEntry.label, NavigationModelItemType.ENTRY_LIST,
+            myFilesEntry);
       }
     } else {
       // Here is the initial version for MyFiles, which is only an entry in JS

ui/file_manager/file_manager/foreground/js/navigation_list_model_unittest.js

@@ -27,7 +27,7 @@ loadTimeData.data = {
 
 function setUp() {
   new MockCommandLinePrivate();
-  // Override VolumeInfo.prototype.resolveDisplayRoot.
+  // Override VolumeInfo.prototype.resolveDisplayRoot to be sync.
   VolumeInfoImpl.prototype.resolveDisplayRoot = function(successCallback) {
     this.displayRoot_ = this.fileSystem_.root;
     successCallback(this.displayRoot_);
@@ -62,9 +62,9 @@ function testModel() {
 
   // Downloads and Crostini are displayed within My files.
   const myFilesEntry = model.item(2).entry;
-  assertEquals(2, myFilesEntry.children.length);
-  assertEquals('Downloads', myFilesEntry.children[0].name);
-  assertEquals('linux-files-label', myFilesEntry.children[1].name);
+  assertEquals(2, myFilesEntry.getUIChildren().length);
+  assertEquals('Downloads', myFilesEntry.getUIChildren()[0].name);
+  assertEquals('linux-files-label', myFilesEntry.getUIChildren()[1].name);
 }
 
 function testNoRecentOrLinuxFiles() {

ui/file_manager/file_manager/foreground/js/ui/directory_tree.js

@@ -730,6 +730,10 @@ function EntryListItem(rootType, modelItem, tree) {
   item.dirEntry_ = modelItem.entry;
   item.parentTree_ = tree;
 
+  if (window.IN_TEST && item.entry && item.entry.volumeInfo) {
+    item.setAttribute(
+        'volume-type-for-testing', item.entry.volumeInfo.volumeType);
+  }
   const icon = queryRequiredElement('.icon', item);
   icon.classList.add('item-icon');
   icon.setAttribute('root-type-icon', rootType);

ui/file_manager/integration_tests/file_manager/file_display.js

@@ -498,7 +498,7 @@ testcase.fileDisplayWithoutVolumesThenMountDownloads = function() {
     },
     // Downloads should appear in My files in the directory tree.
     function() {
-      remoteCall.waitForElement(appId, '[volume-type-for-testing="downloads"]')
+      remoteCall.waitForElement(appId, '[volume-type-icon="downloads"]')
           .then(this.next);
     },
     function() {