Navigate to the next file after deletion in chrome://media-app.

This patch
* Mutates `currentFiles` upon deletion and tries to open the next file.
* allows empty fileLists in `advance()` and `sendFilestoGuest()`
* Adds a test for the scenario of deleting files in a directory
of images.
* reworks some of DeleteOriginalIPC to make the test more
robust as before it was passing due to FakeFileSystemHandle always having
the same default file name. Explicitly adding a filename makes the test
more obvious when debugging.
* Also swap DeleteOriginalIPC assert calls around to have the expected
value on the LHS as we use the `assertEquals` from
go/crcs/chrome/test/data/webui/test_api.js

Bug: 1077732, b/156042029
Change-Id: I1606809db9ccae388182a086aa9045280624c5dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2206195
Commit-Queue: David Lei <dlei@google.com>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770928}

chromeos/components/media_app_ui/resources/js/launch.js

@@ -84,6 +84,15 @@ guestMessagePipe.registerHandler(Message.DELETE_FILE, async (message) => {
   const currentFilename = (await file.handle.getFile()).name;
 
   await directory.removeEntry(currentFilename);
+
+  // Remove the file that was deleted.
+  currentFiles.splice(entryIndex, 1);
+
+  // Attempts to load the file to the right which is at now at
+  // `currentFiles[entryIndex]`, where `entryIndex` was previously the index of
+  // the deleted file.
+  await advance(0);
+
   return {deleteResult: DeleteResult.SUCCESS};
 });
 
@@ -237,8 +246,13 @@ async function sendFilesToGuest() {
   if (currentlyWritableFile) {
     currentlyWritableFile.token = -1;
   }
-  currentlyWritableFile = currentFiles[entryIndex];
-  currentlyWritableFile.token = ++fileToken;
+  if (currentFiles.length) {
+    currentlyWritableFile = currentFiles[entryIndex];
+    currentlyWritableFile.token = ++fileToken;
+  } else {
+    currentlyWritableFile = null;
+  }
+
   return sendSnapshotToGuest([...currentFiles]);  // Shallow copy.
 }
 
