Links in PDF should open in a new window when shift + left clicked.

BUG=628057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2300243004
Cr-Commit-Position: refs/heads/master@{#417094}

chrome/browser/pdf/pdf_extension_test.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/plugins/plugin_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_paths.h"
@@ -857,3 +858,36 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionTest, LinkShiftMiddleClick) {
   const GURL& url = active_web_contents->GetURL();
   ASSERT_EQ(std::string("http://www.example.com/"), url.spec());
 }
+
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, LinkShiftLeftClick) {
+  host_resolver()->AddRule("www.example.com", "127.0.0.1");
+  GURL test_pdf_url(embedded_test_server()->GetURL("/pdf/test-link.pdf"));
+  content::WebContents* guest_contents = LoadPdfGetGuestContents(test_pdf_url);
+  ASSERT_TRUE(guest_contents);
+  ASSERT_EQ(1U, chrome::GetTotalBrowserCount());
+
+  // The link position of the test-link.pdf in page coordinates is (110, 110).
+  // Convert the link position from page coordinates to screen coordinates.
+  gfx::Point link_position(110, 110);
+  ConvertPageCoordToScreenCoord(guest_contents, &link_position);
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  content::WindowedNotificationObserver observer(
+      chrome::NOTIFICATION_BROWSER_WINDOW_READY,
+      content::NotificationService::AllSources());
+  content::SimulateMouseClickAt(web_contents, blink::WebInputEvent::ShiftKey,
+                                blink::WebMouseEvent::Button::Left,
+                                link_position);
+  observer.Wait();
+
+  ASSERT_EQ(2U, chrome::GetTotalBrowserCount());
+
+  content::WebContents* active_web_contents =
+      chrome::FindLastActive()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_NE(web_contents, active_web_contents);
+
+  const GURL& url = active_web_contents->GetURL();
+  ASSERT_EQ(std::string("http://www.example.com/"), url.spec());
+}

chrome/browser/resources/pdf/navigator.js

@@ -4,28 +4,79 @@
 
 'use strict';
 
+/**
+ * Creates a new NavigatorDelegate for calling browser-specific functions to
+ * do the actual navigating.
+ * @param {boolean} isInTab Indicates if the PDF viewer is displayed in a tab.
+ * @param {boolean} isSourceFileUrl Indicates if the navigation source is a
+ *     file:// URL.
+ */
+function NavigatorDelegate(isInTab, isSourceFileUrl) {
+  this.isInTab_ = isInTab;
+  this.isSourceFileUrl_ = isSourceFileUrl;
+}
+
 /**
  * Creates a new Navigator for navigating to links inside or outside the PDF.
  * @param {string} originalUrl The original page URL.
  * @param {Object} viewport The viewport info of the page.
  * @param {Object} paramsParser The object for URL parsing.
- * @param {Function} navigateInCurrentTabCallback The Callback function that
- *    gets called when navigation happens in the current tab.
- * @param {Function} navigateInNewTabCallback The Callback function
- *    that gets called when navigation happens in the new tab.
+ * @param {Object} navigatorDelegate The object with callback functions that
+ *    get called when navigation happens in the current tab, a new tab,
+ *    and a new window.
  */
