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

Previously, the PDF extension called chrome.tabs.setZoom in response to
chrome.tabs.onZoomChange events received while a setZoom call was not
in progress. This setZoom call unnecessarily requested the zoom be set
to the current zoom value. Prior to another fix, this sometimes caused
the zoom indicator bubble to flicker.

This CL refactors the logic for when to call chrome.tabs.setZoom into a
separate, tested class that doesn't call setZoom in response to
onZoomChange events.

Review URL: https://codereview.chromium.org/1026223002

Cr-Commit-Position: refs/heads/master@{#324570}

7 files changed, 248 insertions, 41 deletions

diff --git a/chrome/browser/pdf/pdf_extension_test.cc b/chrome/browser/pdf/pdf_extension_test.cc
index d89c474..792ecfb 100644
--- a/chrome/browser/pdf/pdf_extension_test.cc
+++ b/chrome/browser/pdf/pdf_extension_test.cc
@@ -107,3 +107,7 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionTest, Navigator) {
 IN_PROC_BROWSER_TEST_F(PDFExtensionTest, ParamsParser) {
   RunTestsInFile("params_parser_test.js", "test.pdf");
 }
+
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, ZoomManager) {
+  RunTestsInFile("zoom_manager_test.js", "test.pdf");
+}
diff --git a/chrome/browser/resources/component_extension_resources.grd b/chrome/browser/resources/component_extension_resources.grd
index 4482968..2cd8e6e 100644
--- a/chrome/browser/resources/component_extension_resources.grd
+++ b/chrome/browser/resources/component_extension_resources.grd
@@ -173,6 +173,7 @@
         <include name="IDR_PDF_NAVIGATOR_JS" file="pdf/navigator.js" type="BINDATA" />
         <include name="IDR_PDF_VIEWPORT_SCROLLER_JS" file="pdf/viewport_scroller.js" type="BINDATA" />
         <include name="IDR_PDF_PDF_SCRIPTING_API_JS" file="pdf/pdf_scripting_api.js" type="BINDATA" />
+        <include name="IDR_PDF_ZOOM_MANAGER_JS" file="pdf/zoom_manager.js" type="BINDATA" />
         <include name="IDR_PDF_CONTENT_SCRIPT_JS" file="pdf/content_script.js" type="BINDATA" />
         <if expr="is_macosx">
           <include name="IDR_PDF_SCROLLBARS_CSS" file="pdf/scrollbars_mac.css" type="BINDATA" />
diff --git a/chrome/browser/resources/pdf/index-material.html b/chrome/browser/resources/pdf/index-material.html
index 044e527..ec0e9d0 100644
--- a/chrome/browser/resources/pdf/index-material.html
+++ b/chrome/browser/resources/pdf/index-material.html
@@ -35,6 +35,7 @@
 <script src="open_pdf_params_parser.js"></script>
 <script src="navigator.js"></script>
 <script src="viewport_scroller.js"></script>
+<script src="zoom_manager.js"></script>
 <script src="pdf_scripting_api.js"></script>
 <script src="chrome://resources/js/util.js"></script>
 <script src="pdf.js"></script>
diff --git a/chrome/browser/resources/pdf/index.html b/chrome/browser/resources/pdf/index.html
index 40ed9a9..7062cec 100644
--- a/chrome/browser/resources/pdf/index.html
+++ b/chrome/browser/resources/pdf/index.html
@@ -52,6 +52,7 @@
 <script src="open_pdf_params_parser.js"></script>
 <script src="navigator.js"></script>
 <script src="viewport_scroller.js"></script>
+<script src="zoom_manager.js"></script>
 <script src="pdf_scripting_api.js"></script>
 <script src="chrome://resources/js/util.js"></script>
 <script src="pdf.js"></script>
diff --git a/chrome/browser/resources/pdf/pdf.js b/chrome/browser/resources/pdf/pdf.js
index 4fae231..47baecd 100644
--- a/chrome/browser/resources/pdf/pdf.js
+++ b/chrome/browser/resources/pdf/pdf.js
@@ -176,31 +176,23 @@ function PDFViewer(streamDetails) {
                                     [this.bookmarksPane_]);
   }
 
