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

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 158 matching lines @@
     }.bind(this));
 
     document.body.addEventListener('change-page', function(e) {
       this.viewport_.goToPage(e.detail.page);
     }.bind(this));
 
     this.uiManager_ = new UiManager(window, this.materialToolbar_,
                                     [this.bookmarksPane_]);
   }
 
+  // Set up the zoom API.
+  if (this.shouldManageZoom_()) {
+    chrome.tabs.setZoomSettings(this.streamDetails_.tabId,
+                                {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;

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

Do you think we should pass in the zoom id? Then we could bind a function of
zoomManager directly?

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

I'd prefer to avoid ZoomManager needing to know about tab IDs.

+        this.zoomManager_.onBrowserZoomChange(zoomChangeInfo.newZoomFactor);
+      }.bind(this));
+    }.bind(this));
+  }
+
   // 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);
-    }.bind(this));
-  }
-
   // Parse open pdf parameters.
   this.paramsParser_ =
       new OpenPDFParamsParser(this.getNamedDestination_.bind(this));
   this.navigator_ = new Navigator(this.streamDetails_.originalUrl,
                                   this.viewport_, this.paramsParser_,
                                   onNavigateInCurrentTab, onNavigateInNewTab);
   this.viewportScroller_ =
       new ViewportScroller(this.viewport_, this.plugin_, window);
 }
 
@@ skipping 267 matching lines @@
         if (this.passwordScreen_.active)
           this.passwordScreen_.accept();
 
         if (this.isMaterial_) {
           this.materialToolbar_.docLength =
               this.documentDimensions_.pageDimensions.length;
         } else {
           this.pageIndicator_.initialFadeIn();
           this.toolbar_.initialFadeIn();
         }
-
         break;
       case 'email':
         var href = 'mailto:' + message.data.to + '?cc=' + message.data.cc +
             '&bcc=' + message.data.bcc + '&subject=' + message.data.subject +
             '&body=' + message.data.body;
         window.location.href = href;
         break;
       case 'getAccessibilityJSONReply':
         this.sendScriptingMessage_(message.data);
         break;
@@ skipping 72 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;
     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();
+  },
+
+  setZoom_: function(zoom) {
+    return new Promise(function(resolve, reject) {
+      chrome.tabs.setZoom(this.streamDetails_.tabId, zoom, resolve);

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

Why don't you pass in the function that sets the zoom into the zoom manager and
create the promise inside? It might be a bit clearer.

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

I'd prefer ZoomManager to use a nice interface instead of dealing with
callbacks.

+    }.bind(this));
   },
 
   /**
-   * @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;
-    }
-  },
-
-  /**
    * @private
    * A callback that's called after the viewport changes.
    */
   viewportChanged_: function() {
     if (!this.documentDimensions_)
       return;
 
     // Update the buttons selected.
     if (!this.isMaterial_) {
       $('fit-to-page-button').classList.remove('polymer-selected');
@@ skipping 176 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_;
   }
 };