Improved Pinch-Zoom for PDF

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 dab584d9d0bdc9afb6d3a68f1631627bba6bd8ac..fd6e648f5cec611b01ef87ce9c2d2c2e9094c209 100644
--- a/chrome/browser/resources/pdf/pdf.js
+++ b/chrome/browser/resources/pdf/pdf.js
@@ -217,6 +217,43 @@ function PDFViewer(browserApi) {
   this.zoomToolbar_.addEventListener('zoom-out',
       this.viewport_.zoomOut.bind(this.viewport_));
 
+  // Setup hammer.js touch event listener.

[bokan, 2016/04/21 02:24:00]

Comments should describe "why", not "what". In this case, "why are we adding
hammer.js?"

[alessandroa, 2016/04/21 20:39:39]

Done.

+  this.hammertime_ = new Hammer($('plugin'));
+
+  // We must preventDefault if there are is a two fingers touch.

[bokan, 2016/04/21 02:24:00]

"are is" -> "is"
"fingers" -> "finger"

Same as above, "why must we preventDefault if there's a two finger touch?"

[alessandroa, 2016/04/21 20:39:39]

Acknowledged.

+  this.plugin_.addEventListener('touchstart', function(e) {
+    if (e.touches.length >= 2) {
+      e.preventDefault();
+      this.hammertime_.get('pinch').set({enable: true});
+    } else

[bokan, 2016/04/21 02:24:00]

Else block should also have curly braces {} (if one branch has curly braces,
both should have)

[alessandroa, 2016/04/21 20:39:39]

Acknowledged.

+      this.hammertime_.get('pinch').set({enable: false});
+  }.bind(this));
+
+  this.plugin_.addEventListener('touchend', function(e) {
+    if (e.touches.length >= 2)
+      this.hammertime_.get('pinch').set({enable: true});

[bokan, 2016/04/21 02:24:00]

I think you only need the "enable: false" branch in touchend and "enable: start"
in touchstart.

[alessandroa, 2016/04/21 20:39:39]

Done.

+    else
+      this.hammertime_.get('pinch').set({enable: false});
+  }.bind(this));
+
+  this.hammertime_.on('pinchstart', function(ev) {
+    window.requestAnimationFrame(function() {
+      this.pinchZoomStart(ev);
+    }.bind(this));
+  }.bind(this.viewport_));
+
+  this.hammertime_.on('pinch', function(ev) {
+    window.requestAnimationFrame(function() {
+      this.pinchZoom(ev);

[bokan, 2016/04/21 02:24:00]

The way these are set up, if "pinch" is called 5 times in one frame, we'll call
this function 5 times when requestAnimationFrames are fired. What you really
want is something like this:

var didPinch = false;

on('pinch', function(ev) {
  if (!didPinch) {
    didPinch = true;
    window.requestAnimationFrame(function() {
      didPinch = false;
      this.pinchZoom(ev);
    }).bind(this))
  }
}).bind(this.viewport_);

[alessandroa, 2016/04/21 20:39:39]

Acknowledged.

+    }.bind(this));
+  }.bind(this.viewport_));
+
+  this.hammertime_.on('pinchend', function(ev) {
+    window.requestAnimationFrame(function() {
+      this.pinchZoomEnd(ev);
+    }.bind(this));
+  }.bind(this.viewport_));
+
   if (toolbarEnabled) {
     this.toolbar_ = $('toolbar');
     this.toolbar_.hidden = false;
@@ -450,21 +487,13 @@ PDFViewer.prototype = {
    * @private
    * Notify the plugin to print.
    */
-  print_: function() {
-    this.plugin_.postMessage({
-      type: 'print'
-    });
-  },
+  print_: function() { this.plugin_.postMessage({type: 'print'}); },

[bokan, 2016/04/21 02:24:00]

Undo the whitespace change (makes going back in time with `git blame` more
tedious)

[alessandroa, 2016/04/21 20:39:39]

Done.

 
   /**
    * @private
    * Notify the plugin to save.
    */
-  save_: function() {
-    this.plugin_.postMessage({
-      type: 'save'
-    });
-  },
+  save_: function() { this.plugin_.postMessage({type: 'save'}); },
 
   /**
    * Fetches the page number corresponding to the given named destination from
@@ -687,11 +716,26 @@ PDFViewer.prototype = {
   afterZoom_: function() {
     var position = this.viewport_.position;
     var zoom = this.viewport_.zoom;
+    var pinchVector = this.viewport_.pinchPanVector_;
+    var pinchCenter = this.viewport_.pinchCenter_;
+    var doRender = true;
+    if (!this.viewport_.doRender_)

[bokan, 2016/04/21 15:22:06]

This can just be doRender = this.viewport_.doRender

[alessandroa, 2016/04/21 20:39:39]

Acknowledged.

+      doRender = false;

[wjmaclean, 2016/04/21 14:11:01]

I don't understand ... isn't this the equivalent of

var doRender = this.viewport_.doRender_;

?

[alessandroa, 2016/04/21 20:39:39]

I changed from 1 and 0 to true and false and I forgot about this. Sorry. 

+
+    if (!pinchVector)
+      pinchVector = {x: 0, y: 0};
+    if (!pinchCenter)
+      pinchCenter = {x: 0, y: 0};
     this.plugin_.postMessage({
       type: 'viewport',
       zoom: zoom,
       xOffset: position.x,
-      yOffset: position.y
+      yOffset: position.y,
+      render: doRender,  // Render or not
+      px: pinchCenter.x,
+      py: pinchCenter.y,
+      pinchVectorX: pinchVector.x,
+      pinchVectorY: pinchVector.y
     });
     this.zoomManager_.onPdfZoomChange();
   },
@@ -870,9 +914,7 @@ PDFViewer.prototype = {
   /**
    * @type {Viewport} the viewport of the PDF viewer.
    */
-  get viewport() {
-    return this.viewport_;
-  },
+  get viewport() { return this.viewport_; },

[bokan, 2016/04/21 02:24:00]

Ditto on whitespace here and below

[alessandroa, 2016/04/21 20:39:39]

Sorry .. ctrl + f screwed this up :) 

 
   /**
    * Each bookmark is an Object containing a:
@@ -881,7 +923,5 @@ PDFViewer.prototype = {
    * - array of children (themselves bookmarks)
    * @type {Array} the top-level bookmarks of the PDF.
    */
-  get bookmarks() {
-    return this.bookmarks_;
-  }
+  get bookmarks() { return this.bookmarks_; }
 };