Removed concept of a singular writable file.

As part of the go/bl-file-navigation-order effort, this CL dose 2 major
things:
    * Removes openFile from the delegate in favour of existing on the
      ReceivedFileList.
    * Removes the concept of the media app only having 1 writable file.

The idea that the media app only has 1 writable file was from a older
security model which has since been abandoned. As such this CL updates
the language used and variable names to better reflect how the media app
actually works. Any file can be writable but there is only ever 1
"current" file which is what the app is displaying at any given moment.

Part of this change also removes some legacy code which ensured that
the "currentlyWritableFile" was always the first file in the list which
is not required as the google3 side no longer expects this. Some
test code relied on this assumption however and was updated accordingly
as were some functions such as "firstReceivedItem" which were always
expecting the first file in the file list to be the "current" file.

Completely unrelated to this change is the addition of the
"suppressCrashReports" codepath, which was added due to the fact that
the "FileThatBecomesDirectory" test was triggering a invalid crash
report causing the test to fail. This cl seemed to adjust the test
timing such that this crash report was getting triggered before the
test ended whereas it was ignored previously.

BUG: b/151880563, b/165720635
Change-Id: I89c8ebfecab73dab4ffae0a77ceb434489a1b85b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2524625
Commit-Queue: Zain Afzal <zafzal@google.com>
Reviewed-by: Bugs Nash <bugsnash@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828616}

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

