Improved Pinch-Zoom for PDF.

chrome/browser/resources/pdf/viewport.js

 // Copyright 2014 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.
 
 /**
  * Returns the height of the intersection of two rectangles.
  * @param {Object} rect1 the first rect
  * @param {Object} rect2 the second rect
  * @return {number} the height of the intersection of the rects
  */
@@ skipping 31 matching lines @@
   this.beforeZoomCallback_ = beforeZoomCallback;
   this.afterZoomCallback_ = afterZoomCallback;
   this.allowedToChangeZoom_ = false;
   this.zoom_ = 1;
   this.documentDimensions_ = null;
   this.pageDimensions_ = [];
   this.scrollbarWidth_ = scrollbarWidth;
   this.fittingType_ = Viewport.FittingType.NONE;
   this.defaultZoom_ = defaultZoom;
   this.topToolbarHeight_ = topToolbarHeight;
+  this.prevScale_ = 1;
+  this.pinchPhase_ = Viewport.PinchPhase.PINCH_NONE;
 
   window.addEventListener('scroll', this.updateViewport_.bind(this));
   window.addEventListener('resize', this.resize_.bind(this));
 }
 
 /**
  * Enumeration of page fitting types.
  * @enum {string}
  */
 Viewport.FittingType = {
   NONE: 'none',
   FIT_TO_PAGE: 'fit-to-page',
   FIT_TO_WIDTH: 'fit-to-width'
 };
 
 /**
+ * Enumeration of pinch states.
+ * This should match PinchPhase enum in pdf/out_of_process_instance.h
+ * @enum {number}
+ */
+Viewport.PinchPhase = {
+  PINCH_NONE: 0,

[dpapad, 2016/11/08 18:32:34]

Nit: You can probably drop PINCH_ prefix from all values, without any loss of
useful information.

[Kevin McNee - google account, 2016/11/08 23:08:21]

This is to be consistent with the C++ enum in pdf/out_of_process_instance.h
where we want prefixes for clarity.

+  PINCH_START: 1,
+  PINCH_UPDATE_ZOOM_OUT: 2,
+  PINCH_UPDATE_ZOOM_IN: 3,
+  PINCH_END: 4
+};
+
+/**
  * The increment to scroll a page by in pixels when up/down/left/right arrow
  * keys are pressed. Usually we just let the browser handle scrolling on the
  * window when these keys are pressed but in certain cases we need to simulate
  * these events.
  */
 Viewport.SCROLL_INCREMENT = 40;
 
 /**
  * Predefined zoom factors to be used when zooming in/out. These are in
  * ascending order. This should match the lists in
@@ skipping 142 matching lines @@
   },
 
   /**
    * @type {number} the zoom level of the viewport.
    */
   get zoom() {
     return this.zoom_;
   },
 
   /**
+   * @type {Viewport.PinchPhase} the phase of the current pinch gesture for
+   *    the viewport.
+   */
+  get pinchPhase() {
+    return this.pinchPhase_;
+  },
+
+  /**
+   * @type {Object} the panning caused by the current pinch gesture.
+   */
+  get pinchPanVector() {
+    return this.pinchPanVector_;
+  },
+
+  /**
+   * @type {Object} the center of the current pinch gesture.
+   */
+  get pinchCenter() {
+    return this.pinchCenter_;

[dpapad, 2016/11/08 18:32:35]

This member variable does not appear anywhere in the constructor. It is
basically non existent until it is augmented wihith pinchZoom. Can you declare
it in the constructor, document its type and perhaps give it a default value of
null?

[Kevin McNee - google account, 2016/11/08 23:08:20]

Done.

+  },
+
+  /**
    * @private
    * Used to wrap a function that might perform zooming on the viewport. This is
    * required so that we can notify the plugin that zooming is in progress
    * so that while zooming is taking place it can stop reacting to scroll events
    * from the viewport. This is to avoid flickering.
    */
   mightZoom_: function(f) {
     this.beforeZoomCallback_();
     this.allowedToChangeZoom_ = true;
     f();
@@ skipping 19 matching lines @@
     this.zoom_ = newZoom;
     this.contentSizeChanged_();
     // Scroll to the scaled scroll position.
     this.position = {
       x: currentScrollPos.x * newZoom,
       y: currentScrollPos.y * newZoom
     };
   },
 
   /**
+   * @private
+   * Sets the zoom of the viewport.
+   * Same as setZoomInternal_ but for pinch zoom we have some more operations.
+   * @param {number} scaleDelta the zoom delta.
+   * @param {!Object} center the pinch center in content coordinates
+   */
+  setPinchZoomInternal_: function(scaleDelta, center) {
+    if (!this.allowedToChangeZoom_) {
+      throw 'Called Viewport.setZoomInternal_ without calling ' +

[dpapad, 2016/11/08 18:32:35]

Could this use assert() from
https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=18?

If not, can you throw a proper Error instead?
throw new Error('Called ......');

[Kevin McNee - google account, 2016/11/08 23:08:21]

Done.

+          'Viewport.mightZoom_.';
+    }
+    this.zoom_ = this.clampScale(this.zoom_ * scaleDelta);
+
+    var newCenterInContent = this.frameToContent(center);
+    var delta = {
+      x: (newCenterInContent.x - this.oldCenterInContent.x),
+      y: (newCenterInContent.y - this.oldCenterInContent.y)
+    };
+
+    // Record the scroll position (relative to the pinch center).
+    var currentScrollPos = {
+      x: this.position.x - delta.x * this.zoom_,
+      y: this.position.y - delta.y * this.zoom_
+    };
+
+    this.contentSizeChanged_();
+    // Scroll to the scaled scroll position.
+    this.position = {
+      x: currentScrollPos.x,
+      y: currentScrollPos.y
+    };
+  },
+
+  /**
+   *  @private
+   *  Makes sure that the scale level doesn't get out of the limits.
+   *  @param {number} scale the new scale level

[dpapad, 2016/11/08 18:32:35]

Per styleguide: "Complete sentences are recommended but not required. Complete
sentences should use appropriate capitalization and punctuation."

s/the new scale level/The new scale level./

[Kevin McNee - google account, 2016/11/08 23:08:21]

Done.

+   *  @return {number} the scale clamped in the limit
+   */
+  clampScale: function(scale) {
+    return Math.min(5, Math.max(0.25, scale));
+  },
+
+  /**
+   *  @private
+   *  Converts a point from frame to content coordinates.
+   *  @param {Object} framePoint the frame coordinates
+   *  @return {Object} the content coordinates

[dpapad, 2016/11/08 18:32:35]

!Object

[Kevin McNee - google account, 2016/11/08 23:08:20]

Done.

+   */
+  frameToContent: function(framePoint) {
+    return {
+      x: (framePoint.x + this.position.x) / this.zoom_,
+      y: (framePoint.y + this.position.y) / this.zoom_
+    };
+  },
+
+  /**
    * Sets the zoom to the given zoom level.
    * @param {number} newZoom the zoom level to zoom to.
    */
   setZoom: function(newZoom) {
     this.fittingType_ = Viewport.FittingType.NONE;
     newZoom = Math.max(Viewport.ZOOM_FACTOR_RANGE.min,
                        Math.min(newZoom, Viewport.ZOOM_FACTOR_RANGE.max));
     this.mightZoom_(function() {
       this.setZoomInternal_(newZoom);
       this.updateViewport_();
@@ skipping 173 matching lines @@
         this.position = {
           x: 0,
           y: this.pageDimensions_[page].y * this.zoom_
         };
       }
       this.updateViewport_();
     }.bind(this));
   },
 
   /**
+   * @private
+   * Computes vector between two points.
+   * @param {!Object} First Point
+   * @param {!Object} Second Point
+   * @return {!Object} The vector
+   */
+  vectorDelta: function(p1, p2) {
+    return {
+      x: p2.x - p1.x,
+      y: p2.y - p1.y
+    };
+  },
+
+  /**
    * Zoom the viewport so that a page consumes the entire viewport. Also scrolls
    * the viewport to the top of the current page.
    */
   fitToPage: function() {
     this.fitToPageInternal_(true);
   },
 
   /**
    * Zoom out to the next predefined zoom level.
    */
