Avoid storing pending requests in OpenPDFParamsParser.

OpenPDFParamsParser currently maintains an array of requests and assumes
they will get replied in the same order. By changing the return type of
getNamedDestinationCallback() to a promise, this CL enables
getViewportFromUrlParams() to handle the data returned by the plugin
directly, without storing all the requests inside OpenPDFParamsParser.

This CL also removes the usage of onNamedDestinationReceived() from
browser tests.

Bug: 55776,535978,748852
Change-Id: If6e8285a363d2efee169457856ca465cb498805a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2391102
Commit-Queue: Hui Yingst <nigi@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809449}

chrome/browser/resources/pdf/constants.js

@@ -28,6 +28,14 @@ export const FittingType = {
   FIT_TO_HEIGHT: 'fit-to-height',
 };
 
+/**
+ * @typedef {{
+ *   messageId: string,
+ *   pageNumber: number,
+ * }}
+ */
+export let NamedDestinationMessageData;
+
 /**
  * Enumeration of save message request types. Must Match SaveRequestType in
  * pdf/out_of_process_instance.h.

chrome/browser/resources/pdf/controller.js

@@ -7,7 +7,7 @@ import {NativeEventTarget as EventTarget} from 'chrome://resources/js/cr/event_t
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
 import {PromiseResolver} from 'chrome://resources/js/promise_resolver.m.js';
 
-import {Point, SaveRequestType} from './constants.js';
+import {NamedDestinationMessageData, Point, SaveRequestType} from './constants.js';
 import {PartialPoint, PinchPhase, Viewport} from './viewport.js';
 
 /** @typedef {{type: string, messageId: (string|undefined)}} */
@@ -354,7 +354,11 @@ export class PluginController extends ContentController {
     this.postMessage_({type: 'getPasswordComplete', password: password});
   }
 
-  /** @param {string} destination */
+  /**
+   * @param {string} destination
+   * @return {!Promise<!NamedDestinationMessageData>}
+   *     A promise holding the named destination information from the plugin.
+   */
   getNamedDestination(destination) {
     return this.postMessageWithReply_({
       type: 'getNamedDestination',

chrome/browser/resources/pdf/navigator.js

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import {NativeEventTarget as EventTarget} from 'chrome://resources/js/cr/event_target.m.js';
 import {OpenPdfParamsParser} from './open_pdf_params_parser.js';
 import {Viewport} from './viewport.js';
 
@@ -103,6 +104,14 @@ export class PdfNavigator {
 
     /** @private {!NavigatorDelegate} */
     this.navigatorDelegate_ = navigatorDelegate;
+
+    /** @private {!EventTarget} */
+    this.eventTarget_ = new EventTarget();
+  }
+
+  /** @return {!EventTarget} */
+  getEventTarget() {
+    return this.eventTarget_;
   }
 
   /**
@@ -166,6 +175,9 @@ export class PdfNavigator {
       default:
         break;
     }
+
+    // Dispatch events for tests.
+    this.eventTarget_.dispatchEvent(new CustomEvent('navigate-for-testing'));
   }
 
   /**

chrome/browser/resources/pdf/open_pdf_params_parser.js

@@ -2,7 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import {FittingType, Point} from './constants.js';
+import {assert} from 'chrome://resources/js/assert.m.js';
+import {FittingType, NamedDestinationMessageData, Point} from './constants.js';
 
 /**
  * @typedef {{
@@ -18,14 +19,12 @@ let OpenPdfParams;
 // settings for opening the pdf.
 export class OpenPdfParamsParser {
   /**
-   * @param {function(string):void} getNamedDestinationCallback
-   *     Function called to fetch information for a named destination.
+   * @param {function(string):!Promise<!NamedDestinationMessageData>}
+   *     getNamedDestinationCallback Function called to fetch information for a
+   *     named destination.
    */
   constructor(getNamedDestinationCallback) {
-    /** @private {!Array<!Object>} */
-    this.outstandingRequests_ = [];
-
-    /** @private {!function(string):void} */
+    /** @private {!function(string):!Promise<!NamedDestinationMessageData>} */
     this.getNamedDestinationCallback_ = getNamedDestinationCallback;
   }
 
@@ -168,25 +167,16 @@ export class OpenPdfParamsParser {
     }
 
     if (params.page === undefined && urlParams.has('nameddest')) {
-      this.outstandingRequests_.push({callback: callback, params: params});
       this.getNamedDestinationCallback_(
-          /** @type {string} */ (urlParams.get('nameddest')));
+              /** @type {string} */ (urlParams.get('nameddest')))
+          .then(data => {
+            if (data.pageNumber !== -1) {
+              params.page = data.pageNumber;
+            }
+            callback(params);
+          });
     } else {
       callback(params);
     }
   }
-
-  /**
-   * This is called when a named destination is received and the page number
-   * corresponding to the request for which a named destination is passed.
-   * @param {number} pageNumber The page corresponding to the named destination
-   *    requested.
-   */
-  onNamedDestinationReceived(pageNumber) {
-    const outstandingRequest = this.outstandingRequests_.shift();
-    if (pageNumber !== -1) {
-      outstandingRequest.params.page = pageNumber;
-    }
-    outstandingRequest.callback(outstandingRequest.params);
-  }
 }

