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 c5920f877dc10c80a416c95c23b8fb7ab4ebc105..d0d99cc83376874309916021524b458c5031d628 100644
--- a/chrome/browser/resources/pdf/pdf.js
+++ b/chrome/browser/resources/pdf/pdf.js
@@ -146,7 +146,7 @@ function PDFViewer() {
       var MIN_ZOOM_DELTA = 0.01;
       var zoomDelta = Math.abs(this.viewport_.zoom -
                                zoomChangeInfo.newZoomFactor);
-      if (zoomDelta > MIN_ZOOM_DELTA)
+      if ((zoomDelta > MIN_ZOOM_DELTA) && !this.setZoomInProgress_)

[Nikhil, 2014/08/21 11:36:50]

This was again calling setZoom() with older zoom factor even though we are in
between of zoom triggered by #zoom parameter.
 
For test.pdf#zoom=120
this.viewport_.zoom = 1.2
zoomChangeInfo.newZoomFactor = 1

Since the delta was greater than MIN_ZOOM_DELTA, setZoom() was again called
setting viewport_.zoom to 1 and canceling the effect of #zoom parameter.

Please let me know if this fix is acceptable. If so, I'd really like to move it
to separate CL to keep this CL only about 'zoom' param implementation :) 

[raymes, 2014/08/22 03:52:21]

Hmm I'm not sure if this is completely correct. Basically it means the order of
zoom events isn't preserved. For example if I do the following:
1) Initiate a zoom through the plugin
2) Initiate a zoom from the browser

Then the affect of (2) might be ignored, which isn't what we want. 

The problem here is that you want to set an "initial zoom". Have a look in
Viewport.setDocumentDimensions(). This is where we set the initial zoom (look
inside the block which has 
if (initialDimensions) {...

So I think we should add a member to Viewport (Viewport.initialViewport) which
is used in setDocumentDimensions (rather than the current way it computes the
initial zoom). initialViewport could be a dictionary which contains the initial
position/page to scroll to as well, which could also be set in that method.

Then we could actually just call handleOpenPDFParams_ from the PDFViewer
constructor.

         this.viewport_.setZoom(zoomChangeInfo.newZoomFactor);
     }.bind(this));
   }
@@ -264,10 +264,38 @@ PDFViewer.prototype = {
   },
 
   /**
+   * Handle zoom parameter of open PDF parameters. If this
+   * parameter is passed while opening PDF then PDF should be opened
+   * at the specified zoom level.
+   * @param {number} zoom value.
+   */
+  handleZoomParam_: function(paramValue) {
+    var paramValueSplit = paramValue.split(',');
+    if ((paramValueSplit.length != 1) && (paramValueSplit.length != 3))
+      return;
+
+    // User scale of 100 means zoom value of 100% i.e. zoom factor of 1.0.
+    var zoomFactor = parseFloat(paramValueSplit[0]) / 100;
+
+    // Handle #zoom=scale.
+    if (paramValueSplit.length == 1) {
+      this.viewport_.setZoom(zoomFactor);
+      return;
+    }
+
+    // Handle #zoom=scale,left,top.
+    var position = {x: parseFloat(paramValueSplit[1]),
+                    y: parseFloat(paramValueSplit[2])};
+    this.viewport_.position = position;
+    this.viewport_.setZoom(zoomFactor);
+  },
+
+  /**
    * @private
    * Handle open PDF parameters. These parameters are mentioned in the URL
    * and specify actions to be performed when opening PDF files.
-   * See http://crbug.com/64309 for details.
+   * See http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/pdf_open_parameters.pdf

[raymes, 2014/08/22 03:52:21]

you'll have to split up the URL so it doesn't extend past 80chars :(

+   * for details.
    */
   handleOpenPDFParams_: function() {
     var originalUrl = this.streamDetails.originalUrl;
@@ -292,6 +320,8 @@ PDFViewer.prototype = {
       if (!isNaN(pageNumber))
         this.viewport_.goToPage(pageNumber - 1);
     }
+    if ('zoom' in paramsDictionary)
+      this.handleZoomParam_(paramsDictionary['zoom']);
   },

[raymes, 2014/08/22 03:52:21]

This class is already quite large and I'd like to start pulling stuff out of it
for maintainability. Would you be able to pull out these two functions into a
separate class/file called OpenPDFParamsParser (or something to that effect).
Then you could have some code in the PDFViewer constructor which looks like
this:

var params_parser = new OpenPDFParamsParser(url);
viewport_.initialViewport = params_parser.viewport;  // this is just dictionary
with a few fields

And then in Viewport.setDocumentDimensions:

if (this.initialViewport.position)
  // scroll to position
else if (this.initialViewport.page)
  // scroll to page
else 
  // scroll to (0, 0)

if (this.initialViewport.zoom)
  // zoom to zoom
else
  ...

Does this make sense?

 
   /**
@@ -313,6 +343,8 @@ PDFViewer.prototype = {
       }
     } else if (progress == 100) {
       // Document load complete.
+      if (this.lastViewportPosition_)
+        this.viewport_.position = this.lastViewportPosition_;
       this.handleOpenPDFParams_();
       this.loaded = true;
       var loadEvent = new Event('pdfload');
@@ -320,8 +352,6 @@ PDFViewer.prototype = {
       this.sendScriptingMessage_({
         type: 'documentLoaded'
       });
-      if (this.lastViewportPosition_)
-        this.viewport_.position = this.lastViewportPosition_;
     }
   },