Make renaming a file not reload the file in chrome://media-app.

Previously we called `launchWithDirectory()` to load in the renamed
file. That blocked the renaming IPC finishing on the entire directory
loading. This change takes a similar approach to SAVE_AS updating
the file in `tokenMap` & `currentFiles` relying on the client app
to reload itself (done in `handleChromiumIPC`) so we don't need to
load the file again!

This change unblocks b/163002455, b/163422302 & b/163432652 as well
as adds a return value for invalid tokens.

It also prevents losing our edit stack when we rename a file!

Bug: b/163422302, b/162281131, b/162281681, 996088
Change-Id: I690963870d1346d31cb77a0c44aeb95802216ef5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2352586
Commit-Queue: David Lei <dlei@google.com>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799001}

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

@@ -154,6 +154,13 @@ guestMessagePipe.registerHandler(Message.RENAME_FILE, async (message) => {
   }
 
   const originalFile = await handle.getFile();
+  let originalFileIndex =
+      currentFiles.findIndex(fd => fd.token === renameMsg.token);
+
+  if (originalFileIndex < 0) {
+    return {renameResult: RenameResult.FILE_NO_LONGER_IN_LAST_OPENED_DIRECTORY};
+  }
+
   const renamedFileHandle =
       await directory.getFileHandle(renameMsg.newFilename, {create: true});
   // Copy file data over to the new file.
@@ -171,9 +178,30 @@ guestMessagePipe.registerHandler(Message.RENAME_FILE, async (message) => {
     await directory.removeEntry(originalFile.name);
   }
 
-  // Reload current file so it is in an editable state, this is done before
-  // removing the old file so the relaunch starts sooner.
-  await launchWithDirectory(directory, renamedFileHandle);
+  // Replace the old file in our internal representation. There is no harm using
+  // the old file's token since the old file is removed.
+  tokenMap.set(renameMsg.token, renamedFileHandle);
+  // Remove the entry for `originalFile` in current files, replace it with a
+  // FileDescriptor for the renamed file.
+
+  const renamedFile = await renamedFileHandle.getFile();
+  // Ensure the file is still in `currentFiles` after all the above `awaits`. If
+  // missing it means either new files have loaded (or tried to), see
+  // b/164985809.
+  originalFileIndex =
+      currentFiles.findIndex(fd => fd.token === renameMsg.token);
+
+  if (originalFileIndex < 0) {
+    // Can't navigate to the renamed file so don't add it to `currentFiles`.
+    return {renameResult: RenameResult.SUCCESS};
+  }
+
+  currentFiles.splice(originalFileIndex, 1, {
+    token: renameMsg.token,
+    file: renamedFile,
+    handle: renamedFileHandle,
+    inCurrentDirectory: true
+  });
 
   return {renameResult: RenameResult.SUCCESS};
 });