-function Navigator(originalUrl,
-                   viewport,
-                   paramsParser,
-                   navigateInCurrentTabCallback,
-                   navigateInNewTabCallback) {
+function Navigator(originalUrl, viewport, paramsParser, navigatorDelegate) {
   this.originalUrl_ = originalUrl;
   this.viewport_ = viewport;
   this.paramsParser_ = paramsParser;
-  this.navigateInCurrentTabCallback_ = navigateInCurrentTabCallback;
-  this.navigateInNewTabCallback_ = navigateInNewTabCallback;
+  this.navigatorDelegate_ = navigatorDelegate;
 }
 
+NavigatorDelegate.prototype = {
+  /**
+   * @public
+   * Called when navigation should happen in the current tab.
+   * @param {string} url The url to be opened in the current tab.
+   */
+  navigateInCurrentTab: function(url) {
+    // When the PDFviewer is inside a browser tab, prefer the tabs API because
+    // it can navigate from one file:// URL to another.
+    if (chrome.tabs && this.isInTab_ && this.isSourceFileUrl_)
+      chrome.tabs.update({url: url});
+    else
+      window.location.href = url;
+  },
+
+  /**
+   * @public
+   * Called when navigation should happen in the new tab.
+   * @param {string} url The url to be opened in the new tab.
+   * @param {boolean} active Indicates if the new tab should be the active tab.
+   */
+  navigateInNewTab: function(url, active) {
+    // Prefer the tabs API because it guarantees we can just open a new tab.
+    // window.open doesn't have this guarantee.
+    if (chrome.tabs)
+      chrome.tabs.create({url: url, active: active});
+    else
+      window.open(url);
+  },
+
+  /**
+   * @public
+   * Called when navigation should happen in the new window.
+   * @param {string} url The url to be opened in the new window.
+   */
+  navigateInNewWindow: function(url) {
+    // Prefer the windows API because it guarantees we can just open a new
+    // window. window.open with '_blank' argument doesn't have this guarantee.
+    if (chrome.windows)
+      chrome.windows.create({url: url});
+    else
+      window.open(url, '_blank');
+  }
+};
+
 /**
  * Represents options when navigating to a new url. C++ counterpart of
  * the enum is in ui/base/window_open_disposition.h. This enum represents
@@ -78,16 +129,13 @@ Navigator.prototype = {
             url, this.onViewportReceived_.bind(this));
         break;
       case Navigator.WindowOpenDisposition.NEW_BACKGROUND_TAB:
-        this.navigateInNewTabCallback_(url, false);
+        this.navigatorDelegate_.navigateInNewTab(url, false);
         break;
       case Navigator.WindowOpenDisposition.NEW_FOREGROUND_TAB:
-        this.navigateInNewTabCallback_(url, true);
+        this.navigatorDelegate_.navigateInNewTab(url, true);
         break;
       case Navigator.WindowOpenDisposition.NEW_WINDOW:
-        // TODO(jaepark): Shift + left clicking a link in PDF should open the
-        // link in a new window. See http://crbug.com/628057.
-        this.paramsParser_.getViewportFromUrlParams(
-            url, this.onViewportReceived_.bind(this));
+        this.navigatorDelegate_.navigateInNewWindow(url);
         break;
       case Navigator.WindowOpenDisposition.SAVE_TO_DISK:
         // TODO(jaepark): Alt + left clicking a link in PDF should
@@ -121,7 +169,7 @@ Navigator.prototype = {
     if (pageNumber != undefined && originalUrl == newUrl)
       this.viewport_.goToPage(pageNumber);
     else
-      this.navigateInCurrentTabCallback_(viewportPosition.url);
+      this.navigatorDelegate_.navigateInCurrentTab(viewportPosition.url);
   },
 
   /**

chrome/browser/resources/pdf/pdf.js

@@ -39,36 +39,6 @@ function getFilenameFromURL(url) {
   }
 }
 
-/**
- * Called when navigation happens in the current tab.
- * @param {boolean} isInTab Indicates if the PDF viewer is displayed in a tab.
- * @param {boolean} isSourceFileUrl Indicates if the navigation source is a
- *     file:// URL.
- * @param {string} url The url to be opened in the current tab.
- */
-function onNavigateInCurrentTab(isInTab, isSourceFileUrl, url) {
-  // When the PDFviewer is inside a browser tab, prefer the tabs API because
-  // it can navigate from one file:// URL to another.
-  if (chrome.tabs && isInTab && isSourceFileUrl)
-    chrome.tabs.update({url: url});
-  else
-    window.location.href = url;
-}
-
-/**
- * Called when navigation happens in the new tab.
- * @param {string} url The url to be opened in the new tab.
- * @param {boolean} active Indicates if the new tab should be the active tab.
- */
-function onNavigateInNewTab(url, active) {
-  // Prefer the tabs API because it guarantees we can just open a new tab.
-  // window.open doesn't have this guarantee.
-  if (chrome.tabs)
-    chrome.tabs.create({url: url, active: active});
-  else
-    window.open(url);
-}
-
 /**
  * Whether keydown events should currently be ignored. Events are ignored when
  * an editable element has focus, to allow for proper editing controls.
@@ -260,12 +230,9 @@ function PDFViewer(browserApi) {
 
   var isInTab = this.browserApi_.getStreamInfo().tabId != -1;
   var isSourceFileUrl = this.originalUrl_.indexOf('file://') == 0;
-  this.navigator_ = new Navigator(this.originalUrl_,
-                                  this.viewport_, this.paramsParser_,
-                                  onNavigateInCurrentTab.bind(undefined,
-                                                              isInTab,
-                                                              isSourceFileUrl),
-                                  onNavigateInNewTab);
+  this.navigator_ = new Navigator(
+      this.originalUrl_, this.viewport_, this.paramsParser_,
+      new NavigatorDelegate(isInTab, isSourceFileUrl));
   this.viewportScroller_ =
       new ViewportScroller(this.viewport_, this.plugin_, window);