@@ skipping 20 matching lines @@
       for (var i = Viewport.ZOOM_FACTORS.length - 1; i >= 0; i--) {
         if (Viewport.ZOOM_FACTORS[i] > this.zoom_)
           nextZoom = Viewport.ZOOM_FACTORS[i];
       }
       this.setZoomInternal_(nextZoom);
       this.updateViewport_();
     }.bind(this));
   },
 
   /**
+   * Pinch zoom event handler
+   * @param {!Object} e the pinch event.
+   */
+  pinchZoom: function(e) {
+    this.mightZoom_(function() {
+      this.pinchPhase_ = (e.direction == 'out') ?

[dpapad, 2016/11/08 18:32:35]

Nit: No need for parentheses.

[Kevin McNee - google account, 2016/11/08 23:08:21]

Done.

+                         Viewport.PinchPhase.PINCH_UPDATE_ZOOM_OUT :
+                         Viewport.PinchPhase.PINCH_UPDATE_ZOOM_IN;
+
+      var scaleDelta = e.startScaleRatio / this.prevScale_;
+      this.pinchPanVector_ =
+          this.vectorDelta(e.center, this.first_pinch_center_in_frame_);
+
+      var needsScrollbars = this.documentNeedsScrollbars_(
+          this.clampScale(this.zoom_ * scaleDelta));
+
+      this.pinchCenter_ = e.center;
+
+      // If there's no horizontal scrolling, keep the content centered so the
+      // user can't zoom in on the non-content area.
+      // TODO(mcnee) Investigate other ways of scaling when we don't have
+      // horizontal scrolling. We want to keep the document centered,
+      // but this causes a potentially awkward transition when we start
+      // using the gesture center.
+      if (!needsScrollbars.horizontal) {
+        this.pinchCenter_ = {
+          x: this.window_.innerWidth / 2,
+          y: this.window_.innerHeight / 2
+        };
+      } else if (this.keepContentCentered_) {
+        this.oldCenterInContent =
+            this.frameToContent(this.frameToPluginCoordinate(e.center));
+        this.keepContentCentered_ = false;
+      }
+
+      this.setPinchZoomInternal_(
+          scaleDelta, this.frameToPluginCoordinate(e.center));
+      this.updateViewport_();
+      this.prevScale_ = e.startScaleRatio;
+    }.bind(this));
+  },
+
+  pinchZoomStart: function(e) {
+    this.pinchPhase_ = Viewport.PinchPhase.PINCH_START;
+    this.prevScale_ = 1;
+    this.oldCenterInContent =
+        this.frameToContent(this.frameToPluginCoordinate(e.center));
+
+    var needsScrollbars = this.documentNeedsScrollbars_(this.zoom_);
+    this.keepContentCentered_ = !needsScrollbars.horizontal;
+    // We keep track of begining of the pinch.
+    // By doing so we will be able to compute the pan distance.
+    this.first_pinch_center_in_frame_ = e.center;

[dpapad, 2016/11/08 18:32:35]

s/first_pinch_center_in_frame_/firstPinchCenterInFrame_/

Also can you declare and initialize this in the constructor?

[Kevin McNee - google account, 2016/11/08 23:08:21]

Done.

+  },
+
+  pinchZoomEnd: function(e) {
+    this.mightZoom_(function() {
+      this.pinchPhase_ = Viewport.PinchPhase.PINCH_END;
+      var scaleDelta = e.startScaleRatio / this.prevScale_;
+      this.pinchCenter_ = e.center;
+
+      this.setPinchZoomInternal_(
+          scaleDelta, this.frameToPluginCoordinate(e.center));
+      this.updateViewport_();
+    }.bind(this));
+    this.pinchPhase_ = Viewport.PinchPhase.PINCH_NONE;
+  },
+
+  frameToPluginCoordinate: function(coordinateInFrame) {

[dpapad, 2016/11/08 18:32:35]

clampScale(), verctorDelta() and frameToPluginCoordinate() don't use |this|. Can
you make them functions instead of member methods, similar to
getIntersectionHeight at the top of this file?

[Kevin McNee - google account, 2016/11/08 23:08:20]

Done.

+    var container = $('plugin');
+    return {
+      x: coordinateInFrame.x - container.getBoundingClientRect().left,
+      y: coordinateInFrame.y - container.getBoundingClientRect().top
+    };
+  },
+
+  /**
    * Go to the given page index.
    * @param {number} page the index of the page to go to. zero-based.
    */
   goToPage: function(page) {
     this.mightZoom_(function() {
       if (this.pageDimensions_.length === 0)
         return;
       if (page < 0)
         page = 0;
       if (page >= this.pageDimensions_.length)
@@ skipping 78 matching lines @@
     spaceOnLeft = Math.max(spaceOnLeft, 0);
 
     return {
       x: x * this.zoom_ + spaceOnLeft - this.window_.pageXOffset,
       y: insetDimensions.y * this.zoom_ - this.window_.pageYOffset,
       width: insetDimensions.width * this.zoom_,
       height: insetDimensions.height * this.zoom_
     };
   }
 };