-  // Setup the keyboard event listener.
-  document.onkeydown = this.handleKeyEvent_.bind(this);
-
   // Set up the zoom API.
   if (this.shouldManageZoom_()) {
     chrome.tabs.setZoomSettings(this.streamDetails_.tabId,
-                                {mode: 'manual', scope: 'per-tab'},
-                                this.afterZoom_.bind(this));
-    chrome.tabs.onZoomChange.addListener(function(zoomChangeInfo) {
-      if (zoomChangeInfo.tabId != this.streamDetails_.tabId)
-        return;
-      // 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.
-      var MIN_ZOOM_DELTA = 0.01;
-      var zoomDelta = Math.abs(this.viewport_.zoom -
-                               zoomChangeInfo.newZoomFactor);
-      // 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.
-      if ((zoomDelta > MIN_ZOOM_DELTA) && !this.setZoomInProgress_)
-        this.viewport_.setZoom(zoomChangeInfo.newZoomFactor);
+                                {mode: 'manual', scope: 'per-tab'}, function() {
+      this.zoomManager_ =
+          new ZoomManager(this.viewport_, this.setZoom_.bind(this));
+      chrome.tabs.onZoomChange.addListener(function(zoomChangeInfo) {
+        if (zoomChangeInfo.tabId != this.streamDetails_.tabId)
+          return;
+        this.zoomManager_.onBrowserZoomChange(zoomChangeInfo.newZoomFactor);
+      }.bind(this));
     }.bind(this));
   }
 
+  // Setup the keyboard event listener.
+  document.onkeydown = this.handleKeyEvent_.bind(this);
+
   // Parse open pdf parameters.
   this.paramsParser_ =
       new OpenPDFParamsParser(this.getNamedDestination_.bind(this));
@@ -481,7 +473,6 @@ PDFViewer.prototype = {
           this.pageIndicator_.initialFadeIn();
           this.toolbar_.initialFadeIn();
         }
-
         break;
       case 'email':
         var href = 'mailto:' + message.data.to + '?cc=' + message.data.cc +
@@ -574,35 +565,20 @@ PDFViewer.prototype = {
     var zoom = this.viewport_.zoom;
     if (this.isMaterial_)
       this.zoomSelector_.zoomValue = 100 * zoom;
-    if (this.shouldManageZoom_() && !this.setZoomInProgress_) {
-      this.setZoomInProgress_ = true;
-      chrome.tabs.setZoom(this.streamDetails_.tabId, zoom,
-                          this.setZoomComplete_.bind(this, zoom));
-    }
     this.plugin_.postMessage({
       type: 'viewport',
       zoom: zoom,
       xOffset: position.x,
       yOffset: position.y
     });
+    if (this.zoomManager_)
+      this.zoomManager_.onPdfZoomChange();
   },
 
-  /**
-   * @private
-   * A callback that's called after chrome.tabs.setZoom is complete. This will
-   * call chrome.tabs.setZoom again if the zoom level has changed since it was
-   * last called.
-   * @param {number} lastZoom the zoom level that chrome.tabs.setZoom was called
-   *     with.
-   */
-  setZoomComplete_: function(lastZoom) {
-    var zoom = this.viewport_.zoom;
-    if (zoom !== lastZoom) {
-      chrome.tabs.setZoom(this.streamDetails_.tabId, zoom,
-                          this.setZoomComplete_.bind(this, zoom));
-    } else {
-      this.setZoomInProgress_ = false;
-    }
+  setZoom_: function(zoom) {
+    return new Promise(function(resolve, reject) {
+      chrome.tabs.setZoom(this.streamDetails_.tabId, zoom, resolve);
+    }.bind(this));
   },
 
   /**
diff --git a/chrome/browser/resources/pdf/zoom_manager.js b/chrome/browser/resources/pdf/zoom_manager.js
new file mode 100644
index 0000000..664c88f
--- /dev/null
+++ b/chrome/browser/resources/pdf/zoom_manager.js
@@ -0,0 +1,83 @@
+// 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 {
+  /**
+   * Constructs a ZoomManager
+   * @param {!Viewport} viewport A Viewport for which to manage zoom.
+   * @param {Function} setBrowserZoomFunction A function that sets the browser
+   *     zoom to the provided value.
+   */
+  constructor(viewport, setBrowserZoomFunction) {
+    this.viewport_ = viewport;
+    this.setBrowserZoomFunction_ = setBrowserZoomFunction;
+    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) {
+    // If we are changing the browser zoom level, ignore any browser zoom level
+    // change events. Either, the change occurred before our update and will be
+    // overwritten, or the change being reported is the change we are making,
+    // which we have already handled.
+    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() {
+    // If we are already changing the browser zoom level in response to a
+    // previous extension-initiated zoom-level change, ignore this zoom change.
+    // Once the browser zoom level is changed, we check whether the extension's
+    // zoom level matches the most recently sent zoom level.
+    if (this.changingBrowserZoom_)
+      return;
+
+    let zoom = this.viewport_.zoom;
+    if (this.floatingPointEquals(this.browserZoom_, zoom))
+      return;
+
+    this.changingBrowserZoom_ = this.setBrowserZoomFunction_(zoom).then(
+        function() {
+      this.browserZoom_ = zoom;
+      this.changingBrowserZoom_ = null;
+
+      // 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.
+      this.onPdfZoomChange();
+    }.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;
+  }
+};
diff --git a/chrome/test/data/pdf/zoom_manager_test.js b/chrome/test/data/pdf/zoom_manager_test.js
new file mode 100644
index 0000000..6ce0a3b
--- /dev/null
+++ b/chrome/test/data/pdf/zoom_manager_test.js
@@ -0,0 +1,141 @@
+// 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.
+
+chrome.test.runTests(function() {
+  'use strict';
+
+  class MockViewport {
+    constructor() {
+      this.zooms = [];
+      this.zoom = 1;
+    }
+
+    setZoom(zoom) {
+      this.zooms.push(zoom);
+      this.zoom = zoom;
+    }
+  }
+
+  /**
+   * A mock implementation of the function used by ZoomManager to set the
+   * browser zoom level.
+   */
+  class MockBrowserZoomSetter {
+    constructor() {
+      this.zoom = 1;
+      this.started = false;
+    }
+
+    /**
+     * The function implementing setBrowserZoomFunction.
+     */
+    setBrowserZoom(zoom) {
+      chrome.test.assertFalse(this.started);
+
+      this.zoom = zoom;
+      this.started = true;
+      return new Promise(function(resolve, reject) {
+        this.resolve_ = resolve;
+      }.bind(this));
+    }
+
+    /**
+     * Resolves the promise returned by a call to setBrowserZoom.
+     */
+    complete() {
+      this.resolve_();
+      this.started = false;
+    }
+  };
+
+  return [
+    function testZoomChange() {
+      let viewport = new MockViewport();
+      let browserZoomSetter = new MockBrowserZoomSetter();
+      let zoomManager = new ZoomManager(
+          viewport, browserZoomSetter.setBrowserZoom.bind(browserZoomSetter));
+      viewport.zoom = 2;
+      zoomManager.onPdfZoomChange();
+      chrome.test.assertEq(2, browserZoomSetter.zoom);
+      chrome.test.assertTrue(browserZoomSetter.started);
+      chrome.test.succeed();
+    },
+
+    function testZoomChangedBeforeConstruction() {
+      let viewport = new MockViewport();
+      viewport.zoom = 2;
+      let browserZoomSetter = new MockBrowserZoomSetter();
+      let zoomManager = new ZoomManager(
+          viewport, browserZoomSetter.setBrowserZoom.bind(browserZoomSetter));
+      chrome.test.assertEq(2, browserZoomSetter.zoom);
+      chrome.test.assertTrue(browserZoomSetter.started);
+      chrome.test.succeed();
+    },
+
+    function testBrowserZoomChange() {
+      let viewport = new MockViewport();
+      let zoomManager = new ZoomManager(viewport, Promise.resolve.bind(Promise),
+                                        chrome.test.fail);
+      zoomManager.onBrowserZoomChange(3);
+      chrome.test.assertEq(1, viewport.zooms.length);
+      chrome.test.assertEq(3, viewport.zooms[0]);
+      chrome.test.assertEq(3, viewport.zoom);
+      chrome.test.succeed();
+    },
+
+    function testSmallZoomChange() {
+      let viewport = new MockViewport();
+      let browserZoomSetter = new MockBrowserZoomSetter();
+      let zoomManager = new ZoomManager(
+          viewport, browserZoomSetter.setBrowserZoom.bind(browserZoomSetter));
+      viewport.zoom = 1.0001;
+      zoomManager.onPdfZoomChange();
+      chrome.test.assertEq(1, browserZoomSetter.zoom);
+      chrome.test.assertFalse(browserZoomSetter.started);
+      chrome.test.succeed();
+    },
+
+    function testSmallBrowserZoomChange() {
+      let viewport = new MockViewport();
+      let zoomManager = new ZoomManager(viewport, Promise.resolve.bind(Promise),
+                                        chrome.test.fail);
+      zoomManager.onBrowserZoomChange(0.999);
+      chrome.test.assertEq(0, viewport.zooms.length);
+      chrome.test.assertEq(1, viewport.zoom);
+      chrome.test.succeed();
+    },
+
+    function testMultiplePdfZoomChanges() {
+      let viewport = new MockViewport();
+      let browserZoomSetter = new MockBrowserZoomSetter();
+      let zoomManager = new ZoomManager(
+          viewport, browserZoomSetter.setBrowserZoom.bind(browserZoomSetter));
+      viewport.zoom = 2;
+      zoomManager.onPdfZoomChange();
+      viewport.zoom = 3;
+      zoomManager.onPdfZoomChange();
+      chrome.test.assertTrue(browserZoomSetter.started);
+      chrome.test.assertEq(2, browserZoomSetter.zoom);
+      browserZoomSetter.complete();
+      Promise.resolve().then(function() {
+        chrome.test.assertTrue(browserZoomSetter.started);
+        chrome.test.assertEq(3, browserZoomSetter.zoom);
+        chrome.test.succeed();
+      });
+    },
+
+    function testMultipleBrowserZoomChanges() {
+      let viewport = new MockViewport();
+      let zoomManager = new ZoomManager(viewport, Promise.resolve.bind(Promise),
+                                        chrome.test.fail);
+      zoomManager.onBrowserZoomChange(2);
+      zoomManager.onBrowserZoomChange(3);
+      chrome.test.assertEq(2, viewport.zooms.length);
+      chrome.test.assertEq(2, viewport.zooms[0]);
+      chrome.test.assertEq(3, viewport.zooms[1]);
+      chrome.test.assertEq(3, viewport.zoom);
+      chrome.test.succeed();
+    },
+  ];
+}());