Merge pull request #133 from iangilman/raf

Improved requestAnimationFrame polyfill; fixed timer leaks; added tests

changelog.txt

@@ -4,6 +4,7 @@ OPENSEADRAGON CHANGELOG
 0.9.129: (In Progress)
 
 * Fixed an error when using navPrevNextWrap on single images (#135)
+* Various fixes to our timer handling (#133)
 
 0.9.128:
 

src/openseadragon.js

@@ -1689,6 +1689,7 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){
             };
         } else {
             var aAnimQueue = [],
+                processing = [],
                 iRequestId = 0,
                 iIntervalId;
 
@@ -1699,7 +1700,18 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){
                 if ( !iIntervalId ) {
                     iIntervalId = setInterval( function() {
                         if ( aAnimQueue.length ) {
-                            aAnimQueue.shift( )[ 1 ](+new Date());
+                            var time = new Date().getTime();
+                            // Process all of the currently outstanding frame
+                            // requests, but none that get added during the
+                            // processing.
+                            // Swap the arrays so we don't have to create a new
+                            // array every frame.
+                            var temp = processing;
+                            processing = aAnimQueue;
+                            aAnimQueue = temp;
+                            while ( processing.length ) {
+                                processing.shift()[ 1 ]( time );
+                            }
                         } else {
                             // don't continue the interval, if unnecessary
                             clearInterval( iIntervalId );
@@ -1714,12 +1726,23 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){
             // create a mock cancelAnimationFrame function
             $.cancelAnimationFrame = function( requestId ) {
                 // find the request ID and remove it
-                for ( var i = 0, j = aAnimQueue.length; i < j; i += 1 ) {
+                var i, j;
+                for ( i = 0, j = aAnimQueue.length; i < j; i += 1 ) {
                     if ( aAnimQueue[ i ][ 0 ] === requestId ) {
                         aAnimQueue.splice( i, 1 );
                         return;
                     }
                 }
+
+                // If it's not in the queue, it may be in the set we're currently
+                // processing (if cancelAnimationFrame is called from within a
+                // requestAnimationFrame callback).
+                for ( i = 0, j = processing.length; i < j; i += 1 ) {
+                    if ( processing[ i ][ 0 ] === requestId ) {
+                        processing.splice( i, 1 );
+                        return;
+                    }
+                }
             };
         }
     })( window );

src/viewer.js

@@ -151,7 +151,6 @@ $.Viewer = function( options ) {
     THIS[ this.hash ] = {
         "fsBoundsDelta":     new $.Point( 1, 1 ),
         "prevContainerSize": null,
-        "updateRequestId":   null,
         "animating":         false,
         "forceRedraw":       false,
         "mouseInside":       false,
@@ -168,6 +167,8 @@ $.Viewer = function( options ) {
         "onfullscreenchange": null
     };
 
+    this._updateRequestId = null;
+
     //Inherit some behaviors and properties
     $.EventHandler.call( this );
     $.ControlDock.call( this, options );
@@ -461,12 +462,16 @@ $.extend( $.Viewer.prototype, $.EventHandler.prototype, $.ControlDock.prototype,
      * @return {OpenSeadragon.Viewer} Chainable.
      */
     close: function ( ) {
-        if ( THIS[ this.hash ].updateRequestId !== null ){
+        if ( this._updateRequestId !== null ) {
-            $.cancelAnimationFrame( THIS[ this.hash ].updateRequestId );
+            $.cancelAnimationFrame( this._updateRequestId );
-            THIS[ this.hash ].updateRequestId = null;
+            this._updateRequestId = null;
         }
 
-        if( this.drawer ){
+        if ( this.navigator ) {
+            this.navigator.close();
+        }
+
+        if ( this.drawer ) {
             this.drawer.clearOverlays();
         }
 
@@ -1147,22 +1152,25 @@ function openTileSource( viewer, source ) {
     });
 
     //Instantiate a navigator if configured
-    if ( _this.showNavigator  && ! _this.navigator && !_this.collectionMode ){
+    if ( _this.showNavigator  && !_this.collectionMode ){
-        _this.navigator = new $.Navigator({
+        // Note: By passing the fully parsed source, the navigator doesn't
-            id:          _this.navigatorId,
+        // have to load it again.
-            position:    _this.navigatorPosition,
+        if ( _this.navigator ) {
-            sizeRatio:   _this.navigatorSizeRatio,
+            _this.navigator.open( source );
-            height:      _this.navigatorHeight,
+        } else {
-            width:       _this.navigatorWidth,
+            _this.navigator = new $.Navigator({
-            // By passing the fully parsed source here, the navigator doesn't
+                id:          _this.navigatorId,
-            // have to load it again. Additionally, we don't have to call
+                position:    _this.navigatorPosition,
-            // navigator.open, as it's implicitly called in the ctor.
+                sizeRatio:   _this.navigatorSizeRatio,
-            tileSources: source,
+                height:      _this.navigatorHeight,
-            tileHost:    _this.tileHost,
+                width:       _this.navigatorWidth,
-            prefixUrl:   _this.prefixUrl,
+                tileSources: source,
-            overlays:    _this.overlays,
+                tileHost:    _this.tileHost,
-            viewer:      _this
+                prefixUrl:   _this.prefixUrl,
-        });
+                overlays:    _this.overlays,
+                viewer:      _this
+            });
+        }
     }
 
     //Instantiate a referencestrip if configured
@@ -1186,7 +1194,7 @@ function openTileSource( viewer, source ) {
 
     THIS[ _this.hash ].animating = false;
     THIS[ _this.hash ].forceRedraw = true;
-    THIS[ _this.hash ].updateRequestId = scheduleUpdate( _this, updateMulti );
+    _this._updateRequestId = scheduleUpdate( _this, updateMulti );
 
     //Assuming you had programatically created a bunch of overlays
     //and added them via configuration
@@ -1233,23 +1241,7 @@ function openTileSource( viewer, source ) {
 ///////////////////////////////////////////////////////////////////////////////
 // Schedulers provide the general engine for animation
 ///////////////////////////////////////////////////////////////////////////////
-function scheduleUpdate( viewer, updateFunc, prevUpdateTime ){
+function scheduleUpdate( viewer, updateFunc ){
-    var currentTime,
-        targetTime,
-        deltaTime;
-
-    if ( THIS[ viewer.hash ].animating ) {
-        return $.requestAnimationFrame( function(){
-            updateFunc( viewer );
-        } );
-    }
-
-    currentTime     = +new Date();
-    prevUpdateTime  = prevUpdateTime ? prevUpdateTime : currentTime;
-    // 60 frames per second is ideal
-    targetTime      = prevUpdateTime + 1000 / 60;
-    deltaTime       = Math.max( 1, targetTime - currentTime );
-
     return $.requestAnimationFrame( function(){
         updateFunc( viewer );
     } );
@@ -1455,19 +1447,17 @@ function onContainerEnter( tracker, position, buttonDownElement, buttonDownAny )
 ///////////////////////////////////////////////////////////////////////////////
 
 function updateMulti( viewer ) {
-
-    var beginTime;
-
     if ( !viewer.source ) {
-        THIS[ viewer.hash ].updateRequestId = null;
+        viewer._updateRequestId = null;
         return;
     }
 
-    beginTime = +new Date();
     updateOnce( viewer );
 
-    THIS[ viewer.hash ].updateRequestId = scheduleUpdate( viewer,
+    // Request the next frame, unless we've been closed during the updateOnce()
-        arguments.callee, beginTime );
+    if ( viewer.source ) {
+        viewer._updateRequestId = scheduleUpdate( viewer, updateMulti );
+    }
 }
 
 function updateOnce( viewer ) {

test/basic.js

@@ -13,8 +13,7 @@
                 id:            'example',
                 prefixUrl:     '/build/openseadragon/images/',
                 tileSources:   '/test/data/testpattern.dzi',
-                springStiffness: 100, // Faster animation = faster tests
+                springStiffness: 100 // Faster animation = faster tests
-                showNavigator:  true
             });
 
             ok(viewer, 'Viewer exists');
@@ -25,6 +24,8 @@
                 equal(eventSender, viewer, 'Sender of open event was viewer');
                 ok(eventData, 'Handler also received event data');
                 ok(viewer.viewport, 'Viewport exists');
+                ok(viewer.source, 'source exists');
+                ok(viewer._updateRequestId, 'timer is on');
                 start();
             };
 
@@ -123,9 +124,14 @@
     asyncTest('Close', function() {
         var closeHandler = function() {
             viewer.removeHandler('close', closeHandler);
+            ok(!viewer.source, 'no source');
             $('#example').empty();
             ok(true, 'Close event was sent');
-            start();
+            ok(!viewer._updateRequestId, 'timer is off');
+            setTimeout(function() {
+                ok(!viewer._updateRequestId, 'timer is still off');
+                start();
+            }, 100);
         };
 
         viewer.addHandler('close', closeHandler);

test/navigator.js

@@ -461,4 +461,59 @@ QUnit.config.autostart = false;
             });
     });
 
+    asyncTest('Viewer closing one image and opening another', function() {
+        var timeWatcher = Util.timeWatcher();
+
+        viewer = OpenSeadragon({
+            id:            'example',
+            prefixUrl:     '/build/openseadragon/images/',
+            tileSources:   '/test/data/testpattern.dzi',
+            springStiffness: 100, // Faster animation = faster tests
+            showNavigator:  true
+        });
+
+        var openHandler1 = function(eventSender, eventData) {
+            viewer.removeHandler('open', openHandler1);
+            ok(viewer.navigator, 'navigator exists');
+            viewer.navigator.addHandler('open', navOpenHandler1);
+        };
+
+        var navOpenHandler1 = function(eventSender, eventData) {
+            viewer.navigator.removeHandler('open', navOpenHandler1);
+            equal(viewer.navigator.source, viewer.source, 'viewer and navigator have the same source');
+            ok(viewer.navigator._updateRequestId, 'navigator timer is on');
+            viewer.addHandler('close', closeHandler1);
+            viewer.addHandler('open', openHandler2);
+            viewer.open('/test/data/tall.dzi');
+        };
+
+        var closeHandler1 = function() {
+            viewer.removeHandler('close', closeHandler1);
+            ok(true, 'calling open closes the old one');
+            equal(viewer.navigator.source, null, 'navigator source has been cleared');
+        };
+
+        var openHandler2 = function(eventSender, eventData) {
+            viewer.removeHandler('open', openHandler2);
+            viewer.navigator.addHandler('open', navOpenHandler2);
+        };
+
+        var navOpenHandler2 = function(eventSender, eventData) {
+            viewer.navigator.removeHandler('open', navOpenHandler2);
+            equal(viewer.navigator.source, viewer.source, 'viewer and navigator have the same source');
+            viewer.addHandler('close', closeHandler2);
+            viewer.close();
+        };
+
+        var closeHandler2 = function() {
+            viewer.removeHandler('close', closeHandler2);
+            ok(!viewer.navigator._updateRequestId, 'navigator timer is off');
+            setTimeout(function() {
+                ok(!viewer.navigator._updateRequestId, 'navigator timer is still off');
+                timeWatcher.done();
+            }, 100);
+        };
+
+        viewer.addHandler('open', openHandler1);
+    });
 })();