OOP PDF: Do not call setZoom in response to an onZoomChange event.

chrome/browser/resources/pdf/zoom_manager.js

Index: chrome/browser/resources/pdf/zoom_manager.js
diff --git a/chrome/browser/resources/pdf/zoom_manager.js b/chrome/browser/resources/pdf/zoom_manager.js
new file mode 100644
index 0000000000000000000000000000000000000000..481e51b519bf26968ed9b1db372ed84183936133
--- /dev/null
+++ b/chrome/browser/resources/pdf/zoom_manager.js
@@ -0,0 +1,70 @@
+// Copyright 2015 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';
+
+/**
+ * A class that manages updating the browser with zoom changes.
+ */
+class ZoomManager {
+  constructor(viewport, changeBrowserZoom) {

[raymes, 2015/04/09 03:40:01]

nit: setBrowserZoomFunction. Can we have documentation for the parameters?

[Sam McNally, 2015/04/09 08:24:40]

Done.

+    this.viewport_ = viewport;
+    this.changeBrowserZoom_ = changeBrowserZoom;
+    this.browserZoom_ = 1;
+    this.changingBrowserZoom_ = null;
+    this.onPdfZoomChange();
+  }
+
+  /**
+   * Invoked when a browser-initiated zoom-level change occurs.
+   * @param {number} newZoom the zoom level to zoom to.
+   */
+  onBrowserZoomChange(newZoom) {
+    // We should not change zoom level when we are responsible for initiating
+    // the zoom. onZoomChange() is called before setZoomComplete() callback

[raymes, 2015/04/09 03:40:01]

Should this be "onBrowserZoomChange"?

[Sam McNally, 2015/04/09 08:24:40]

Rewrote this comment.

+    // when we initiate the zoom.
+    if (this.changingBrowserZoom_)
+      return;
+
+    if (this.floatingPointEquals(this.browserZoom_, newZoom))
+      return;
+
+    this.browserZoom_ = newZoom;
+    this.viewport_.setZoom(newZoom);
+  }
+
+  /**
+   * Invoked when an extension-initiated zoom-level change occurs.
+   */
+  onPdfZoomChange() {
+    // We should not change zoom level when we are responsible for initiating
+    // the zoom. onZoomChange() is called before setZoomComplete() callback
+    // when we initiate the zoom.

[raymes, 2015/04/09 03:40:01]

I'm not sure if this comment makes sense here? Won't this just stop the
extension from setting the zoom if there is a zoom change already in progress?
In that case we will always call back into onPdfZoomChange below to make sure
the zoom is up to date.

[Sam McNally, 2015/04/09 08:24:40]

Rewrote this comment too.

+    if (this.changingBrowserZoom_)
+      return;
+
+    let zoom = this.viewport_.zoom;
+    if (this.floatingPointEquals(this.browserZoom_, zoom))
+      return;
+
+    this.changingBrowserZoom_ = this.changeBrowserZoom_(zoom).then(function() {
+      this.browserZoom_ = zoom;
+      this.changingBrowserZoom_ = null;
+      this.onPdfZoomChange();

[raymes, 2015/04/09 03:40:01]

This looks funny to a reader of the code who doesn't know what's going on. I
think we can make a note:
// The extension's zoom level may have changed while the browser zoom change was
in progress. We call back into onPdfZoomChange to ensure the browser zoom is up
to date.

[Sam McNally, 2015/04/09 08:24:40]

Done.

+    }.bind(this));
+  }
+
+  /**
+   * Returns whether two numbers are approximately equal.
+   * @param {number} a The first number.
+   * @param {number} b The second number.
+   */
+  floatingPointEquals(a, b) {
+    let MIN_ZOOM_DELTA = 0.01;
+    // If the zoom level is close enough to the current zoom level, don't
+    // change it. This avoids us getting into an infinite loop of zoom changes
+    // due to floating point error.
+    return Math.abs(a - b) <= MIN_ZOOM_DELTA;
+  }
+};