chrome/browser/resources/pdf/pdf_viewer_base.js

@@ -223,10 +223,7 @@ export class PDFViewerBaseElement extends PolymerElement {
 
     // Parse open pdf parameters.
     this.paramsParser = new OpenPdfParamsParser(destination => {
-      this.pluginController_.getNamedDestination(destination).then(data => {
-        this.paramsParser.onNamedDestinationReceived(
-            /** @type {{ pageNumber: number }} */ (data).pageNumber);
-      });
+      return this.pluginController_.getNamedDestination(destination);
     });
 
     // Can only reload if we are in a normal tab.

chrome/test/data/pdf/navigator_test.js

@@ -2,11 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import {eventToPromise} from 'chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/_test_resources/webui/test_util.m.js';
 import {NavigatorDelegate, PdfNavigator, WindowOpenDisposition} from 'chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/navigator.js';
 import {OpenPdfParamsParser} from 'chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/open_pdf_params_parser.js';
 import {PDFScriptingAPI} from 'chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/pdf_scripting_api.js';
 
-import {getZoomableViewport, MockDocumentDimensions, MockElement, MockSizer, MockViewportChangedCallback} from './test_util.js';
+import {getZoomableViewport, MockDocumentDimensions, MockElement, MockSizer, MockViewportChangedCallback, testAsync} from './test_util.js';
 
 /** @implements {NavigatorDelegate} */
 class MockNavigatorDelegate {
@@ -96,7 +97,8 @@ function doNavigationUrlTests(originalUrl, url, expectedResultUrl) {
   viewport.setViewportChangedCallback(mockViewportChangedCallback.callback);
 
   const paramsParser = new OpenPdfParamsParser(function(name) {
-    paramsParser.onNamedDestinationReceived(-1);
+    return Promise.resolve(
+        {messageId: 'getNamedDestination_1', pageNumber: -1});
   });
 
   const navigatorDelegate = new MockNavigatorDelegate();
@@ -128,11 +130,14 @@ const tests = [
 
     const paramsParser = new OpenPdfParamsParser(function(destination) {
       if (destination === 'US') {
-        paramsParser.onNamedDestinationReceived(0);
+        return Promise.resolve(
+            {messageId: 'getNamedDestination_1', pageNumber: 0});
       } else if (destination === 'UY') {
-        paramsParser.onNamedDestinationReceived(2);
+        return Promise.resolve(
+            {messageId: 'getNamedDestination_2', pageNumber: 2});
       } else {
-        paramsParser.onNamedDestinationReceived(-1);
+        return Promise.resolve(
+            {messageId: 'getNamedDestination_3', pageNumber: -1});
       }
     });
     const url = 'http://xyz.pdf';
@@ -148,42 +153,54 @@ const tests = [
     viewport.setDocumentDimensions(documentDimensions);
     viewport.setZoom(1);
 
-    mockCallback.reset();
-    // This should move viewport to page 0.
-    navigator.navigate(url + '#US', WindowOpenDisposition.CURRENT_TAB);
-    chrome.test.assertTrue(mockCallback.wasCalled);
-    chrome.test.assertEq(0, viewport.position.x);
-    chrome.test.assertEq(0, viewport.position.y);
-
-    mockCallback.reset();
-    navigatorDelegate.reset();
-    // This should open "http://xyz.pdf#US" in a new tab. So current tab
-    // viewport should not update and viewport position should remain same.
-    navigator.navigate(url + '#US', WindowOpenDisposition.NEW_BACKGROUND_TAB);
-    chrome.test.assertFalse(mockCallback.wasCalled);
-    chrome.test.assertTrue(navigatorDelegate.navigateInNewTabCalled);
-    chrome.test.assertEq(0, viewport.position.x);
-    chrome.test.assertEq(0, viewport.position.y);
-
-    mockCallback.reset();
-    // This should move viewport to page 2.
-    navigator.navigate(url + '#UY', WindowOpenDisposition.CURRENT_TAB);
-    chrome.test.assertTrue(mockCallback.wasCalled);
-    chrome.test.assertEq(0, viewport.position.x);
-    chrome.test.assertEq(300, viewport.position.y);
-
-    mockCallback.reset();
-    navigatorDelegate.reset();
-    // #ABC is not a named destination in the page so viewport should not
-    // update and viewport position should remain same. As this link will open
-    // in the same tab.
-    navigator.navigate(url + '#ABC', WindowOpenDisposition.CURRENT_TAB);
-    chrome.test.assertFalse(mockCallback.wasCalled);
-    chrome.test.assertTrue(navigatorDelegate.navigateInCurrentTabCalled);
-    chrome.test.assertEq(0, viewport.position.x);
-    chrome.test.assertEq(300, viewport.position.y);
-
-    chrome.test.succeed();
+    testAsync(async () => {
+      mockCallback.reset();
+      let navigatingDone =
+          eventToPromise('navigate-for-testing', navigator.getEventTarget());
+      // This should move viewport to page 0.
+      navigator.navigate(url + '#US', WindowOpenDisposition.CURRENT_TAB);
+      await navigatingDone;
+      chrome.test.assertTrue(mockCallback.wasCalled);
+      chrome.test.assertEq(0, viewport.position.x);
+      chrome.test.assertEq(0, viewport.position.y);
+
+      mockCallback.reset();
+      navigatorDelegate.reset();
+      navigatingDone =
+          eventToPromise('navigate-for-testing', navigator.getEventTarget());
+      // This should open "http://xyz.pdf#US" in a new tab. So current tab
+      // viewport should not update and viewport position should remain same.
+      navigator.navigate(url + '#US', WindowOpenDisposition.NEW_BACKGROUND_TAB);
+      await navigatingDone;
+      chrome.test.assertFalse(mockCallback.wasCalled);
+      chrome.test.assertTrue(navigatorDelegate.navigateInNewTabCalled);
+      chrome.test.assertEq(0, viewport.position.x);
+      chrome.test.assertEq(0, viewport.position.y);
+
+      mockCallback.reset();
+      navigatingDone =
+          eventToPromise('navigate-for-testing', navigator.getEventTarget());
+      // This should move viewport to page 2.
+      navigator.navigate(url + '#UY', WindowOpenDisposition.CURRENT_TAB);
+      await navigatingDone;
+      chrome.test.assertTrue(mockCallback.wasCalled);
+      chrome.test.assertEq(0, viewport.position.x);
+      chrome.test.assertEq(300, viewport.position.y);
+
+      mockCallback.reset();
+      navigatorDelegate.reset();
+      navigatingDone =
+          eventToPromise('navigate-for-testing', navigator.getEventTarget());
+      // #ABC is not a named destination in the page so viewport should not
+      // update, and the viewport position should remain same as testNavigate3's
+      // navigating results, as this link will open in the same tab.
+      navigator.navigate(url + '#ABC', WindowOpenDisposition.CURRENT_TAB);
+      await navigatingDone;
+      chrome.test.assertFalse(mockCallback.wasCalled);
+      chrome.test.assertTrue(navigatorDelegate.navigateInCurrentTabCalled);
+      chrome.test.assertEq(0, viewport.position.x);
+      chrome.test.assertEq(300, viewport.position.y);
+    });
   },
   /**
    * Test opening a url in the same tab, in a new tab, and in a new window for

chrome/test/data/pdf/params_parser_test.js

@@ -14,13 +14,17 @@ const tests = [
   function testParamsParser() {
     const paramsParser = new OpenPdfParamsParser(function(destination) {
       if (destination === 'RU') {
-        paramsParser.onNamedDestinationReceived(26);
+        return Promise.resolve(
+            {messageId: 'getNamedDestination_1', pageNumber: 26});
       } else if (destination === 'US') {
-        paramsParser.onNamedDestinationReceived(0);
+        return Promise.resolve(
+            {messageId: 'getNamedDestination_2', pageNumber: 0});
       } else if (destination === 'UY') {
-        paramsParser.onNamedDestinationReceived(22);
+        return Promise.resolve(
+            {messageId: 'getNamedDestination_3', pageNumber: 22});
       } else {
-        paramsParser.onNamedDestinationReceived(-1);
+        return Promise.resolve(
+            {messageId: 'getNamedDestination_4', pageNumber: -1});
       }
     });