@@ -283,8 +283,7 @@ guestMessagePipe.registerHandler(Message.SAVE_AS, async (message) => {
     // Note `pickedFileHandle` could be the same as a `FileSystemFileHandle`
     // that exists in `tokenMap`. Possibly even the `File` currently open. But
     // that's OK. E.g. the next overwrite-file request will just invoke
-    // `saveBlobToFile` in the same way. Note there may be no currently writable
-    // file (e.g. save from clipboard).
+    // `saveBlobToFile` in the same way.
     await saveBlobToFile(pickedFileDescriptor.handle, blob);
   } catch (/** @type {!DOMException} */ e) {
     // If something went wrong revert the token back to its original
@@ -415,7 +414,9 @@ async function saveBlobToFile(handle, data) {
  * @param {string} fileName
  */
 function warnIfUncommon(e, fileName) {
-  if (e.name === 'NotFoundError' || e.name === 'NotAllowedError') {
+  // Errors we expect to be thrown in normal operation.
+  const commonErrors = ['NotFoundError', 'NotAllowedError', 'NotAFile'];
+  if (commonErrors.includes(e.name)) {
     return;
   }
   console.warn(`Unexpected ${e.name} on ${fileName}: ${e.message}`);
@@ -469,7 +470,7 @@ function fileDescriptorToFileContext(fd) {
 }
 
 /**
- * Loads the provided file list into the guest without making any file writable.
+ * Loads the provided file list into the guest.
  * Note: code paths can defer loads i.e. `launchWithDirectory()` increment
  * `globalLaunchNumber` to ensure their deferred load is still relevant when it
  * finishes processing. Other code paths that call `sendSnapshotToGuest()` don't
@@ -496,13 +497,13 @@ async function sendSnapshotToGuest(
   if (localLaunchNumber !== globalLaunchNumber) {
     return;
   }
-
   /** @type {!LoadFilesMessage} */
   const loadFilesMessage = {
-    writableFileIndex: focusIndex,
+    currentFileIndex: focusIndex,
     // Handle can't be passed through a message pipe.
     files: snapshot.map(fileDescriptorToFileContext)
   };
+
   // Clear handles to the open files in the privileged context so they are
   // refreshed on a navigation request. The refcount to the File will be alive
   // in the postMessage object until the guest takes its own reference.
@@ -875,7 +876,6 @@ async function advance(direction, currentFileToken) {
   } else {
     entryIndex = -1;
   }
-
   await sendFilesToGuest();
 }
 

chromeos/components/media_app_ui/resources/js/media_app.externs.js

@@ -85,7 +85,7 @@ mediaApp.AbstractFile.prototype.renameOriginalFile;
 /**
  * A function that will save the provided blob in the file pointed to by
  * pickedFileToken. Once saved the new file takes over this.token and becomes
- * currently writable. The original file is given a new token
+ * the current file. The original file is given a new token
  * and pushed forward in the navigation order.
  * @type {function(!Blob, number): !Promise<undefined>|undefined}
  */
@@ -110,11 +110,6 @@ mediaApp.AbstractFileList.prototype.currentFileIndex;
  * @return {(null|!mediaApp.AbstractFile)}
  */
 mediaApp.AbstractFileList.prototype.item = function(index) {};
-/**
- * Returns the file which is currently writable or null if there isn't one.
- * @return {?mediaApp.AbstractFile}
- */
-mediaApp.AbstractFileList.prototype.getCurrentlyWritable = function() {};
 /**
  * Loads the next file in the navigation order into the media app.
  * @param {number=} currentFileToken the token of the file that is currently
@@ -135,12 +130,15 @@ mediaApp.AbstractFileList.prototype.loadPrev = function(currentFileToken) {};
  */
 mediaApp.AbstractFileList.prototype.addObserver = function(observer) {};
 /**
- * Request for the user to be prompted with a open file picker. Once the user
- * selects a file, the file is inserted into the navigation order after the
- * current file and navigated to.
- * @return {!Promise<undefined>}
+ * A function that requests for the user to be prompted with an open file
+ * picker. Once the user selects a file, the file is inserted into the
+ * navigation order after the current file and then navigated to.
+ * TODO(b/165720635): Remove the undefined here once we can ensure all file
+ * lists implement a openFile function.
+ * @type {function(): !Promise<undefined>|undefined}
  */
-mediaApp.AbstractFileList.prototype.openFile = function() {};
+mediaApp.AbstractFileList.prototype.openFile;
+
 /**
  * The delegate which exposes open source privileged WebUi functions to
  * MediaApp.
@@ -171,13 +169,6 @@ mediaApp.ClientApiDelegate.prototype.openFeedbackDialog = function() {};
  */
 mediaApp.ClientApiDelegate.prototype.requestSaveFile = function(
     suggestedName, mimeType) {};
-/**
- * Request for the user to be prompted with a open file picker. Once the user
- * selects a file, the file is inserted into the navigation order after the
- * current file and navigated to.
- * @return {!Promise<undefined>}
- */
-mediaApp.ClientApiDelegate.prototype.openFile = function() {};
 /**
  * Attempts to extract a JPEG "preview" from a RAW image file. Throws on any
  * failure. Note this is typically a full-sized preview, not a thumbnail.

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

@@ -67,7 +67,7 @@ let FileContext;
  * Message sent by the privileged context to the unprivileged context indicating
  * the files available to open.
  * @typedef {{
- *    writableFileIndex: number,
+ *    currentFileIndex: number,
  *    files: !Array<!FileContext>
  * }}
  */

chromeos/components/media_app_ui/resources/js/receiver.js

@@ -107,38 +107,33 @@ class ReceivedFile {
 }
 
 /**
- * Source of truth for what files are loaded in the app and writable. This can
+ * Source of truth for what files are loaded in the app. This can
  * be appended to via `ReceivedFileList.addFiles()`.
  * @type {?ReceivedFileList}
  */
 let lastLoadedReceivedFileList = null;
 
 /**
- * A file list consisting of all files received from the parent. Exposes the
- * currently writable file and all other readable files in the current
- * directory.
+ * A file list consisting of all files received from the parent. Exposes all
+ * readable files in the directory, some of which may be writable.
  * @implements mediaApp.AbstractFileList
  */
 class ReceivedFileList {
   /** @param {!LoadFilesMessage} filesMessage */
   constructor(filesMessage) {
-    // We make sure the 0th item in the list is the writable one so we
-    // don't break older versions of the media app which uses item(0) instead
-    // of getCurrentlyWritable()
-    // TODO(b/151880563): remove this.
-    let writableFileIndex = filesMessage.writableFileIndex;
-    const files = filesMessage.files;
-    while (writableFileIndex > 0) {
-      files.push(files.shift());
-      writableFileIndex--;
+    const {files, currentFileIndex} = filesMessage;
+    if (files.length) {
+      // If we were not provided with a currentFileIndex, default to making the
+      // first file the current file.
+      this.currentFileIndex = currentFileIndex >= 0 ? currentFileIndex : 0;
+    } else {
+      // If we are empty we have no current file.
+      this.currentFileIndex = -1;
     }
 
     this.length = files.length;
-    this.currentFileIndex = files.length ? 0 : -1;
     /** @type {!Array<!ReceivedFile>} */
     this.files = files.map(f => new ReceivedFile(f));
-    /** @type {number} */
-    this.writableFileIndex = 0;
     /** @type {!Array<function(!mediaApp.AbstractFileList): void>} */
     this.observers = [];
   }
@@ -148,15 +143,6 @@ class ReceivedFileList {
     return this.files[index] || null;
   }
 
-  /**
-   * Returns the file which is currently writable or null if there isn't one.
-   * @override
-   * @return {?mediaApp.AbstractFile}
-   */
-  getCurrentlyWritable() {
-    return this.item(this.writableFileIndex);
-  }
-
   /** @override */
   async loadNext(currentFileToken) {
     // Awaiting this message send allows callers to wait for the full effects of
@@ -335,7 +321,7 @@ window.addEventListener('DOMContentLoaded', () => {
 // Ensure that if no files are loaded into the media app there is a default
 // empty file list available.
 window.customLaunchData = {
-  files: new ReceivedFileList({files: [], writableFileIndex: 0})
+  files: new ReceivedFileList({files: [], currentFileIndex: -1})
 };
 
 // Attempting to show file pickers in the sandboxed <iframe> is guaranteed to

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

@@ -77,7 +77,7 @@ class BacklightApp extends HTMLElement {
   /** @override  */
   async loadFiles(files) {
     let child;
-    const file = files.item(0);
+    const file = files.item(files.currentFileIndex);
     await this.preprocessFile(file);
     if (file) {
       const isVideo = file.mimeType.match('^video/');

chromeos/components/media_app_ui/test/driver_api.js

@@ -13,12 +13,12 @@ let TestMessageResponseData;
 
 /**
  * Object sent over postMessage to run a command or extract data.
- * TODO(b/165720635): Remove legacyOpenFile when google3 updates.
  * @typedef {{
  *     deleteLastFile: (boolean|undefined),
  *     getFileErrors: (boolean|undefined),
  *     getLastFileName: (boolean|undefined),
  *     navigate: ({direction: string, token: number}|undefined),
+ *     openFile: (boolean|undefined),
  *     overwriteLastFile: (string|undefined),
  *     pathToRoot: (!Array<string>|undefined),
  *     property: (string|undefined),
@@ -26,8 +26,7 @@ let TestMessageResponseData;
  *     requestFullscreen: (boolean|undefined),
  *     requestSaveFile: (boolean|undefined),
  *     saveAs: (string|undefined),
- *     legacyOpenFile: (boolean|undefined),
- *     openFile: (boolean|undefined),
+ *     suppressCrashReports: (boolean|undefined),
  *     testQuery: string,
  * }}
  */

chromeos/components/media_app_ui/test/guest_query_receiver.js

@@ -17,8 +17,9 @@ const guestTestCases = new Map();
 /**
  * @return {!mediaApp.AbstractFile}
  */
-function firstReceivedItem() {
-  return assertCast(assertCast(lastReceivedFileList).item(0));
+function currentFile() {
+  const fileList = assertCast(lastReceivedFileList);
+  return assertCast(fileList.item(fileList.currentFileIndex));
 }
 
 /**
@@ -44,6 +45,7 @@ async function runTestQuery(data) {
       }
     }
   } else if (data.navigate !== undefined) {
+    // Simulate a user navigating to the next/prev file.
     if (data.navigate.direction === 'next') {
       await assertCast(lastReceivedFileList).loadNext(data.navigate.token);
       result = 'loadNext called';
@@ -54,8 +56,9 @@ async function runTestQuery(data) {
       result = 'nothing called';
     }
   } else if (data.overwriteLastFile) {
+    // Simulate a user overwriting the currently open file.
     const testBlob = new Blob([data.overwriteLastFile]);
-    const file = firstReceivedItem();
+    const file = currentFile();
     await assertCast(file.overwriteOriginal).call(file, testBlob);
     extraResultData = {
       receiverFileName: file.name,
@@ -63,10 +66,10 @@ async function runTestQuery(data) {
     };
     result = 'overwriteOriginal resolved';
   } else if (data.deleteLastFile) {
+    // Simulate a user deleting the currently open file.
     try {
-      const deleteResult =
-          await assertCast(firstReceivedItem().deleteOriginalFile)
-              .call(firstReceivedItem());
+      const deleteResult = await assertCast(currentFile().deleteOriginalFile)
+                               .call(currentFile());
       if (deleteResult === DeleteResult.FILE_MOVED) {
         result = 'deleteOriginalFile resolved file moved';
       } else {
@@ -76,10 +79,10 @@ async function runTestQuery(data) {
       result = `deleteOriginalFile failed Error: ${error}`;
     }
   } else if (data.renameLastFile) {
+    // Simulate a user renaming the currently open file.
     try {
-      const renameResult =
-          await assertCast(firstReceivedItem().renameOriginalFile)
-              .call(firstReceivedItem(), data.renameLastFile);
+      const renameResult = await assertCast(currentFile().renameOriginalFile)
+                               .call(currentFile(), data.renameLastFile);
       if (renameResult === RenameResult.FILE_EXISTS) {
         result = 'renameOriginalFile resolved file exists';
       } else if (
@@ -94,6 +97,7 @@ async function runTestQuery(data) {
       result = `renameOriginalFile failed Error: ${error}`;
     }
   } else if (data.requestSaveFile) {
+    // Call requestSaveFile on the delegate.
     const existingFile = assertCast(lastReceivedFileList).item(0);
     if (!existingFile) {
       result = 'requestSaveFile failed, no file loaded';
@@ -103,11 +107,13 @@ async function runTestQuery(data) {
       result = assertCast(pickedFile.token).toString();
     }
   } else if (data.saveAs) {
+    // Call save as on the first item in the last received file list, simulating
+    // a user clicking save as in the file.
     const existingFile = assertCast(lastReceivedFileList).item(0);
     if (!existingFile) {
       result = 'saveAs failed, no file loaded';
     } else {
-      const file = firstReceivedItem();
+      const file = currentFile();
       try {
         const token = (await DELEGATE.requestSaveFile(
                            existingFile.name, existingFile.mimeType))
@@ -124,15 +130,23 @@ async function runTestQuery(data) {
   } else if (data.getFileErrors) {
     result =
         assertCast(lastReceivedFileList).files.map(file => file.error).join();
-  } else if (data.legacyOpenFile) {
-    // TODO(b/165720635): Remove this once google3 shifts over to using openFile
-    // on the fileList.
-    await DELEGATE.openFile();
   } else if (data.openFile) {
+    // Call open file on file list, simulating a user trying to open a new file.
     await assertCast(lastReceivedFileList).openFile();
   } else if (data.getLastFileName) {
-    result = firstReceivedItem().name;
+    result = currentFile().name;
+  } else if (data.suppressCrashReports) {
+    // TODO(b/172981864): Remove this once we stop triggering crash reports for
+    // NotAFile errors.
+
+    // Remove the implementation of reportError so test code
+    // can safely check that the right errors are being thrown without
+    // triggering a crash.
+    if (chrome) {
+      chrome.crashReportPrivate.reportError = () => {};
+    }
   }
+
   return {testQueryResult: result, testQueryResultData: extraResultData};
 }
 

chromeos/components/media_app_ui/test/media_app_ui_browsertest.js

@@ -418,7 +418,6 @@ TEST_F('MediaAppUIBrowserTest', 'NavigateWithUnopenableSibling', async () => {
     fileToFileHandle(await createTestImageFile(222 /* width */, 10, '2.png')),
     fileToFileHandle(await createTestImageFile(333 /* width */, 10, '3.png')),
   ];
-
   await launchWithHandles(handles);
   let result = await waitForImageAndGetWidth('1.png');
   assertEquals(result, '111');
@@ -454,7 +453,7 @@ TEST_F('MediaAppUIBrowserTest', 'NavigateWithUnopenableSibling', async () => {
   result = await waitForErrorUX();
   assertMatch(result, GENERIC_ERROR_MESSAGE_REGEX);
   assertEquals(currentFiles.length, 3);
-  assertEquals(await getFileErrors(), 'NotAllowedError,,');
+  assertEquals(await getFileErrors(), ',,NotAllowedError');
 
   // Navigating back to an openable file should still work, and the error should
   // "stick".
@@ -470,6 +469,7 @@ TEST_F('MediaAppUIBrowserTest', 'NavigateWithUnopenableSibling', async () => {
 // Tests a hypothetical scenario where a file may be deleted and replaced with
 // an openable directory with the same name while the app is running.
 TEST_F('MediaAppUIBrowserTest', 'FileThatBecomesDirectory', async () => {
+  await sendTestMessage({suppressCrashReports: true});
   const handles = [
     fileToFileHandle(await createTestImageFile(111 /* width */, 10, '1.png')),
     fileToFileHandle(await createTestImageFile(222 /* width */, 10, '2.png')),
@@ -489,7 +489,7 @@ TEST_F('MediaAppUIBrowserTest', 'FileThatBecomesDirectory', async () => {
   result = await waitForErrorUX();
   assertMatch(result, GENERIC_ERROR_MESSAGE_REGEX);
   assertEquals(currentFiles.length, 2);
-  assertEquals(await getFileErrors(), 'NotAFile,');
+  assertEquals(await getFileErrors(), ',NotAFile');
 
   testDone();
 });
@@ -1005,25 +1005,6 @@ TEST_F('MediaAppUIBrowserTest', 'SaveAsErrorHandling', async () => {
   testDone();
 });
 
-// Tests the IPC behind the openFile delegate function.
-// TODO(b/165720635): Remove this once google3 shifts over to using openFile on
-// the fileList.
-TEST_F('MediaAppUIBrowserTest', 'LegacyOpenFileIPC', async () => {
-  const pickedFileHandle = new FakeFileSystemFileHandle('picked_file.jpg');
-  window.showOpenFilePicker = () => Promise.resolve([pickedFileHandle]);
-
-  await sendTestMessage({legacyOpenFile: true});
-
-  const lastToken = [...tokenMap.keys()].slice(-1)[0];
-  assertEquals(entryIndex, 0);
-  assertEquals(currentFiles.length, 1);
-  assertEquals(currentFiles[0].handle, pickedFileHandle);
-  assertEquals(currentFiles[0].handle.name, 'picked_file.jpg');
-  assertEquals(currentFiles[0].token, lastToken);
-  assertEquals(tokenMap.get(currentFiles[0].token), currentFiles[0].handle);
-  testDone();
-});
-
 // Tests the IPC behind the openFile function on receivedFileList.
 TEST_F('MediaAppUIBrowserTest', 'OpenFileIPC', async () => {
   const pickedFileHandle = new FakeFileSystemFileHandle('picked_file.jpg');