PDF Viewer: Return a Promise from getViewportFromUrlParams().

The API was previously callback-based, instead of Promise-based.
Callback-based APIs are more tedious to use. As a concrete example,
the PDFExtensionJSTest.ParamsParser tests were incorrectly not waiting
for any of the callbacks to execute before declaring the test
successful.

In the process also
 - Changed Navigator#navigate() to return a Promise, instead of
   firing a 'navigate-for-testing' event.
 - Fixed incorrect type annotation for OpenPdfParams#viewPosition.

Bug: None
Change-Id: Ib14b6499d3c661bb62c21c1836324ceecf1494ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2618219
Commit-Queue: Daniel Hosseinian <dhoss@chromium.org>
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Auto-Submit: dpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842168}

chrome/browser/resources/pdf/navigator.js

@@ -2,7 +2,6 @@
 // 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';
 
@@ -104,14 +103,6 @@ export class PdfNavigator {
 
     /** @private {!NavigatorDelegate} */
     this.navigatorDelegate_ = navigatorDelegate;
-
-    /** @private {!EventTarget} */
-    this.eventTarget_ = new EventTarget();
-  }
-
-  /** @return {!EventTarget} */
-  getEventTarget() {
-    return this.eventTarget_;
   }
 
   /**
@@ -120,10 +111,11 @@ export class PdfNavigator {
    * @param {string} urlString The URL to navigate to.
    * @param {!WindowOpenDisposition} disposition The window open
    *     disposition when navigating to the new URL.
+   * @return {!Promise<void>} When navigation has completed (used for testing).
    */
   navigate(urlString, disposition) {
     if (urlString.length === 0) {
-      return;
+      return Promise.resolve();
     }
 
     // If |urlFragment| starts with '#', then it's for the same URL with a
@@ -145,17 +137,19 @@ export class PdfNavigator {
     try {
       url = new URL(urlString);
     } catch (err) {
-      return;
+      return Promise.reject(err);
     }
 
     if (!this.isValidUrl_(url)) {
-      return;
+      return Promise.resolve();
     }
 
+    let whenDone = Promise.resolve();
+
     switch (disposition) {
       case WindowOpenDisposition.CURRENT_TAB:
-        this.paramsParser_.getViewportFromUrlParams(
-            url.href, this.onViewportReceived_.bind(this));
+        whenDone = this.paramsParser_.getViewportFromUrlParams(url.href).then(
+            this.onViewportReceived_.bind(this));
         break;
       case WindowOpenDisposition.NEW_BACKGROUND_TAB:
         this.navigatorDelegate_.navigateInNewTab(url.href, false);
@@ -169,15 +163,14 @@ export class PdfNavigator {
       case WindowOpenDisposition.SAVE_TO_DISK:
         // TODO(jaepark): Alt + left clicking a link in PDF should
         // download the link.
-        this.paramsParser_.getViewportFromUrlParams(
-            url.href, this.onViewportReceived_.bind(this));
+        whenDone = this.paramsParser_.getViewportFromUrlParams(url.href).then(
+            this.onViewportReceived_.bind(this));
         break;
       default:
         break;
     }
 
-    // Dispatch events for tests.
-    this.eventTarget_.dispatchEvent(new CustomEvent('navigate-for-testing'));
+    return whenDone;
   }
 
   /**

chrome/browser/resources/pdf/open_pdf_params_parser.js

@@ -11,11 +11,11 @@ import {Size} from './viewport.js';
  *   url: (string|undefined),
  *   zoom: (number|undefined),
  *   view: (!FittingType|undefined),
- *   viewPosition: (!Point|undefined),
+ *   viewPosition: (number|undefined),
  *   position: (!Object|undefined),
  * }}
  */
-let OpenPdfParams;
+export let OpenPdfParams;
 
 // Parses the open pdf parameters passed in the url to set initial viewport
 // settings for opening the pdf.
@@ -224,10 +224,9 @@ export class OpenPdfParamsParser {
    * See http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/
    * pdfs/pdf_open_parameters.pdf for details.
    * @param {string} url that needs to be parsed.
-   * @param {function(!OpenPdfParams)} callback function to be called with
-   *     viewport info.
+   * @return {!Promise<!OpenPdfParams>}
    */
-  getViewportFromUrlParams(url, callback) {
+  async getViewportFromUrlParams(url) {
     const params = {};
     params['url'] = url;
 
@@ -254,22 +253,23 @@ export class OpenPdfParamsParser {
     }
 
     if (params.page === undefined && urlParams.has('nameddest')) {
-      this.getNamedDestinationCallback_(
-              /** @type {string} */ (urlParams.get('nameddest')))
-          .then(data => {
-            if (data.pageNumber !== -1) {
-              params.page = data.pageNumber;
-            }
-            if (data.namedDestinationView) {
-              Object.assign(
-                  params,
-                  this.parseNameddestViewParam_(
-                      /** @type {string} */ (data.namedDestinationView)));
-            }
-            callback(params);
-          });
-    } else {
-      callback(params);
+      const data = await this.getNamedDestinationCallback_(
+          /** @type {string} */ (urlParams.get('nameddest')));
+
+      if (data.pageNumber !== -1) {
+        params.page = data.pageNumber;
+      }
+
+      if (data.namedDestinationView) {
+        Object.assign(
+            params,
+            this.parseNameddestViewParam_(
+                /** @type {string} */ (data.namedDestinationView)));
+      }
+
+      return params;
     }
+
+    return Promise.resolve(params);
   }
 }

chrome/browser/resources/pdf/pdf_viewer_base.js

@@ -12,7 +12,7 @@ import {BrowserApi, ZoomBehavior} from './browser_api.js';
 import {FittingType, Point} from './constants.js';
 import {ContentController, MessageData, PluginController, PluginControllerEventType} from './controller.js';
 import {PDFMetrics, UserAction} from './metrics.js';
-import {OpenPdfParamsParser} from './open_pdf_params_parser.js';
+import {OpenPdfParams, OpenPdfParamsParser} from './open_pdf_params_parser.js';
 import {LoadState} from './pdf_scripting_api.js';
 import {DocumentDimensionsMessageData, MessageObject} from './pdf_viewer_utils.js';
 import {Viewport} from './viewport.js';
@@ -318,8 +318,8 @@ export class PDFViewerBaseElement extends PolymerElement {
       if (this.lastViewportPosition) {
         this.viewport_.position = this.lastViewportPosition;
       }
-      this.paramsParser.getViewportFromUrlParams(
-          this.originalUrl, params => this.handleURLParams_(params));
+      this.paramsParser.getViewportFromUrlParams(this.originalUrl)
+          .then(this.handleURLParams_.bind(this));
       this.setLoadState(LoadState.SUCCESS);
       this.sendDocumentLoadedMessage();
       while (this.delayedScriptingMessages_.length > 0) {
@@ -497,7 +497,7 @@ export class PDFViewerBaseElement extends PolymerElement {
    * Handle open pdf parameters. This function updates the viewport as per
    * the parameters mentioned in the url while opening pdf. The order is
    * important as later actions can override the effects of previous actions.
-   * @param {Object} params The open params passed in the URL.
+   * @param {!OpenPdfParams} params The open params passed in the URL.
    * @private
    */
   handleURLParams_(params) {

chrome/test/data/pdf/navigator_test.js

@@ -2,7 +2,6 @@
 // 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';
@@ -56,12 +55,12 @@ class MockNavigatorDelegate {
  * @param {!MockViewportChangedCallback} viewportChangedCallback
  * @param {!MockNavigatorDelegate} navigatorDelegate
  */
-function doNavigationUrlTest(
+async function doNavigationUrlTest(
     navigator, url, disposition, expectedResultUrl, viewportChangedCallback,
     navigatorDelegate) {
   viewportChangedCallback.reset();
   navigatorDelegate.reset();
-  navigator.navigate(url, disposition);
+  await navigator.navigate(url, disposition);
   chrome.test.assertFalse(viewportChangedCallback.wasCalled);
   chrome.test.assertEq(expectedResultUrl, navigatorDelegate.url);
   if (expectedResultUrl === undefined) {
@@ -89,7 +88,7 @@ function doNavigationUrlTest(
  * @param {string} url
  * @param {(string|undefined)} expectedResultUrl
  */
-function doNavigationUrlTests(originalUrl, url, expectedResultUrl) {
+async function doNavigationUrlTests(originalUrl, url, expectedResultUrl) {
   const mockWindow = new MockElement(100, 100, null);
   const mockSizer = new MockSizer();
   const mockViewportChangedCallback = new MockViewportChangedCallback();
@@ -105,13 +104,13 @@ function doNavigationUrlTests(originalUrl, url, expectedResultUrl) {
   const navigator =
       new PdfNavigator(originalUrl, viewport, paramsParser, navigatorDelegate);
 
-  doNavigationUrlTest(
+  await doNavigationUrlTest(
       navigator, url, WindowOpenDisposition.CURRENT_TAB, expectedResultUrl,
       mockViewportChangedCallback, navigatorDelegate);
-  doNavigationUrlTest(
+  await doNavigationUrlTest(
       navigator, url, WindowOpenDisposition.NEW_BACKGROUND_TAB,
       expectedResultUrl, mockViewportChangedCallback, navigatorDelegate);
-  doNavigationUrlTest(
+  await doNavigationUrlTest(
       navigator, url, WindowOpenDisposition.NEW_WINDOW, expectedResultUrl,
       mockViewportChangedCallback, navigatorDelegate);
 }
@@ -154,47 +153,36 @@ const tests = [
     viewport.setZoom(1);
 
     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;
+    await 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();
-    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;
+    await 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();
-    navigatingDone =
-        eventToPromise('navigate-for-testing', navigator.getEventTarget());
     // This should move viewport to page 2.
-    navigator.navigate(url + '#UY', WindowOpenDisposition.CURRENT_TAB);
-    await navigatingDone;
+    await 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();
-    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;
+    await navigator.navigate(url + '#ABC', WindowOpenDisposition.CURRENT_TAB);
     chrome.test.assertFalse(mockCallback.wasCalled);
     chrome.test.assertTrue(navigatorDelegate.navigateInCurrentTabCalled);
     chrome.test.assertEq(0, viewport.position.x);
@@ -207,44 +195,45 @@ const tests = [
    * a valid scheme, so the navigator must determine the url by following
    * similar heuristics as Adobe Acrobat Reader.
    */
-  function testNavigateForLinksWithoutScheme() {
+  async function testNavigateForLinksWithoutScheme() {
     const url = 'http://www.example.com/subdir/xyz.pdf';
 
     // Sanity check.
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, 'https://www.foo.com/bar.pdf', 'https://www.foo.com/bar.pdf');
 
     // Open relative links.
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, 'foo/bar.pdf', 'http://www.example.com/subdir/foo/bar.pdf');
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, 'foo.com/bar.pdf',
         'http://www.example.com/subdir/foo.com/bar.pdf');
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, '../www.foo.com/bar.pdf',
         'http://www.example.com/www.foo.com/bar.pdf');
 
     // Open an absolute link.
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, '/foodotcom/bar.pdf', 'http://www.example.com/foodotcom/bar.pdf');
 
     // Open a http url without a scheme.
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, 'www.foo.com/bar.pdf', 'http://www.foo.com/bar.pdf');
 
     // Test three dots.
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, '.../bar.pdf', 'http://www.example.com/subdir/.../bar.pdf');
 
     // Test forward slashes.
-    doNavigationUrlTests(url, '..\\bar.pdf', 'http://www.example.com/bar.pdf');
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
+        url, '..\\bar.pdf', 'http://www.example.com/bar.pdf');
+    await doNavigationUrlTests(
         url, '.\\bar.pdf', 'http://www.example.com/subdir/bar.pdf');
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, '\\bar.pdf', 'http://www.example.com/subdir//bar.pdf');
 
     // Regression test for https://crbug.com/569040
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, 'http://something.else/foo#page=5',
         'http://something.else/foo#page=5');
 
@@ -254,31 +243,31 @@ const tests = [
    * Test opening a url in the same tab, in a new tab, and in a new window with
    * a file:/// url as the current location.
    */
-  function testNavigateFromLocalFile() {
+  async function testNavigateFromLocalFile() {
     const url = 'file:///some/path/to/myfile.pdf';
 
     // Open an absolute link.
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, '/foodotcom/bar.pdf', 'file:///foodotcom/bar.pdf');
 
     chrome.test.succeed();
   },
 
-  function testNavigateInvalidUrls() {
+  async function testNavigateInvalidUrls() {
     const url = 'https://example.com/some-web-document.pdf';
 
     // From non-file: to file:
-    doNavigationUrlTests(url, 'file:///bar.pdf', undefined);
+    await doNavigationUrlTests(url, 'file:///bar.pdf', undefined);
 
-    doNavigationUrlTests(url, 'chrome://version', undefined);
+    await doNavigationUrlTests(url, 'chrome://version', undefined);
 
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, 'javascript:// this is not a document.pdf', undefined);
 
-    doNavigationUrlTests(
+    await doNavigationUrlTests(
         url, 'this-is-not-a-valid-scheme://path.pdf', undefined);
 
-    doNavigationUrlTests(url, '', undefined);
+    await doNavigationUrlTests(url, '', undefined);
 
     chrome.test.succeed();
   }