@@ -614,6 +642,8 @@ async function processOtherFilesInDirectory(
 
     // Only allow traversal of related file types.
     if (entry && isFileRelated(focusFile, entry.file)) {
+      // Note: The focus file will be processed here again but will be skipped
+      // over when added to `currentFiles`.
       relatedFiles.push({
         token: generateToken(entry.handle),
         file: entry.file,
@@ -730,6 +760,9 @@ async function launchWithDirectory(directory, handle) {
   // The app is operable with the first file now.
 
   // Process other files in directory.
+  // TODO(https://github.com/WICG/native-file-system/issues/215): Don't process
+  // other files if there is only 1 file which is already loaded by
+  // `sendSnapshotToGuest()` above.
   await loadOtherRelatedFiles(
       directory, asFile.file, asFile.handle, localLaunchNumber);
 }

chromeos/components/media_app_ui/resources/js/message_types.js

@@ -103,6 +103,7 @@ let NavigateMessage;
  * @enum {number}
  */
 const RenameResult = {
+  FILE_NO_LONGER_IN_LAST_OPENED_DIRECTORY: -1,
   SUCCESS: 0,
   FILE_EXISTS: 1,
 };

chromeos/components/media_app_ui/test/guest_query_receiver.js

@@ -82,6 +82,11 @@ async function runTestQuery(data) {
               .call(firstReceivedItem(), data.renameLastFile);
       if (renameResult === RenameResult.FILE_EXISTS) {
         result = 'renameOriginalFile resolved file exists';
+      } else if (
+          renameResult ===
+          RenameResult.FILE_NO_LONGER_IN_LAST_OPENED_DIRECTORY) {
+        result = 'renameOriginalFile resolved ' +
+            'FILE_NO_LONGER_IN_LAST_OPENED_DIRECTORY';
       } else {
         result = 'renameOriginalFile resolved success';
       }

chromeos/components/media_app_ui/test/media_app_ui_browsertest.js

@@ -284,12 +284,9 @@ TEST_F('MediaAppUIBrowserTest', 'NonLaunchableIpcAfterFastLoad', async () => {
   testDone();
 });
 
-
 // Tests that we can launch the MediaApp with the selected (first) file,
-// interact with it by invoking IPC (rename) that re-launches the
-// MediaApp (calls `launchWithDirectory`), then the rest of the files
-// in the current directory are loaded in.
-TEST_F('MediaAppUIBrowserTest', 'ReLaunchableIpcAfterFastLoad', async () => {
+// and re-launch it before all files from the first launch are loaded in.
+TEST_F('MediaAppUIBrowserTest', 'ReLaunchableAfterFastLoad', async () => {
   const files =
       await createMultipleImageFiles(['file1', 'file2', 'file3', 'file4']);
   const directory = await createMockTestDirectory(files);
@@ -303,48 +300,42 @@ TEST_F('MediaAppUIBrowserTest', 'ReLaunchableIpcAfterFastLoad', async () => {
 
   await assertSingleFileLaunch(directory, files.length);
 
-  // Invoke Rename IPC that relaunches the app, this calls
-  // `launchWithDirectory()` which increments globalLaunchNumber.
-  const messageRename = {renameLastFile: 'new_file_name.png'};
-  const testResponse = await sendTestMessage(messageRename);
-  assertEquals(
-      testResponse.testQueryResult, 'renameOriginalFile resolved success');
+  // Mutate the second file.
+  directory.files[1].name = 'changed.png';
+  // Relaunch the app with the second file.
+  await launchWithDirectory(directory, directory.files[1]);
 
-  // Ensure rename relaunches the app incremented the `globalLaunchNumber`.
+  // Ensure second launch incremented the `globalLaunchNumber`.
   assertEquals(1, globalLaunchNumber);
-  // The renamed file (also the focused file) is at the end of the
-  // `FileSystemDirectoryHandle.files` since it was deleted and a new file
-  // with the same contents and a different name was created.
-  assertEquals(directory.files[3].name, 'new_file_name.png');
 
-  // The call to `launchWithDirectory()` from rename relaunching the app loads
-  // other files into `currentFiles`.
+  // Second launch loads other files into `currentFiles`.
   await assertFilesLoaded(
-      directory, ['new_file_name.png', 'file2.png', 'file3.png', 'file4.png'],
-      'fast files: check files after renaming');
-  const currentFilesAfterRenameLaunch = [...currentFiles];
-  const loadedFilesAfterRename = await getLoadedFiles();
-
-  // Try to load with previous launch number, has no effect as it is aborted
-  // early due to different launch numbers.
+      directory, ['changed.png', 'file3.png', 'file4.png', 'file1.png'],
+      'fast files: check files after relaunching');
+  const currentFilesAfterSecondLaunch = [...currentFiles];
+  const loadedFilesSecondLaunch = await getLoadedFiles();
+
+  // Try to load with previous launch number simulating the first launch
+  // completing after the second launch. Has no effect as it is aborted early
+  // due to different launch numbers.
   const previousLaunchNumber = 0;
   await loadOtherRelatedFiles(
       directory, focusFile.file, focusFile.handle, previousLaunchNumber);
 
-  // Ensure same files as before the call to `loadOtherRelatedFiles()` by for
-  // equality equality.
-  currentFilesAfterRenameLaunch.map(
+  // Ensure `currentFiles is the same as the file state at the end of the second
+  // launch before the call to `loadOtherRelatedFiles()`.
+  currentFilesAfterSecondLaunch.map(
       (fd, index) => assertEquals(
           fd, currentFiles[index],
           `Equality check for file ${
               JSON.stringify(fd)} in currentFiles filed`));
 
-  // Focus file stays index 0.
+  // Focus file (file that the directory was launched with) stays index 0.
   const lastLoadedFiles = await getLoadedFiles();
-  assertEquals('new_file_name.png', lastLoadedFiles[0].name);
-  assertEquals(loadedFilesAfterRename[0].name, lastLoadedFiles[0].name);
-  // Focus file in the `FileSystemDirectoryHandle` is at index 3.
-  assertEquals(directory.files[3].name, lastLoadedFiles[0].name);
+  assertEquals('changed.png', lastLoadedFiles[0].name);
+  assertEquals(loadedFilesSecondLaunch[0].name, lastLoadedFiles[0].name);
+  // Focus file in the `FileSystemDirectoryHandle` is at index 1.
+  assertEquals(directory.files[1].name, lastLoadedFiles[0].name);
 
   testDone();
 });
@@ -764,49 +755,94 @@ TEST_F('MediaAppUIBrowserTest', 'NavigateIPC', async () => {
 });
 
 // Tests the IPC behind the implementation of ReceivedFile.renameOriginalFile()
-// in the untrusted context.
+// in the untrusted context. This test is integration-y making sure we rename
+// the focus file and that gets inserted in the right place in `currentFiles`
+// preserving navigation order.
 TEST_F('MediaAppUIBrowserTest', 'RenameOriginalIPC', async () => {
-  const directory = await launchWithFiles([await createTestImageFile()]);
-  const firstFileHandle = directory.files[0];
-  const firstFile = firstFileHandle.getFileSync();
-
-  let testResponse;
+  const directory = await launchWithFiles([
+    await createTestImageFile(1, 1, 'file1.png'),
+    await createTestImageFile(1, 1, 'file2.png')
+  ]);
 
   // Nothing should be deleted initially.
   assertEquals(null, directory.lastDeleted);
 
+  // Navigate to second file "file2.png".
+  await advance(1);
+
   // Test normal rename flow.
-  const messageRename = {renameLastFile: 'new_file_name.png'};
+  const file2Handle = directory.files[entryIndex];
+  const file2File = file2Handle.getFileSync();
+  const file2Token = currentFiles[entryIndex].token;
+  let messageRename = {renameLastFile: 'new_file_name.png'};
+  let testResponse;
+
   testResponse = await sendTestMessage(messageRename);
 
   assertEquals(
       testResponse.testQueryResult, 'renameOriginalFile resolved success');
   // The original file that was renamed got deleted.
-  assertEquals(firstFileHandle, directory.lastDeleted);
-  // There is still one file which is the renamed version of the original file.
-  assertEquals(directory.files.length, 1);
-  assertEquals(directory.files[0].name, 'new_file_name.png');
+  assertEquals(file2Handle, directory.lastDeleted);
+  assertEquals(directory.files.length, 2);
+  assertEquals(directory.files[entryIndex].name, 'new_file_name.png');
+  // The new file uses the same token as the old file.
+  assertEquals(currentFiles[entryIndex].token, file2Token);
   // Check the new file written has the correct data.
-  const newHandle = directory.files[0];
-  const newFile = await newHandle.getFile();
-  assertEquals(newFile.size, firstFile.size);
-  assertEquals(await newFile.text(), await firstFile.text());
+  const renamedHandle = directory.files[entryIndex];
+  const renamedFile = await renamedHandle.getFile();
+  assertEquals(renamedFile.size, file2File.size);
+  assertEquals(await renamedFile.text(), await file2File.text());
+  // Check the internal representation (token map & currentFiles) is updated.
+  assertEquals(tokenMap.get(file2Token), renamedHandle);
+  assertEquals(currentFiles[entryIndex].handle, renamedHandle);
+
+  // Check navigation order is preserved.
+  assertEquals(entryIndex, 1);
+  assertEquals(currentFiles[entryIndex].handle.name, 'new_file_name.png');
+  assertEquals(currentFiles[0].handle.name, 'file1.png');
 
-  // Test renaming when a file with the new name already exists.
+  // Advancing wraps around back to the first file.
+  await advance(1);
+
+  assertEquals(entryIndex, 0);
+  assertEquals(currentFiles[entryIndex].handle.name, 'file1.png');
+  assertEquals(currentFiles[1].handle.name, 'new_file_name.png');
+
+  // Test renaming when a file with the new name already exists, tries to rename
+  // `file1.png` to `new_file_name.png` which already exists.
   const messageRenameExists = {renameLastFile: 'new_file_name.png'};
   testResponse = await sendTestMessage(messageRenameExists);
 
   assertEquals(
       testResponse.testQueryResult, 'renameOriginalFile resolved file exists');
   // No change to the existing file.
-  assertEquals(directory.files.length, 1);
-  assertEquals(directory.files[0].name, 'new_file_name.png');
+  assertEquals(directory.files.length, 2);
+  assertEquals(directory.files[entryIndex].name, 'file1.png');
+  assertEquals(directory.files[1].name, 'new_file_name.png');
+
+  // Test renaming when something is out of sync with `currentFiles` and has an
+  // expired token.
+  const expiredToken = tokenGenerator.next().value;
+  currentFiles[entryIndex].token = expiredToken;
+
+  messageRename = {renameLastFile: 'another_name.png'};
+
+  testResponse = await sendTestMessage(messageRename);
+
+  // Fails silently, nothing changes.
+  assertEquals(
+      testResponse.testQueryResult,
+      'renameOriginalFile resolved FILE_NO_LONGER_IN_LAST_OPENED_DIRECTORY');
+  assertEquals(currentFiles[entryIndex].handle.name, 'file1.png');
+  assertEquals(currentFiles.length, 2);
+  assertEquals(directory.files.length, 2);
+
+  // Test it throws an error by simulating a failed directory change.
+  simulateLosingAccessToDirectory();
 
   // Prevent the trusted context throwing errors resulting JS errors.
   guestMessagePipe.logClientError = error => console.log(JSON.stringify(error));
   guestMessagePipe.rethrowErrors = false;
-  // Test it throws an error by simulating a failed directory change.
-  simulateLosingAccessToDirectory();
 
   const messageRenameNoOp = {renameLastFile: 'new_file_name_2.png'};
   testResponse = await sendTestMessage(messageRenameNoOp);
@@ -815,6 +851,7 @@ TEST_F('MediaAppUIBrowserTest', 'RenameOriginalIPC', async () => {
       testResponse.testQueryResult,
       'renameOriginalFile failed Error: Error: rename-file: Rename failed. ' +
           'File without launch directory.');
+
   testDone();
 });