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 9805660b562c34e82cae189e4670abaace68eb8e..7c6a3c23bc3e55311b116884ff11de2ff2fb8844 100644
--- a/chrome/browser/resources/pdf/pdf.js
+++ b/chrome/browser/resources/pdf/pdf.js
@@ -191,6 +191,46 @@ function PDFViewer(browserApi) {
   this.zoomToolbar_.addEventListener('zoom-out',
       this.viewport_.zoomOut.bind(this.viewport_));
 
+  // We add Hammer.js in order to handle touch events like pinch-zoom.
+  this.hammertime_ = new Hammer($('plugin'));
+
+  // We must preventDefault if there is a two finger touch. By doing so browser
+  // zoom does not interfere with our way of handling the event.
+  this.plugin_.addEventListener('touchstart', function(e) {
+    if (e.touches.length >= 2) {
+      e.preventDefault();
+      this.hammertime_.get('pinch').set({enable: true});
+    }
+  }.bind(this));
+
+  this.plugin_.addEventListener('touchend', function(e) {
+    this.hammertime_.get('pinch').set({enable: false});
+  }.bind(this));
+
+  this.hammertime_.on('pinchstart', function(ev) {
+    this.pinchZoomStart(ev);
+  }.bind(this.viewport_));
+
+  this.hammertime_.on('pinch', function(ev) {
+    if(!this.didPinch_) {

[Kevin McNee - google account, 2016/10/06 21:53:17]

Style: missing space

[Kevin McNee - google account, 2016/10/13 18:19:23]

Done.

+      this.didPinch_ = true;

[Kevin McNee - google account, 2016/10/07 19:08:49]

There are several places where we access viewport's private properties.

didPinch_, didPinchEnd_, pinchPanVector_, pinchCenter_, doRender_ are all
private to viewport.

[Kevin McNee - google account, 2016/10/13 18:19:23]

Done.

+      window.requestAnimationFrame(function() {
+        this.didPinch_ = false;
+        this.pinchZoom(ev);
+      }.bind(this));
+    }
+  }.bind(this.viewport_));

[Kevin McNee - google account, 2016/10/07 19:08:49]

I'm not really a fan of these kind of binds. It makes it harder to figure out
what 'this' refers to in the function. Indeed, nowhere else in this directory do
these kind of binds occur, so I'll be changing this.

[Kevin McNee - google account, 2016/10/13 18:19:23]

Done.

+
+  this.hammertime_.on('pinchend', function(ev) {
+    if(!this.didPinchEnd_) {

[Kevin McNee - google account, 2016/10/06 21:53:17]

Style: missing space

[Kevin McNee - google account, 2016/10/13 18:19:23]

Done.

+      this.didPinchEnd_ = true;
+      window.requestAnimationFrame(function() {

[Kevin McNee - google account, 2016/10/06 21:53:17]

Comment from previous review:
bokan: I don't think we need to rAF the PinchEnd event.

[Kevin McNee - google account, 2016/10/07 19:08:49]

The method that this calls, pinchZoomEnd, does a final render, so wouldn't we
want to keep this?

[bokan, 2016/10/07 19:16:03]

It's been a while since I've looked at this. I think the reason we have rAFs
here at all is to throttle the pinch events to 1 per frame. We won't get
multiple PinchEnd events in a single frame so it'd be fine to do it immediately.
In any case, I think it's a style issue, fine either way.

[Kevin McNee - google account, 2016/10/07 19:38:11]

Oh, I see. I'll double check, but assuming the event behaves nicely, we could
simplify this to something like:
this.hammertime_.on('pinchend',
this.viewport_.pinchZoomEnd.bind(this.viewport_));

[Kevin McNee - google account, 2016/10/13 18:04:02]

It looks like this was preventing a race where a pinch update that was scheduled
by rAF gets sent after the pinch end event. It looks like the rAF specification
preserves order, so it's probably fine to leave the rAF as is, rather than
explicitly managing order. We can get rid of the didPinchEnd though, since it
only happens once.

+        this.didPinchEnd_ = false;
+        this.pinchZoomEnd(ev);
+      }.bind(this));
+    }
+  }.bind(this.viewport_));
+
   if (toolbarEnabled) {
     this.toolbar_ = $('toolbar');
     this.toolbar_.hidden = false;
@@ -669,11 +709,24 @@ 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 = this.viewport.doRender_;

[Kevin McNee - google account, 2016/10/07 19:08:49]

Consistent use of viewport / viewport_

[Kevin McNee - google account, 2016/10/13 18:19:23]

Done.

+
+    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

[Kevin McNee - google account, 2016/10/06 21:53:17]

Comment from previous review:
bokan: I think a better name here would be needsReraster

[wjmaclean, 2016/10/07 12:30:21]

Agreed, needsReraster is better.

Nit: update comment as well.

[Kevin McNee - google account, 2016/10/13 18:19:23]

Done.

+      px: pinchCenter.x,
+      py: pinchCenter.y,
+      pinchVectorX: pinchVector.x,
+      pinchVectorY: pinchVector.y
     });
     this.zoomManager_.onPdfZoomChange();
   },