OOP PDF - Add support for "zoom" open pdf parameter

chrome/browser/resources/pdf/pdf.js

Index: chrome/browser/resources/pdf/pdf.js
diff --git a/chrome/browser/resources/pdf/pdf.js b/chrome/browser/resources/pdf/pdf.js
index 2bb5511402abe108e48da0d5f28c52449bf086c0..34c36f6cefc85fe2a6c0fb6a19e32ed386af5c91 100644
--- a/chrome/browser/resources/pdf/pdf.js
+++ b/chrome/browser/resources/pdf/pdf.js
@@ -147,7 +147,10 @@ function PDFViewer() {
       var MIN_ZOOM_DELTA = 0.01;
       var zoomDelta = Math.abs(this.viewport_.zoom -
                                zoomChangeInfo.newZoomFactor);
-      if (zoomDelta > MIN_ZOOM_DELTA)
+      // We should not change zoom level when we are responsible for initiating
+      // the zoom. onZoomChange() is called before setZoomComplete() callback
+      // when we initiate the zoom.
+      if ((zoomDelta > MIN_ZOOM_DELTA) && !this.setZoomInProgress_)

[Nikhil, 2014/08/29 10:31:47]

Should this be a part of this CL or a separate one? Maybe associate it with OOP
PDF Issue - 303491.

[raymes, 2014/09/01 01:04:58]

It's fine to keep it here.

[Nikhil, 2014/09/01 09:11:44]

Acknowledged.

         this.viewport_.setZoom(zoomChangeInfo.newZoomFactor);
     }.bind(this));
   }
@@ -270,6 +273,27 @@ PDFViewer.prototype = {
 
   /**
    * @private
+   * 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.
+   */
+  handleURLParams_: function() {
+    if (this.urlParams_.page)
+      this.viewport_.goToPage(this.urlParams_.page);
+    if (this.urlParams_.position) {
+      // Make sure we don't cancel effect of page parameter.
+      var currentPosition = this.viewport_.position;
+      var destinationPosition = {};
+      destinationPosition['x'] = currentPosition.x + this.urlParams_.position.x;
+      destinationPosition['y'] = currentPosition.y + this.urlParams_.position.y;

[raymes, 2014/09/01 01:04:58]

Is this the way that adobe reader works (does it add the position param to the
page param)? I would have though it would either choose one or the other.

Maybe you could just write this as:
this.viewport_.position = {
  x: this.viewport_.position.x + this.urlParams_.position.x;
  y: this.viewport_.position.y + this.urlParams_.position.y;
};

[Nikhil, 2014/09/01 09:11:44]

Please consider the following use case -
example.com/test.pdf#page=2&zoom=120,150,150 (or #zoom=120,150,150&page=2)

The above should zoom the position (150,150) of page 2 to 120% zoom level. Same
behavior is observed with Adobe Reader as pdf viewer. I think Firefox doesn't
support zoom parameter as of now.

To support above behavior, current implementation is in the following way -
[1] Go to page if mentioned
[2] Adjust position if mentioned (using current position)
[3] Set zoom level if mentioned

Hmm, maybe we should factor in current position in [2] only if page param is
passed? I think it could be possible for authors to specify a position and
hyperlink it to navigate up/down in a big document, in which case I think
current logic will fail. Since it will always use _current_ position and add to
it. But, I can't seem to find such a pdf to test this. 

Please let me know WDYT.

[raymes, 2014/09/02 03:27:36]

The way you have written it looks fine. But I think you can simplify the code as
I suggested in my previous comment.

+      this.viewport_.position = destinationPosition;
+    }
+    if (this.urlParams_.zoom)
+      this.viewport_.setZoom(this.urlParams_.zoom);
+  },
+
+  /**
+   * @private
    * Update the loading progress of the document in response to a progress
    * message being received from the plugin.
    * @param {number} progress the progress as a percentage.
@@ -287,19 +311,15 @@ PDFViewer.prototype = {
       }
     } else if (progress == 100) {
       // Document load complete.
+      if (this.lastViewportPosition_)
+        this.viewport_.position = this.lastViewportPosition_;
+      this.handleURLParams_();
       this.loaded = true;
       var loadEvent = new Event('pdfload');
       window.dispatchEvent(loadEvent);
       this.sendScriptingMessage_({
         type: 'documentLoaded'
       });
-      if (this.lastViewportPosition_)
-        this.viewport_.position = this.lastViewportPosition_;
-
-      // Handle open pdf params. Order is important as later actions can
-      // override the effects of previous actions.
-      if (this.urlParams_.page)
-        this.viewport_.goToPage(this.urlParams_.page);
     }
   },