Improved Pinch-Zoom for PDF.

chrome/browser/resources/pdf/pdf.js

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 'use strict';
 
 /**
  * @return {number} Width of a scrollbar in pixels
  */
 function getScrollbarWidth() {
@@ skipping 173 matching lines @@
   this.zoomToolbar_ = $('zoom-toolbar');
   this.zoomToolbar_.addEventListener('fit-to-width',
       this.viewport_.fitToWidth.bind(this.viewport_));
   this.zoomToolbar_.addEventListener('fit-to-page',
       this.fitToPage_.bind(this));
   this.zoomToolbar_.addEventListener('zoom-in',
       this.viewport_.zoomIn.bind(this.viewport_));
   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;
     this.toolbar_.addEventListener('save', this.save_.bind(this));
     this.toolbar_.addEventListener('print', this.print_.bind(this));
     this.toolbar_.addEventListener('rotate-right',
         this.rotateClockwise_.bind(this));
     // Must attach to mouseup on the plugin element, since it eats mousedown
     // and click events.
     this.plugin_.addEventListener('mouseup',
@@ skipping 458 matching lines @@
   },
 
   /**
    * @private
    * A callback that's called after the zoom changes. Notify the plugin of the
    * zoom change and to continue reacting to scroll events.
    */
   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();
   },
 
   /**
    * @private
    * A callback that's called after the viewport changes.
    */
   viewportChanged_: function() {
     if (!this.documentDimensions_)
@@ skipping 172 matching lines @@
    * Each bookmark is an Object containing a:
    * - title
    * - page (optional)
    * - array of children (themselves bookmarks)
    * @type {Array} the top-level bookmarks of the PDF.
    */
   get bookmarks() {
     return this.bookmarks_;
   }
 };