@@ -443,12 +457,13 @@ async function launchWithDirectory(directory, handle) {
  * @param {number} direction How far to advance (e.g. +/-1).
  */
 async function advance(direction) {
-  if (!currentFiles.length || entryIndex < 0) {
-    return;
-  }
-  entryIndex = (entryIndex + direction) % currentFiles.length;
-  if (entryIndex < 0) {
-    entryIndex += currentFiles.length;
+  if (currentFiles.length) {
+    entryIndex = (entryIndex + direction) % currentFiles.length;
+    if (entryIndex < 0) {
+      entryIndex += currentFiles.length;
+    }
+  } else {
+    entryIndex = -1;
   }
 
   await sendFilesToGuest();

chromeos/components/media_app_ui/resources/mock/js/app_main.js

@@ -57,12 +57,19 @@ class BacklightApp extends HTMLElement {
 
   /** @override  */
   async loadFiles(files) {
+    let child;
     const file = files.item(0);
-    const isVideo = file.mimeType.match('^video/');
-    const factory = isVideo ? createVideoChild : createImgChild;
-    // Note the mock app will just leak this Blob URL.
-    const child = await factory(URL.createObjectURL(file.blob), file.name);
-    // Simulate an app that shows one image at a time.
+    if (file) {
+      const isVideo = file.mimeType.match('^video/');
+      const factory = isVideo ? createVideoChild : createImgChild;
+      // Note the mock app will just leak this Blob URL.
+      child = await factory(URL.createObjectURL(file.blob), file.name);
+    } else {
+      // Emulate zero state.
+      child = document.createElement('img');
+    }
+    // Simulate an app that shows one image (either the loaded image or zero
+    // state) at a time.
     this.replaceChild(child, this.currentMedia);
     this.currentMedia = child;
   }

chromeos/components/media_app_ui/test/driver.js

@@ -340,3 +340,16 @@ function assertMatchErrorStack(stackTrace, regexLines, opt_message) {
   const regex = `(.|\\n)*${regexLines.join('(.|\\n)*')}(.|\\n)*`;
   assertMatch(stackTrace, regex, opt_message);
 }
+
+/**
+ * Returns the files loaded in the most recent call to `loadFiles()`.
+ * @return {Promise<?ReceivedFileList>}
+ */
+async function getLoadedFiles() {
+  const response = /** @type {LastLoadedFilesResponse} */ (
+      await guestMessagePipe.sendMessage('get-last-loaded-files'));
+  if (response.fileList) {
+    return response.fileList.files;
+  }
+  return null;
+}

chromeos/components/media_app_ui/test/driver_api.js

@@ -24,3 +24,10 @@ let TestMessageQueryData;
 
 /** @typedef {{testCase: string}} */
 let TestMessageRunTestCase;
+
+/**
+ * Return type of `get-last-loaded-files` used to spy on the files sent to the
+ * guest app using `loadFiles()`.
+ * @typedef {{fileList: ?{files: !ReceivedFileList}}}
+ */
+let LastLoadedFilesResponse;

chromeos/components/media_app_ui/test/guest_query_receiver.js

@@ -38,10 +38,10 @@ async function runTestQuery(data) {
     }
   } else if (data.navigate !== undefined) {
     if (data.navigate === 'next') {
-      lastReceivedFileList.loadNext();
+      await lastReceivedFileList.loadNext();
       result = 'loadNext called';
     } else if (data.navigate === 'prev') {
-      lastReceivedFileList.loadPrev();
+      await lastReceivedFileList.loadPrev();
       result = 'loadPrev called';
     } else {
       result = 'nothing called';
@@ -157,6 +157,11 @@ function installTestHandlers() {
     return runTestCase(/** @type{TestMessageRunTestCase} */ (data));
   });
 
+  parentMessagePipe.registerHandler('get-last-loaded-files', () => {
+    return /** @type {LastLoadedFilesResponse} */ (
+        {fileList: lastReceivedFileList});
+  });
+
   // Log errors, rather than send them to console.error. This allows the error
   // handling tests to work correctly, and is also required for
   // signalTestHandlersReady() to operate without failing tests.

chromeos/components/media_app_ui/test/media_app_ui_browsertest.js

@@ -431,12 +431,9 @@ TEST_F('MediaAppUIBrowserTest', 'CrossContextErrors', async () => {
 // Tests the IPC behind the implementation of ReceivedFile.deleteOriginalFile()
 // in the untrusted context.
 TEST_F('MediaAppUIBrowserTest', 'DeleteOriginalIPC', async () => {
-  const directory = await createMockTestDirectory();
-  // Simulate steps taken to load a file via a launch event.
-  const firstFile = directory.files[0];
-  await loadFile(await createTestImageFile(), firstFile);
-  // Set `currentDirectoryHandle` in launch.js.
-  currentDirectoryHandle = directory;
+  let directory = await launchWithFiles(
+      [await createTestImageFile(1, 1, 'first_file_name.png')]);
+  const testHandle = directory.files[0];
   let testResponse;
 
   // Nothing should be deleted initially.
@@ -448,23 +445,28 @@ TEST_F('MediaAppUIBrowserTest', 'DeleteOriginalIPC', async () => {
   // Assertion will fail if exceptions from launch.js are thrown, no exceptions
   // indicates the file was successfully deleted.
   assertEquals(
-      testResponse.testQueryResult, 'deleteOriginalFile resolved success');
-  assertEquals(firstFile, directory.lastDeleted);
+      'deleteOriginalFile resolved success', testResponse.testQueryResult);
+  assertEquals(testHandle, directory.lastDeleted);
   // File removed from `DirectoryHandle` internal state.
-  assertEquals(directory.files.length, 0);
+  assertEquals(0, directory.files.length);
 
-  // Even though the name is the same, the new file shouldn't
-  // be deleted as it has a different `FileHandle` reference.
-  directory.addFileHandleForTest(new FakeFileSystemFileHandle());
+  // Load another file and replace its handle in the underlying
+  // `FileSystemDirectoryHandle`. This gets us into a state where the file on
+  // disk has been deleted and a new file with the same name replaces it without
+  // updating the `FakeSystemDirectoryHandle`. The new file shouldn't be deleted
+  // as it has a different `FileHandle` reference.
+  directory = await launchWithFiles(
+      [await createTestImageFile(1, 1, 'first_file_name.png')]);
+  directory.files[0] = new FakeFileSystemFileHandle('first_file_name.png');
 
   // Try delete the first file again, should result in file moved.
   const messageDeleteMoved = {deleteLastFile: true};
   testResponse = await guestMessagePipe.sendMessage('test', messageDeleteMoved);
 
   assertEquals(
-      testResponse.testQueryResult, 'deleteOriginalFile resolved file moved');
+      'deleteOriginalFile resolved file moved', testResponse.testQueryResult);
   // New file not removed from `DirectoryHandle` internal state.
-  assertEquals(directory.files.length, 1);
+  assertEquals(1, directory.files.length);
 
   // Prevent the trusted context throwing errors resulting JS errors.
   guestMessagePipe.logClientError = error => console.log(JSON.stringify(error));
@@ -476,8 +478,71 @@ TEST_F('MediaAppUIBrowserTest', 'DeleteOriginalIPC', async () => {
   testResponse = await guestMessagePipe.sendMessage('test', messageDeleteNoOp);
 
   assertEquals(
-      testResponse.testQueryResult,
-      'deleteOriginalFile resolved UNKNOWN_ERROR');
+      'deleteOriginalFile resolved UNKNOWN_ERROR',
+      testResponse.testQueryResult);
+  testDone();
+});
+
+// Tests when a file is deleted, the app tries to open the next available file
+// and reloads with those files.
+TEST_F('MediaAppUIBrowserTest', 'DeletionOpensNextFile', async () => {
+  const testFiles = [
+    await createTestImageFile(1, 1, 'test_file_1.png'),
+    await createTestImageFile(1, 1, 'test_file_2.png'),
+    await createTestImageFile(1, 1, 'test_file_3.png'),
+  ];
+  const directory = await launchWithFiles(testFiles);
+  let testResponse;
+  // Shallow copy so mutations to `directory.files` don't effect
+  // `testHandles`.
+  const testHandles = [...directory.files];
+
+  // Check the app loads all 3 files.
+  let lastLoadedFiles = await getLoadedFiles();
+  assertEquals(3, lastLoadedFiles.length);
+  assertEquals('test_file_1.png', lastLoadedFiles[0].name);
+  assertEquals('test_file_2.png', lastLoadedFiles[1].name);
+  assertEquals('test_file_3.png', lastLoadedFiles[2].name);
+
+  // Delete the first file.
+  const messageDelete = {deleteLastFile: true};
+  testResponse = await guestMessagePipe.sendMessage('test', messageDelete);
+
+  assertEquals(
+      'deleteOriginalFile resolved success', testResponse.testQueryResult);
+  assertEquals(testHandles[0], directory.lastDeleted);
+  assertEquals(directory.files.length, 2);
+
+  // Check the app reloads the file list with the remaining two files.
+  lastLoadedFiles = await getLoadedFiles();
+  assertEquals(2, lastLoadedFiles.length);
+  assertEquals('test_file_2.png', lastLoadedFiles[0].name);
+  assertEquals('test_file_3.png', lastLoadedFiles[1].name);
+
+  // Navigate to the last file (originally the third file) and delete it
+  await guestMessagePipe.sendMessage('test', {navigate: 'next'});
+  testResponse = await guestMessagePipe.sendMessage('test', messageDelete);
+
+  assertEquals(
+      'deleteOriginalFile resolved success', testResponse.testQueryResult);
+  assertEquals(testHandles[2], directory.lastDeleted);
+  assertEquals(directory.files.length, 1);
+
+  // Check the app reloads the file list with the last remaining file
+  // (originally the second file).
+  lastLoadedFiles = await getLoadedFiles();
+  assertEquals(1, lastLoadedFiles.length);
+  assertEquals(testFiles[1].name, lastLoadedFiles[0].name);
+
+  // Delete the last file, should lead to zero state.
+  testResponse = await guestMessagePipe.sendMessage('test', messageDelete);
+  assertEquals(
+      'deleteOriginalFile resolved success', testResponse.testQueryResult);
+
+  // The app should be in zero state with no media loaded.
+  lastLoadedFiles = await getLoadedFiles();
+  assertEquals(0, lastLoadedFiles.length);
+
   testDone();
 });