Fix zoom PDF param.

The "left" param was ignored because it was applied before the zoom,
and the page still fit in the window at 1x zoom. Scrolling on x was
non-op at this zoom level.

Bug: 319910,64309
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I7b476c018f9972f2b1e0075e933e6489f12e6974
Reviewed-on: https://chromium-review.googlesource.com/817690
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523223}

chrome/browser/resources/pdf/elements/viewer-bookmark/viewer-bookmark.js

@@ -66,9 +66,12 @@ Polymer({
   onClick: function() {
     if (this.bookmark.hasOwnProperty('page')) {
       if (this.bookmark.hasOwnProperty('y')) {
-        this.fire(
-            'change-page-and-y',
-            {page: this.bookmark.page, y: this.bookmark.y, origin: 'bookmark'});
+        this.fire('change-page-and-xy', {
+          page: this.bookmark.page,
+          x: 0,
+          y: this.bookmark.y,
+          origin: 'bookmark'
+        });
       } else {
         this.fire(
             'change-page', {page: this.bookmark.page, origin: 'bookmark'});

chrome/browser/resources/pdf/pdf.js

@@ -232,8 +232,8 @@ function PDFViewer(browserApi) {
       this.metrics.onPageSelectorNavigation();
   });
 
-  document.body.addEventListener('change-page-and-y', e => {
-    this.viewport_.goToPageAndY(e.detail.page, e.detail.y);
+  document.body.addEventListener('change-page-and-xy', e => {
+    this.viewport_.goToPageAndXY(e.detail.page, e.detail.x, e.detail.y);
     if (e.detail.origin == 'bookmark')
       this.metrics.onFollowBookmark();
   });
@@ -526,20 +526,16 @@ PDFViewer.prototype = {
    * @param {Object} params The open params passed in the URL.
    */
   handleURLParams_: function(params) {
-    if (params.page != undefined)
-      this.viewport_.goToPage(params.page);
+    if (params.zoom)
+      this.viewport_.setZoom(params.zoom);
 
     if (params.position) {
-      // Make sure we don't cancel effect of page parameter.
-      this.viewport_.position = {
-        x: this.viewport_.position.x + params.position.x,
-        y: this.viewport_.position.y + params.position.y
-      };
+      this.viewport_.goToPageAndXY(
+          params.page ? params.page : 0, params.position.x, params.position.y);
+    } else if (params.page) {
+      this.viewport_.goToPage(params.page);
     }
 
-    if (params.zoom)
-      this.viewport_.setZoom(params.zoom);
-
     if (params.view) {
       this.isUserInitiatedEvent_ = false;
       this.zoomToolbar_.forceFit(params.view);

chrome/browser/resources/pdf/viewport.js

@@ -798,15 +798,16 @@ Viewport.prototype = {
    * @param {number} page the index of the page to go to. zero-based.
    */
   goToPage: function(page) {
-    this.goToPageAndY(page, 0);
+    this.goToPageAndXY(page, 0, 0);
   },
 
   /**
    * Go to the given y position in the given page index.
    * @param {number} page the index of the page to go to. zero-based.
+   * @param {number} x the x position in the page to go to.
    * @param {number} y the y position in the page to go to.
    */
-  goToPageAndY: function(page, y) {
+  goToPageAndXY: function(page, x, y) {
     this.mightZoom_(() => {
       if (this.pageDimensions_.length === 0)
         return;
@@ -822,7 +823,7 @@ Viewport.prototype = {
       if (!this.isPagedMode())
         toolbarOffset = this.topToolbarHeight_;
       this.position = {
-        x: dimensions.x * this.zoom,
+        x: (dimensions.x + x) * this.zoom,
         y: (dimensions.y + y) * this.zoom - toolbarOffset
       };
       this.updateViewport_();

chrome/test/data/pdf/bookmarks_test.js

@@ -70,37 +70,43 @@ var tests = [
     chrome.test.assertEq(1, subBookmarks.length, "one sub bookmark");
 
     var lastPageChange;
+    var lastXChange;
     var lastYChange;
     var lastUriNavigation;
     bookmarkContent.addEventListener('change-page', function(e) {
       lastPageChange = e.detail.page;
+      lastXChange = undefined;
       lastYChange = undefined;
       lastUriNavigation = undefined;
     });
-    bookmarkContent.addEventListener('change-page-and-y', function(e) {
+    bookmarkContent.addEventListener('change-page-and-xy', function(e) {
       lastPageChange = e.detail.page;
+      lastXChange = e.detail.x;
       lastYChange = e.detail.y;
       lastUriNavigation = undefined;
     });
     bookmarkContent.addEventListener('navigate', function(e) {
       lastPageChange = undefined;
+      lastXChange = undefined;
       lastYChange = undefined;
       lastUriNavigation = e.detail.uri;
     });
 
     function testTapTarget(tapTarget, expectedEvent) {
       lastPageChange = undefined;
+      lastXChange = undefined;
       lastYChange = undefined;
       lastUriNavigation = undefined;
       MockInteractions.tap(tapTarget);
       chrome.test.assertEq(expectedEvent.page, lastPageChange);
+      chrome.test.assertEq(expectedEvent.x, lastXChange);
       chrome.test.assertEq(expectedEvent.y, lastYChange);
       chrome.test.assertEq(expectedEvent.uri, lastUriNavigation);
     }
 
-    testTapTarget(rootBookmarks[0].$.item, {page: 0, y: 166})
-    testTapTarget(subBookmarks[0].$.item, {page: 1, y: 166})
-    testTapTarget(rootBookmarks[1].$.item, {page: 2, y: 166})
+    testTapTarget(rootBookmarks[0].$.item, {page: 0, x: 0, y: 166})
+    testTapTarget(subBookmarks[0].$.item, {page: 1, x: 0, y: 166})
+    testTapTarget(rootBookmarks[1].$.item, {page: 2, x: 0, y: 166})
     testTapTarget(rootBookmarks[2].$.item, {uri: "http://www.chromium.org"})
 
     chrome.test.succeed();

chrome/test/data/pdf/viewport_test.js

@@ -551,6 +551,60 @@ var tests = [
     chrome.test.succeed();
   },
 
+  function testGoToPageAndXY() {
+    var mockWindow = new MockWindow(100, 100);
+    var mockSizer = new MockSizer();
+    var mockCallback = new MockViewportChangedCallback();
+    var viewport = new Viewport(mockWindow, mockSizer, mockCallback.callback,
+                                function() {}, function() {}, function() {},
+                                0, 1, 0);
+    var documentDimensions = new MockDocumentDimensions();
+
+    documentDimensions.addPage(100, 100);
+    documentDimensions.addPage(200, 200);
+    documentDimensions.addPage(100, 400);
+    viewport.setDocumentDimensions(documentDimensions);
+    viewport.setZoom(1);
+
+    mockCallback.reset();
+    viewport.goToPageAndXY(0, 0, 0);
+    chrome.test.assertTrue(mockCallback.wasCalled);
+    chrome.test.assertEq(0, viewport.position.x);
+    chrome.test.assertEq(0, viewport.position.y);
+
+    mockCallback.reset();
+    viewport.goToPageAndXY(1, 0, 0);
+    chrome.test.assertTrue(mockCallback.wasCalled);
+    chrome.test.assertEq(0, viewport.position.x);
+    chrome.test.assertEq(100, viewport.position.y);
+
+    mockCallback.reset();
+    viewport.goToPageAndXY(2, 42, 46);
+    chrome.test.assertTrue(mockCallback.wasCalled);
+    chrome.test.assertEq(0 + 42, viewport.position.x);
+    chrome.test.assertEq(300 + 46, viewport.position.y);
+
+    mockCallback.reset();
+    viewport.goToPageAndXY(2, 42, 0);
+    chrome.test.assertTrue(mockCallback.wasCalled);
+    chrome.test.assertEq(0 + 42, viewport.position.x);
+    chrome.test.assertEq(300, viewport.position.y);
+
+    mockCallback.reset();
+    viewport.goToPageAndXY(2, 0, 46);
+    chrome.test.assertTrue(mockCallback.wasCalled);
+    chrome.test.assertEq(0, viewport.position.x);
+    chrome.test.assertEq(300 + 46, viewport.position.y);
+
+    viewport.setZoom(0.5);
+    mockCallback.reset();
+    viewport.goToPageAndXY(2, 42, 46);
+    chrome.test.assertTrue(mockCallback.wasCalled);
+    chrome.test.assertEq(0 + 21, viewport.position.x);
+    chrome.test.assertEq(150 + 23, viewport.position.y);
+    chrome.test.succeed();
+  },
+
   function testGetPageScreenRect() {
     var mockWindow = new MockWindow(100, 100);
     var mockSizer = new MockSizer();