Make navigation relative to current item.

Previously the media app had an issue where some operations could
update the launch.js side entryIndex without the receiver.js side
knowing about it, i.e A navigation request is sent to launch.js and
actioned but the media_app has unsaved changes and doesn't load the new
file in. In these cases the user sees file 1 but the entryIndex is
file 2, making navigating to the next file confusing as they jump to
file 3. This CL fixes this by making all navigation requests provide
the file that is currently loaded, launch.js then uses this to inform
what the next file should be rather then relying on the possibly
incorrect entry index.

Google 3 side of this change: cl/327574130
BUG: b/163662946

Change-Id: Iece116c5161b8ba4a2345fe343ce9e343eed986b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2366212Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Zain Afzal <zafzal@google.com>
Cr-Commit-Position: refs/heads/master@{#800046}

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

@@ -209,7 +209,7 @@ guestMessagePipe.registerHandler(Message.RENAME_FILE, async (message) => {
 guestMessagePipe.registerHandler(Message.NAVIGATE, async (message) => {
   const navigate = /** @type {!NavigateMessage} */ (message);
 
-  await advance(navigate.direction);
+  await advance(navigate.direction, navigate.currentFileToken);
 });
 
 guestMessagePipe.registerHandler(Message.REQUEST_SAVE_FILE, async (message) => {
@@ -794,10 +794,20 @@ async function launchWithMultipleSelection(directory, handles) {
  * Advance to another file.
  *
  * @param {number} direction How far to advance (e.g. +/-1).
+ * @param {number=} currentFileToken The token of the file that
+ *     direction is in reference to. If unprovided it's assumed that
+ *     currentFiles[entryIndex] is the current file.
  */
-async function advance(direction) {
+async function advance(direction, currentFileToken) {
+  let currIndex = entryIndex;
+  if (currentFileToken) {
+    const fileIndex =
+        currentFiles.findIndex(fd => fd.token === currentFileToken);
+    currIndex = fileIndex === -1 ? currIndex : fileIndex;
+  }
+
   if (currentFiles.length) {
-    entryIndex = (entryIndex + direction) % currentFiles.length;
+    entryIndex = (currIndex + direction) % currentFiles.length;
     if (entryIndex < 0) {
       entryIndex += currentFiles.length;
     }

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

@@ -28,6 +28,12 @@ mediaApp.AbstractFile.prototype.blob;
  * @type {string}
  */
 mediaApp.AbstractFile.prototype.name;
+/**
+ * A unique number that represents this file, used to communicate the file in
+ * IPC with a parent frame.
+ * @type {number|undefined}
+ */
+mediaApp.AbstractFile.prototype.token;
 /**
  * Size of the file, e.g., from the HTML5 File API.
  * @type {number}
@@ -104,15 +110,19 @@ mediaApp.AbstractFileList.prototype.item = function(index) {};
  */
 mediaApp.AbstractFileList.prototype.getCurrentlyWritable = function() {};
 /**
- * Loads in the next file in the list as a writable.
+ * Loads the next file in the navigation order into the media app.
+ * @param {number=} currentFileToken the token of the file that is currently
+ *     loaded into the media app.
  * @return {!Promise<undefined>}
  */
-mediaApp.AbstractFileList.prototype.loadNext = function() {};
+mediaApp.AbstractFileList.prototype.loadNext = function(currentFileToken) {};
 /**
- * Loads in the previous file in the list as a writable.
+ * Loads the previous file in the navigation order into the media app.
+ * @param {number=} currentFileToken the token of the file that is currently
+ *     loaded into the media app.
  * @return {!Promise<undefined>}
  */
-mediaApp.AbstractFileList.prototype.loadPrev = function() {};
+mediaApp.AbstractFileList.prototype.loadPrev = function(currentFileToken) {};
 /**
  * @param {function(!mediaApp.AbstractFileList): void} observer invoked when the
  *     size or contents of the file list changes.

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

@@ -93,8 +93,12 @@ let OverwriteViaFilePickerResponse;
 /**
  * Message sent by the unprivileged context to the privileged context requesting
  * the app be relaunched with the next/previous file in the current directory
- * set to writable. Direction must be either 'next' or 'prev'.
- * @typedef {{direction: number}}
+ * set to writable. Direction is a number specifying how many files to advance
+ * by, positive integers specify files "next" in the navigation order whereas
+ * negative integers specify files "back" in the navigation order.
+ * The `currentFileToken` is the token of the file which is currently opened,
+ * this is used to decide what `direction` is in reference to.
+ * @typedef {{direction: number, currentFileToken: (number|undefined)}}
  */
 let NavigateMessage;
 

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

@@ -156,26 +156,20 @@ class ReceivedFileList {
     return this.item(this.writableFileIndex);
   }
 
-  /**
-   * Loads in the next file in the list as a writable.
-   * @override
-   * @return {!Promise<undefined>}
-   */
-  async loadNext() {
+  /** @override */
+  async loadNext(currentFileToken) {
     // Awaiting this message send allows callers to wait for the full effects of
     // the navigation to complete. This may include a call to load a new set of
     // files, and the initial decode, which replaces this AbstractFileList and
     // alters other app state.
-    await parentMessagePipe.sendMessage(Message.NAVIGATE, {direction: 1});
+    await parentMessagePipe.sendMessage(
+        Message.NAVIGATE, {currentFileToken, direction: 1});
   }
 
-  /**
-   * Loads in the previous file in the list as a writable.
-   * @override
-   * @return {!Promise<undefined>}
-   */
-  async loadPrev() {
-    await parentMessagePipe.sendMessage(Message.NAVIGATE, {direction: -1});
+  /** @override */
+  async loadPrev(currentFileToken) {
+    await parentMessagePipe.sendMessage(
+        Message.NAVIGATE, {currentFileToken, direction: -1});
   }
 
   /** @override */

chromeos/components/media_app_ui/test/driver_api.js

@@ -17,7 +17,7 @@ let TestMessageResponseData;
  *     deleteLastFile: (boolean|undefined),
  *     getFileErrors: (boolean|undefined),
  *     getLastFileName: (boolean|undefined),
- *     navigate: (string|undefined),
+ *     navigate: ({direction: string, token: number}|undefined),
  *     overwriteLastFile: (string|undefined),
  *     pathToRoot: (!Array<string>|undefined),
  *     property: (string|undefined),

chromeos/components/media_app_ui/test/guest_query_receiver.js

@@ -44,11 +44,11 @@ async function runTestQuery(data) {
       }
     }
   } else if (data.navigate !== undefined) {
-    if (data.navigate === 'next') {
-      await assertCast(lastReceivedFileList).loadNext();
+    if (data.navigate.direction === 'next') {
+      await assertCast(lastReceivedFileList).loadNext(data.navigate.token);
       result = 'loadNext called';
-    } else if (data.navigate === 'prev') {
-      await assertCast(lastReceivedFileList).loadPrev();
+    } else if (data.navigate.direction === 'prev') {
+      await assertCast(lastReceivedFileList).loadPrev(data.navigate.token);
       result = 'loadPrev called';
     } else {
       result = 'nothing called';

chromeos/components/media_app_ui/test/media_app_ui_browsertest.js

@@ -707,7 +707,8 @@ TEST_F('MediaAppUIBrowserTest', 'DeletionOpensNextFile', async () => {
   assertEquals('test_file_3.png', lastLoadedFiles[1].name);
 
   // Navigate to the last file (originally the third file) and delete it
-  await sendTestMessage({navigate: 'next'});
+  const token = currentFiles[entryIndex].token;
+  await sendTestMessage({navigate: {direction: 'next', token}});
   testResponse = await sendTestMessage(messageDelete);
 
   assertEquals(
@@ -738,22 +739,63 @@ TEST_F('MediaAppUIBrowserTest', 'DeletionOpensNextFile', async () => {
 TEST_F('MediaAppUIBrowserTest', 'NavigateIPC', async () => {
   await launchWithFiles(
       [await createTestImageFile(), await createTestImageFile()]);
+  const fileOneToken = currentFiles[0].token;
+  const fileTwoToken = currentFiles[1].token;
   assertEquals(entryIndex, 0);
 
-  let result = await sendTestMessage({navigate: 'next'});
+  let result = await sendTestMessage(
+      {navigate: {direction: 'next', token: fileOneToken}});
   assertEquals(result.testQueryResult, 'loadNext called');
   assertEquals(entryIndex, 1);
 
-  result = await sendTestMessage({navigate: 'prev'});
+  result = await sendTestMessage(
+      {navigate: {direction: 'prev', token: fileTwoToken}});
   assertEquals(result.testQueryResult, 'loadPrev called');
   assertEquals(entryIndex, 0);
 
-  result = await sendTestMessage({navigate: 'prev'});
+  result = await sendTestMessage(
+      {navigate: {direction: 'prev', token: fileOneToken}});
   assertEquals(result.testQueryResult, 'loadPrev called');
   assertEquals(entryIndex, 1);
   testDone();
 });
 
+// Tests the loadNext and loadPrev functions on the received file list correctly
+// navigate when they are working with a out of date file list.
+// Regression test for b/163662946
+TEST_F('MediaAppUIBrowserTest', 'NavigateOutOfSync', async () => {
+  await launchWithFiles(
+      [await createTestImageFile(), await createTestImageFile()]);
+  const fileOneToken = currentFiles[0].token;
+  const fileTwoToken = currentFiles[1].token;
+
+  // Simulate some operation updating entryIndex without reloading the media
+  // app.
+  entryIndex = 1;
+
+  let result = await sendTestMessage(
+      {navigate: {direction: 'next', token: fileOneToken}});
+  assertEquals(result.testQueryResult, 'loadNext called');
+  // The media app is focused on file 0 so the next file is file 1.
+  assertEquals(entryIndex, 1);
+
+  entryIndex = 0;
+
+  result = await sendTestMessage(
+      {navigate: {direction: 'prev', token: fileTwoToken}});
+  assertEquals(result.testQueryResult, 'loadPrev called');
+  assertEquals(entryIndex, 0);
+
+  // The received file list and entry index currently agree that the 0th file is
+  // open. Tell loadNext that the 1st file is current to ensure that navigate
+  // respects our provided token over any other signal.
+  result = await sendTestMessage(
+      {navigate: {direction: 'next', token: fileTwoToken}});
+  assertEquals(result.testQueryResult, 'loadNext called');
+  assertEquals(entryIndex, 0);
+  testDone();
+});
+
 // Tests the IPC behind the implementation of ReceivedFile.renameOriginalFile()
 // 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`