Seen while running : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100813 Minefield/4.0b4pre. I see Bug 581038 on file, but I don't seem to see this same issue on Mac or using Win XP STR: 1. Create a group of 4 tabs. 2. Resize the window slightly 3. See the attached screenshot

It's quite ugly and easily repro'd on Windows OS's but not on Mac. requesting blocking.

I can reproduce this on OS X as well, although the interpolation effects are different (see attachment above). My repro for this: 1. Start with 4 thumbnails in a 4 wide by 1 high group. 2. Reduce the group size as much as possible, but don't go into the stacked mode. 3. Change the order of the thumbnails in the group. 4. Make larger, and you should see this effect. Without knowing the code, my hunch is that there's a dynamic thumbnail cache that removes unused thumbnails when they're added to a group, but doesn't recreate new ones as needed when the group is resized.

(In reply to comment #3) > I can reproduce this on OS X as well, although the interpolation effects are > different (see attachment above). My repro for this: > > 1. Start with 4 thumbnails in a 4 wide by 1 high group. > 2. Reduce the group size as much as possible, but don't go into the stacked > mode. > 3. Change the order of the thumbnails in the group. > 4. Make larger, and you should see this effect. > > Without knowing the code, my hunch is that there's a dynamic thumbnail cache > that removes unused thumbnails when they're added to a group, but doesn't > recreate new ones as needed when the group is resized. It may take a moment for the thumbnails to update, but they should get around to it... do they not?

This bug has two causes: 1. Thumbnails weren't updating when the group was resized 2. Win7 looked especially bad because of the implementation of Context.drawWindow(), which appears to be a point-sampled scaling. There's no way to change this through Chrome Javascript, as far as I can tell. This patch only fixes 1. Because constantly regenerating the thumbnail canvas is expensive, a heartbeat system was previously written to add the tab thumbnail onto an update queue that would then process a thumbnail every 100ms, but the heartbeat was never initiated. Upon turning this on, the expensive canvas scaling was blocking UI interactivity. The solution was to add an isIdle() method to the UI global, so that the heartbeat would only process thumbnails when the user wouldn't notice. The isIdle is based on allowing a minimum amount of time to pass between the last operation (mouse move) and the current time.

v2: Simplified heartbeat state, and now resilient to multiple startHeartbeat calls.

Comment on attachment 473945 [details] [diff] [review] v2 re-request review once you've gotten feedback+ from ian.

Comment on attachment 473945 [details] [diff] [review] v2 > var drag = { > info: null, >- zIndex: 100 >+ zIndex: 100, >+ lastMoveTime: 0 > }; > >+//---------- >+//Variable: resize >+//The resize (actually a Drag) that is currently in process >+var resize = { >+ info: null, >+ lastMoveTime: 0 >+}; Is there a point to having separate lastMoveTime's? >diff --git a/browser/base/content/tabview/tabitems.js b/browser/base/content/tabview/tabitems.js >--- a/browser/base/content/tabview/tabitems.js >+++ b/browser/base/content/tabview/tabitems.js >@@ -649,7 +649,7 @@ let TabItems = { > items: [], > paintingPaused: 0, > _tabsWaitingForUpdate: [], >- _heartbeatOn: false, >+ _heartbeatInFlight: false, What's the point of this rename, aside from sounding cooler? ;) >+ _checkHeartbeat: function TabItems__checkHeartbeat() { >+ if (!this.isPaintingPaused()) { return on if (this.isPaintingPaused()) instead. >@@ -889,12 +899,8 @@ let TabItems = { > // three times before TabItems will start updating thumbnails again. > resumePainting: function TabItems_resumePainting() { ... >+ if (!this.isPaintingPaused()) { >+ this._checkHeartbeat(); > } Nit: remove { } r? the next patch with those fixes. Please choose a reviewer who is on win7 and can easily check that the issue has improved, though.

(In reply to comment #8) > Is there a point to having separate lastMoveTime's? It lets us track the data we want, and then figure out how we're going to use it in isIdle(). I think this small bit of flexibility is a good practice. > >- _heartbeatOn: false, > >+ _heartbeatInFlight: false, > > What's the point of this rename, aside from sounding cooler? ;) heartbeatOn is ambiguous, and was used differently as an enable state. heartbeatInFlight means that there is currently a timeout chain in flight.

(In reply to comment #9) > > >- _heartbeatOn: false, > > >+ _heartbeatInFlight: false, > > > > What's the point of this rename, aside from sounding cooler? ;) > > heartbeatOn is ambiguous, and was used differently as an enable state. > heartbeatInFlight means that there is currently a timeout chain in flight. You should explain in the code what "in flight" means. It's not immediately clear to me what "a chain is in flight" means, let alone to other coders from around the world who may later read your code. ;)

(In reply to comment #5) > This bug has two causes: 1. Thumbnails weren't updating when the group was > resized 2. Win7 looked especially bad because of the implementation of > Context.drawWindow(), which appears to be a point-sampled scaling. There's no > way to change this through Chrome Javascript, as far as I can tell. This patch > only fixes 1. please file a bug for #2.

Comment on attachment 474285 [details] [diff] [review] v3 canceling review, please add a test and re-request.

Test resizes group and verifies image update.

Comment on attachment 477283 [details] [diff] [review] v4: (w/ test) Reassigning review to dolske while dietrich is jammed up with b7 stuff.

Comment on attachment 477283 [details] [diff] [review] v4: (w/ test) >+++ b/browser/base/content/tabview/tabitems.js ... >- _heartbeatOn: false, >+ _heartbeatInFlight: false, // see explanation at startHeartbeat() below I found "in flight" a bit confusing too. Not sure if the rename is really helping. >+ startHeartbeat: function TabItems_startHeartbeat() { >+ if (!this._heartbeatInFlight) { >+ this._heartbeatInFlight = true; >+ this._checkHeartbeat(); >+ } >+ }, You should set |_heartbeatInFlight = true| closer to the actual setTimeout() call. Like, 1 line above or below it. This helps eliminate any chance of getting into a state where you think a heartbeat is pending, but it really isn't. For example, should you ever call startHeartbeat() while painting is paused, checkHeartbeat() just immediately returns without starting the timer, and now your heartbeat state is broken. >+++ b/browser/base/content/tabview/ui.js ... >+ // Constant: _maxInteractiveWait >+ // If the UI is in the middle of an operation, this is the max amount of >+ // milliseconds to wait between input events before we no longer consider >+ // the operation interactive. >+ _maxInteractiveWait: 10, 10ms? That seems suspiciously low. I'd be mildly surprised if mouse-driven events even came that frequently except for rapid/large movements. Seems like this ought to err on the high side (250 or 500ms?), to help ensure thumbnail paints don't occur until it's quite clear the UI isn't being manipulated. Perhaps with an onmouseup exception, so that upon the final resize tabs start to immediately resample? (Well, at least _1_ might, since the others would still go at 100ms intervals. So not sure if that helps with snappyness perception or not.) >+++ b/browser/base/content/test/tabview/Makefile.in ... >+ browser_tabview_bug587231.js \ This file isn't in the patch. Forget to "hg add" it?

I changed the name back to heartbeatOn (thought of changing to heartbeatPending but I don't really care). I think your suggestion of placing _heartbeatOn close to the setTimeout() isn't quite right. In the scenario where startHeartbeat is called while painting is paused, the state is fine since _checkHeartbeat gets called when painting is resumed. If there's nothing left on the queue, then _heartbeatOn gets turned off. The point of putting _heartbeatOn in startHeartbeat is to keep _checkHeartbeat from being called multiple times, which would put multiple calls to setTimeout in flight and would defeat having a single queue that processes tabitem paints when the UI is idle. I've included the test, which was left out before.

To be more clear, a subtle point of this is that when painting is paused, the setTimeout() chain stops, and is restarted (if there is work pending) by the post-pause call to _checkHeartbeat().

Initial _checkHeartbeat() wasn't on timeout queue, which is a bug.

(In reply to comment #18) > To be more clear, a subtle point of this is that when painting is paused, the > setTimeout() chain stops, and is restarted (if there is work pending) by the > post-pause call to _checkHeartbeat(). Yeah, I get that, but the case I'm worried about is still broken: For example, should you ever call startHeartbeat() while painting is paused, checkHeartbeat() just immediately returns without starting the timer, and now your heartbeat state is broken. IE, heartbeatOn indicates if there's a pending setTimeout or not, so you need to avoid getting into a state where heartbeatOn == true but no timer was started (or heartbeatOn == false when there is one, but the code prevents this already). With the current patch, I think the easiest way to address this would be for checkHeartbeat() to immediately set _heartbeatOn to false. You can also then have the "this._tabsWaitingForUpdate.length" check just call startHeartbeat() instead of duplicating the setTimeout code.

Comment on attachment 483097 [details] [diff] [review] v6 (w/test) ># vim: se ft=diff : ># HG changeset patch ># User Sean Dunn <seanedunn@yahoo.com> ># Date 2010-09-09 22:29 >Bug 587231 - "[Win7] Thumbnails sometimes like an abstract painting in TabCandy" [] > >diff --git a/browser/base/content/tabview/drag.js b/browser/base/content/tabview/drag.js >--- a/browser/base/content/tabview/drag.js >+++ b/browser/base/content/tabview/drag.js >@@ -38,19 +38,27 @@ > // ********** > // Title: drag.js > > // ---------- > // Variable: drag > // The Drag that's currently in process. > var drag = { > info: null, >- zIndex: 100 >+ zIndex: 100, >+ lastMoveTime: 0 > }; > >+//---------- >+//Variable: resize >+//The resize (actually a Drag) that is currently in process >+var resize = { >+ info: null, >+ lastMoveTime: 0 >+}; > > // ########## > // Class: Drag (formerly DragInfo) > // Helper class for dragging <Item>s > // > // ---------- > // Constructor: Drag > // Called to create a Drag in response to an <Item> draggable "start" event. >diff --git a/browser/base/content/tabview/items.js b/browser/base/content/tabview/items.js >--- a/browser/base/content/tabview/items.js >+++ b/browser/base/content/tabview/items.js >@@ -211,35 +211,34 @@ Item.prototype = { > // Private to this file. > accept: function dropAcceptFunction(item) { > return (item && item.isATabItem && (!item.parent || !item.parent.expanded)); > } > }; > > // ___ resize > var self = this; >- var resizeInfo = null; > this.resizeOptions = { > aspectRatio: self.keepProportional, > minWidth: 90, > minHeight: 90, > start: function(e,ui) { > if (this.isAGroupItem) > GroupItems.setActiveGroupItem(this); >- resizeInfo = new Drag(this, e, true); // true = isResizing >+ resize.info = new Drag(this, e, true); // true = isResizing > }, > resize: function(e,ui) { > // TODO: maybe the stationaryCorner should be topright for rtl langs? >- resizeInfo.snap('topleft', false, self.keepProportional); >+ resize.info.snap('topleft', false, self.keepProportional); > }, > stop: function() { > self.setUserSize(); > self.pushAway(); >- resizeInfo.stop(); >- resizeInfo = null; >+ resize.info.stop(); >+ resize.info = null; > } > }; > }, > > // ---------- > // Function: getBounds > // Returns a copy of the Item's bounds as a <Rect>. > getBounds: function Item_getBounds() { >@@ -595,16 +594,19 @@ Item.prototype = { > var startPos; > var startSent; > var startEvent; > var droppables; > var dropTarget; > > // ___ mousemove > var handleMouseMove = function(e) { >+ // global drag tracking >+ drag.lastMoveTime = Date.now(); >+ > // positioning > var mouse = new Point(e.pageX, e.pageY); > if (!startSent) { > if(Math.abs(mouse.x - startMouse.x) > self.dragOptions.minDragDistance || > Math.abs(mouse.y - startMouse.y) > self.dragOptions.minDragDistance) { > if (typeof self.dragOptions.start == "function") > self.dragOptions.start.apply(self, > [startEvent, {position: {left: startPos.x, top: startPos.y}}]); >@@ -762,16 +764,19 @@ Item.prototype = { > $container.addClass('iq-resizable'); > > var self = this; > var startMouse; > var startSize; > > // ___ mousemove > var handleMouseMove = function(e) { >+ // global resize tracking >+ resize.lastMoveTime = Date.now(); >+ > var mouse = new Point(e.pageX, e.pageY); > var box = self.getBounds(); > box.width = Math.max(self.resizeOptions.minWidth || 0, startSize.x + (mouse.x - startMouse.x)); > box.height = Math.max(self.resizeOptions.minHeight || 0, startSize.y + (mouse.y - startMouse.y)); > > if (self.resizeOptions.aspectRatio) { > if (startAspect < 1) > box.height = box.width * startAspect; >diff --git a/browser/base/content/tabview/tabitems.js b/browser/base/content/tabview/tabitems.js >--- a/browser/base/content/tabview/tabitems.js >+++ b/browser/base/content/tabview/tabitems.js >@@ -91,16 +91,18 @@ function TabItem(tab, options) { > this.sizeExtra.x = parseInt($div.css('padding-left')) > + parseInt($div.css('padding-right')); > > this.sizeExtra.y = parseInt($div.css('padding-top')) > + parseInt($div.css('padding-bottom')); > > this.bounds = $div.bounds(); > >+ this._lastTabUpdateTime = Date.now(); >+ > // ___ superclass setup > this._init($div[0]); > > // ___ reconnect to data from Storage > this._hasBeenDrawn = false; > let reconnected = TabItems.reconnect(this); > > // ___ drag/drop >@@ -670,18 +672,18 @@ TabItem.prototype = Utils.extend(new Ite > let TabItems = { > minTabWidth: 40, > tabWidth: 160, > tabHeight: 120, > fontSize: 9, > items: [], > paintingPaused: 0, > _tabsWaitingForUpdate: [], >- _heartbeatOn: false, >- _heartbeatTiming: 100, // milliseconds between beats >+ _heartbeatOn: false, // see explanation at startHeartbeat() below >+ _heartbeatTiming: 100, // milliseconds between _checkHeartbeat() calls > _lastUpdateTime: Date.now(), > _eventListeners: [], > > // ---------- > // Function: init > // Set up the necessary tracking to maintain the <TabItems>s. > init: function TabItems_init() { > Utils.assert(window.AllTabs, "AllTabs must be initialized first"); >@@ -760,16 +762,17 @@ let TabItems = { > let isCurrentTab = ( > !UI._isTabViewVisible() && > tab == gBrowser.selectedTab > ); > > if (shouldDefer && !isCurrentTab) { > if (this._tabsWaitingForUpdate.indexOf(tab) == -1) > this._tabsWaitingForUpdate.push(tab); >+ this.startHeartbeat(); > } else > this._update(tab); > } catch(e) { > Utils.log(e); > } > }, > > // ---------- >@@ -820,27 +823,28 @@ let TabItems = { > let w = $canvas.width(); > let h = $canvas.height(); > if (w != tabItem.canvasEl.width || h != tabItem.canvasEl.height) { > tabItem.canvasEl.width = w; > tabItem.canvasEl.height = h; > } > } > >+ this._lastUpdateTime = Date.now(); >+ tabItem._lastTabUpdateTime = this._lastUpdateTime; >+ > tabItem.tabCanvas.paint(); > > // ___ cache > // TODO: this logic needs to be better; hiding too soon now > if (tabItem.isShowingCachedData && !tab.hasAttribute("busy")) > tabItem.hideCachedData(); > } catch(e) { > Utils.log(e); > } >- >- this._lastUpdateTime = Date.now(); > }, > > // ---------- > // Function: link > // Takes in a xul:tab, creates a TabItem for it and adds it to the scene. > link: function TabItems_link(tab, options) { > try { > Utils.assertThrow(tab, "tab"); >@@ -887,64 +891,76 @@ let TabItems = { > // ---------- > // when a tab becomes unpinned, create a TabItem for it > handleTabUnpin: function TabItems_handleTabUnpin(xulTab) { > this.link(xulTab); > this.update(xulTab); > }, > > // ---------- >- // Function: heartbeat >- // Allows us to spreadout update calls over a period of time. >- heartbeat: function TabItems_heartbeat() { >- if (!this._heartbeatOn) >+ // Function: startHeartbeat >+ // Start a new heartbeat if there isn't one already started. >+ // The heartbeat is a chain of setTimeout calls that allows us to spread >+ // out update calls over a period of time. >+ // _heartbeatOn is used to make sure that we don't add multiple >+ // setTimeout chains. >+ startHeartbeat: function TabItems_startHeartbeat() { >+ if (!this._heartbeatOn) { >+ this._heartbeatOn = true; >+ let self = this; >+ setTimeout(function() { >+ self._checkHeartbeat(); >+ }, this._heartbeatTiming); >+ } >+ }, >+ >+ // ---------- >+ // Function: _checkHeartbeat >+ // This periodically checks for tabs waiting to be updated, and calls >+ // _update on them. >+ // Should only be called by startHeartbeat and resumePainting. >+ _checkHeartbeat: function TabItems__checkHeartbeat() { >+ if (this.isPaintingPaused()) > return; > >- if (this._tabsWaitingForUpdate.length) { >+ if (this._tabsWaitingForUpdate.length && UI.isIdle()) { > this._update(this._tabsWaitingForUpdate[0]); >- // _update will remove the tab from the waiting list >+ //_update will remove the tab from the waiting list > } > >- let self = this; > if (this._tabsWaitingForUpdate.length) { >+ let self = this; > setTimeout(function() { >- self.heartbeat(); >+ self._checkHeartbeat(); > }, this._heartbeatTiming); > } else >- this._hearbeatOn = false; >- }, >- >- // ---------- >- // Function: pausePainting >- // Tells TabItems to stop updating thumbnails (so you can do >- // animations without thumbnail paints causing stutters). >- // pausePainting can be called multiple times, but every call to >- // pausePainting needs to be mirrored with a call to <resumePainting>. >- pausePainting: function TabItems_pausePainting() { >- this.paintingPaused++; >- >- if (this.isPaintingPaused() && this._heartbeatOn) >+ // we have nothing else to do. > this._heartbeatOn = false; > }, > >- // ---------- >- // Function: resumePainting >- // Undoes a call to <pausePainting>. For instance, if you called >- // pausePainting three times in a row, you'll need to call resumePainting >- // three times before TabItems will start updating thumbnails again. >- resumePainting: function TabItems_resumePainting() { >- this.paintingPaused--; >- >- if (!this.isPaintingPaused() && >- this._tabsWaitingForUpdate.length && >- !this._heartbeatOn) { >- this._heartbeatOn = true; >- this.heartbeat(); >- } >- }, >+ // ---------- >+ // Function: pausePainting >+ // Tells TabItems to stop updating thumbnails (so you can do >+ // animations without thumbnail paints causing stutters). >+ // pausePainting can be called multiple times, but every call to >+ // pausePainting needs to be mirrored with a call to <resumePainting>. >+ pausePainting: function TabItems_pausePainting() { >+ this.paintingPaused++; >+ }, >+ >+ // ---------- >+ // Function: resumePainting >+ // Undoes a call to <pausePainting>. For instance, if you called >+ // pausePainting three times in a row, you'll need to call resumePainting >+ // three times before TabItems will start updating thumbnails again. >+ resumePainting: function TabItems_resumePainting() { >+ this.paintingPaused--; >+ if (!this.isPaintingPaused()) >+ this._checkHeartbeat(); >+ }, > > // ---------- > // Function: isPaintingPaused > // Returns a boolean indicating whether painting > // is paused or not. > isPaintingPaused: function TabItems_isPaintingPaused() { > return this.paintingPaused > 0; > }, >diff --git a/browser/base/content/tabview/ui.js b/browser/base/content/tabview/ui.js >--- a/browser/base/content/tabview/ui.js >+++ b/browser/base/content/tabview/ui.js >@@ -83,16 +83,22 @@ let UI = { > // Variable: _eventListeners > // Keeps track of event listeners added to the AllTabs object. > _eventListeners: {}, > > // Variable: _cleanupFunctions > // An array of functions to be called at uninit time > _cleanupFunctions: [], > >+ // Constant: _maxInteractiveWait >+ // If the UI is in the middle of an operation, this is the max amount of >+ // milliseconds to wait between input events before we no longer consider >+ // the operation interactive. >+ _maxInteractiveWait: 250, >+ > // ---------- > // Function: init > // Must be called after the object is created. > init: function UI_init() { > try { > let self = this; > > // ___ storage >@@ -284,16 +290,28 @@ let UI = { > // > blurAll: function UI_blurAll() { > iQ(":focus").each(function(element) { > element.blur(); > }); > }, > > // ---------- >+ // Function: isIdle >+ // Returns true if the last interaction was long enough ago to consider the >+ // UI idle. Used to determine whether interactivity would be sacrificed if >+ // the CPU was to become busy. >+ // >+ isIdle: function UI_isIdle() { >+ let time = Date.now(); >+ let maxEvent = Math.max(drag.lastMoveTime, resize.lastMoveTime); >+ return (time - maxEvent) > this._maxInteractiveWait; >+ }, >+ >+ // ---------- > // Function: getActiveTab > // Returns the currently active tab as a <TabItem> > // > getActiveTab: function UI_getActiveTab() { > return this._activeTab; > }, > > // ---------- >diff --git a/browser/base/content/test/tabview/Makefile.in b/browser/base/content/test/tabview/Makefile.in >--- a/browser/base/content/test/tabview/Makefile.in >+++ b/browser/base/content/test/tabview/Makefile.in >@@ -42,16 +42,17 @@ relativesrcdir = browser/base/content/t > > include $(DEPTH)/config/autoconf.mk > include $(topsrcdir)/config/rules.mk > > _BROWSER_FILES = \ > browser_tabview_apptabs.js \ > browser_tabview_bug580412.js \ > browser_tabview_bug587040.js \ >+ browser_tabview_bug587231.js \ > browser_tabview_bug587990.js \ > browser_tabview_bug590606.js \ > browser_tabview_bug591706.js \ > browser_tabview_bug594176.js \ > browser_tabview_bug595191.js \ > browser_tabview_bug595518.js \ > browser_tabview_bug595804.js \ > browser_tabview_bug595943.js \ >diff --git a/browser/base/content/test/tabview/browser_tabview_bug587231.js b/browser/base/content/test/tabview/browser_tabview_bug587231.js >new file mode 100644 >--- /dev/null >+++ b/browser/base/content/test/tabview/browser_tabview_bug587231.js >@@ -0,0 +1,125 @@ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is tabview drag and drop test. >+ * >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (C) 2010 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Sean Dunn <seanedunn@yahoo.com> >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either the GNU General Public License Version 2 or later (the "GPL"), or >+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * and other provisions required by the GPL or the LGPL. If you do not delete >+ * the provisions above, a recipient may use your version of this file under >+ * the terms of any one of the MPL, the GPL or the LGPL. >+ * >+ * ***** END LICENSE BLOCK ***** */ >+ >+let activeTab; >+let testTab; >+let testGroup; >+let contentWindow; >+ >+function test() { >+ waitForExplicitFinish(); >+ contentWindow = document.getElementById("tab-view").contentWindow; >+ >+ // create new tab >+ testTab = gBrowser.addTab("http://mochi.test:8888/"); >+ >+ window.addEventListener("tabviewshown", onTabViewWindowLoaded, false); >+ TabView.toggle(); >+} >+ >+function onTabViewWindowLoaded() { >+ window.removeEventListener("tabviewshown", onTabViewWindowLoaded, false); >+ ok(TabView.isVisible(), "Tab View is visible"); >+ >+ // create group >+ let testGroupRect = new contentWindow.Rect(20, 20, 300, 300); >+ testGroup = new contentWindow.GroupItem([], { bounds: testGroupRect }); >+ ok(testGroup.isEmpty(), "This group is empty"); >+ >+ // place tab in group >+ let testTabItem = testTab.tabItem; >+ >+ if (testTabItem.parent) >+ testTabItem.parent.remove(testTabItem); >+ testGroup.add(testTabItem); >+ >+ // record last update time of tab canvas >+ let initialUpdateTime = testTabItem._lastTabUpdateTime; >+ >+ // simulate resize >+ let resizer = contentWindow.iQ('.iq-resizable-handle', testGroup.container)[0]; >+ let offsetX = 100; >+ let offsetY = 100; >+ let delay = contentWindow.TabItems._heartbeatTiming; >+ >+ let funcChain = new Array(); >+ funcChain.push(function() { >+ EventUtils.synthesizeMouse( >+ resizer, 1, 1, { type: "mousedown" }, contentWindow); >+ setTimeout(funcChain.shift(), delay); >+ }); >+ // drag >+ for (let i = 4; i >= 0; i--) { >+ funcChain.push(function() { >+ EventUtils.synthesizeMouse( >+ resizer, Math.round(offsetX/4), Math.round(offsetY/4), >+ { type: "mousemove" }, contentWindow); >+ setTimeout(funcChain.shift(), delay); >+ }); >+ } >+ funcChain.push(function() { >+ EventUtils.synthesizeMouse(resizer, 0, 0, { type: "mouseup" }, >+ contentWindow); >+ setTimeout(funcChain.shift(), delay); >+ }); >+ funcChain.push(function() { >+ // verify that update time has changed after last update >+ ok((testTabItem._lastTabUpdateTime - initialUpdateTime) > >+ contentWindow.TabItems._heartbeatTiming, >+ "Tab has been updated."); >+ >+ // clean up >+ testGroup.remove(testTab.tabItem); >+ testTab.tabItem.close(); >+ testGroup.close(); >+ >+ let currentTabs = contentWindow.TabItems.getItems(); >+ ok(currentTabs[0], "A tab item exists to make active"); >+ contentWindow.UI.setActiveTab(currentTabs[0]); >+ >+ window.addEventListener("tabviewhidden", finishTest, false); >+ TabView.toggle(); >+ }); >+ setTimeout(funcChain.shift(), delay); >+} >+ >+function finishTest() { >+ window.removeEventListener("tabviewhidden", finishTest, false); >+ ok(!TabView.isVisible(), "Tab View is not visible"); >+ >+ finish(); >+}

(In reply to comment #22) > For example, should you ever call startHeartbeat() while painting is paused, > checkHeartbeat() just immediately returns without starting the timer, and now > your heartbeat state is broken. > > IE, heartbeatOn indicates if there's a pending setTimeout or not, so you need > to avoid getting into a state where heartbeatOn == true but no timer was > started (or heartbeatOn == false when there is one, but the code prevents this > already). In the case where heartbeatOn is true, and painting is paused (so no timer is set), it doesn't matter, since _checkHeartbeat is called after resuming painting, and the setTimeout chain is gauranteed to be started again. Despite that, I made your change, and I also fixed a possible situation where if pause/resume was called while a setTimeout was in flight, it would cause two sets of timeout chains to exist: I now call startHeartbeat from resumePainting as well, guaranteeing that setTimeout is only ever called if there isn't a timeout pending.

Comment on attachment 485813 [details] [diff] [review] v7 (w/test) ># vim: se ft=diff : ># HG changeset patch ># User Sean Dunn <seanedunn@yahoo.com> ># Date 2010-09-09 22:29 >Bug 587231 - "[Win7] Thumbnails sometimes like an abstract painting in TabCandy" [] > >diff --git a/browser/base/content/tabview/drag.js b/browser/base/content/tabview/drag.js >--- a/browser/base/content/tabview/drag.js >+++ b/browser/base/content/tabview/drag.js >@@ -38,19 +38,27 @@ > // ********** > // Title: drag.js > > // ---------- > // Variable: drag > // The Drag that's currently in process. > var drag = { > info: null, >- zIndex: 100 >+ zIndex: 100, >+ lastMoveTime: 0 > }; > >+//---------- >+//Variable: resize >+//The resize (actually a Drag) that is currently in process >+var resize = { >+ info: null, >+ lastMoveTime: 0 >+}; > > // ########## > // Class: Drag (formerly DragInfo) > // Helper class for dragging <Item>s > // > // ---------- > // Constructor: Drag > // Called to create a Drag in response to an <Item> draggable "start" event. >diff --git a/browser/base/content/tabview/items.js b/browser/base/content/tabview/items.js >--- a/browser/base/content/tabview/items.js >+++ b/browser/base/content/tabview/items.js >@@ -211,35 +211,34 @@ Item.prototype = { > // Private to this file. > accept: function dropAcceptFunction(item) { > return (item && item.isATabItem && (!item.parent || !item.parent.expanded)); > } > }; > > // ___ resize > var self = this; >- var resizeInfo = null; > this.resizeOptions = { > aspectRatio: self.keepProportional, > minWidth: 90, > minHeight: 90, > start: function(e,ui) { > if (this.isAGroupItem) > GroupItems.setActiveGroupItem(this); >- resizeInfo = new Drag(this, e, true); // true = isResizing >+ resize.info = new Drag(this, e, true); // true = isResizing > }, > resize: function(e,ui) { > // TODO: maybe the stationaryCorner should be topright for rtl langs? >- resizeInfo.snap('topleft', false, self.keepProportional); >+ resize.info.snap('topleft', false, self.keepProportional); > }, > stop: function() { > self.setUserSize(); > self.pushAway(); >- resizeInfo.stop(); >- resizeInfo = null; >+ resize.info.stop(); >+ resize.info = null; > } > }; > }, > > // ---------- > // Function: getBounds > // Returns a copy of the Item's bounds as a <Rect>. > getBounds: function Item_getBounds() { >@@ -595,16 +594,19 @@ Item.prototype = { > var startPos; > var startSent; > var startEvent; > var droppables; > var dropTarget; > > // ___ mousemove > var handleMouseMove = function(e) { >+ // global drag tracking >+ drag.lastMoveTime = Date.now(); >+ > // positioning > var mouse = new Point(e.pageX, e.pageY); > if (!startSent) { > if(Math.abs(mouse.x - startMouse.x) > self.dragOptions.minDragDistance || > Math.abs(mouse.y - startMouse.y) > self.dragOptions.minDragDistance) { > if (typeof self.dragOptions.start == "function") > self.dragOptions.start.apply(self, > [startEvent, {position: {left: startPos.x, top: startPos.y}}]); >@@ -762,16 +764,19 @@ Item.prototype = { > $container.addClass('iq-resizable'); > > var self = this; > var startMouse; > var startSize; > > // ___ mousemove > var handleMouseMove = function(e) { >+ // global resize tracking >+ resize.lastMoveTime = Date.now(); >+ > var mouse = new Point(e.pageX, e.pageY); > var box = self.getBounds(); > box.width = Math.max(self.resizeOptions.minWidth || 0, startSize.x + (mouse.x - startMouse.x)); > box.height = Math.max(self.resizeOptions.minHeight || 0, startSize.y + (mouse.y - startMouse.y)); > > if (self.resizeOptions.aspectRatio) { > if (startAspect < 1) > box.height = box.width * startAspect; >diff --git a/browser/base/content/tabview/tabitems.js b/browser/base/content/tabview/tabitems.js >--- a/browser/base/content/tabview/tabitems.js >+++ b/browser/base/content/tabview/tabitems.js >@@ -91,16 +91,18 @@ function TabItem(tab, options) { > this.sizeExtra.x = parseInt($div.css('padding-left')) > + parseInt($div.css('padding-right')); > > this.sizeExtra.y = parseInt($div.css('padding-top')) > + parseInt($div.css('padding-bottom')); > > this.bounds = $div.bounds(); > >+ this._lastTabUpdateTime = Date.now(); >+ > // ___ superclass setup > this._init($div[0]); > > // ___ reconnect to data from Storage > this._hasBeenDrawn = false; > let reconnected = TabItems.reconnect(this); > > // ___ drag/drop >@@ -670,18 +672,18 @@ TabItem.prototype = Utils.extend(new Ite > let TabItems = { > minTabWidth: 40, > tabWidth: 160, > tabHeight: 120, > fontSize: 9, > items: [], > paintingPaused: 0, > _tabsWaitingForUpdate: [], >- _heartbeatOn: false, >- _heartbeatTiming: 100, // milliseconds between beats >+ _heartbeatOn: false, // see explanation at startHeartbeat() below >+ _heartbeatTiming: 100, // milliseconds between _checkHeartbeat() calls > _lastUpdateTime: Date.now(), > _eventListeners: [], > > // ---------- > // Function: init > // Set up the necessary tracking to maintain the <TabItems>s. > init: function TabItems_init() { > Utils.assert(window.AllTabs, "AllTabs must be initialized first"); >@@ -760,16 +762,17 @@ let TabItems = { > let isCurrentTab = ( > !UI._isTabViewVisible() && > tab == gBrowser.selectedTab > ); > > if (shouldDefer && !isCurrentTab) { > if (this._tabsWaitingForUpdate.indexOf(tab) == -1) > this._tabsWaitingForUpdate.push(tab); >+ this.startHeartbeat(); > } else > this._update(tab); > } catch(e) { > Utils.log(e); > } > }, > > // ---------- >@@ -820,27 +823,28 @@ let TabItems = { > let w = $canvas.width(); > let h = $canvas.height(); > if (w != tabItem.canvasEl.width || h != tabItem.canvasEl.height) { > tabItem.canvasEl.width = w; > tabItem.canvasEl.height = h; > } > } > >+ this._lastUpdateTime = Date.now(); >+ tabItem._lastTabUpdateTime = this._lastUpdateTime; >+ > tabItem.tabCanvas.paint(); > > // ___ cache > // TODO: this logic needs to be better; hiding too soon now > if (tabItem.isShowingCachedData && !tab.hasAttribute("busy")) > tabItem.hideCachedData(); > } catch(e) { > Utils.log(e); > } >- >- this._lastUpdateTime = Date.now(); > }, > > // ---------- > // Function: link > // Takes in a xul:tab, creates a TabItem for it and adds it to the scene. > link: function TabItems_link(tab, options) { > try { > Utils.assertThrow(tab, "tab"); >@@ -887,64 +891,73 @@ let TabItems = { > // ---------- > // when a tab becomes unpinned, create a TabItem for it > handleTabUnpin: function TabItems_handleTabUnpin(xulTab) { > this.link(xulTab); > this.update(xulTab); > }, > > // ---------- >- // Function: heartbeat >- // Allows us to spreadout update calls over a period of time. >- heartbeat: function TabItems_heartbeat() { >- if (!this._heartbeatOn) >+ // Function: startHeartbeat >+ // Start a new heartbeat if there isn't one already started. >+ // The heartbeat is a chain of setTimeout calls that allows us to spread >+ // out update calls over a period of time. >+ // _heartbeatOn is used to make sure that we don't add multiple >+ // setTimeout chains. >+ startHeartbeat: function TabItems_startHeartbeat() { >+ if (!this._heartbeatOn) { >+ this._heartbeatOn = true; >+ let self = this; >+ setTimeout(function() { >+ self._checkHeartbeat(); >+ }, this._heartbeatTiming); >+ } >+ }, >+ >+ // ---------- >+ // Function: _checkHeartbeat >+ // This periodically checks for tabs waiting to be updated, and calls >+ // _update on them. >+ // Should only be called by startHeartbeat and resumePainting. >+ _checkHeartbeat: function TabItems__checkHeartbeat() { >+ this._heartbeatOn = false; >+ >+ if (this.isPaintingPaused()) > return; > >- if (this._tabsWaitingForUpdate.length) { >+ if (this._tabsWaitingForUpdate.length && UI.isIdle()) { > this._update(this._tabsWaitingForUpdate[0]); >- // _update will remove the tab from the waiting list >+ //_update will remove the tab from the waiting list > } > >- let self = this; > if (this._tabsWaitingForUpdate.length) { >- setTimeout(function() { >- self.heartbeat(); >- }, this._heartbeatTiming); >- } else >- this._hearbeatOn = false; >+ this.startHeartbeat(); >+ } > }, > >- // ---------- >- // Function: pausePainting >- // Tells TabItems to stop updating thumbnails (so you can do >- // animations without thumbnail paints causing stutters). >- // pausePainting can be called multiple times, but every call to >- // pausePainting needs to be mirrored with a call to <resumePainting>. >- pausePainting: function TabItems_pausePainting() { >- this.paintingPaused++; >- >- if (this.isPaintingPaused() && this._heartbeatOn) >- this._heartbeatOn = false; >- }, >- >- // ---------- >- // Function: resumePainting >- // Undoes a call to <pausePainting>. For instance, if you called >- // pausePainting three times in a row, you'll need to call resumePainting >- // three times before TabItems will start updating thumbnails again. >- resumePainting: function TabItems_resumePainting() { >- this.paintingPaused--; >- >- if (!this.isPaintingPaused() && >- this._tabsWaitingForUpdate.length && >- !this._heartbeatOn) { >- this._heartbeatOn = true; >- this.heartbeat(); >- } >- }, >+ // ---------- >+ // Function: pausePainting >+ // Tells TabItems to stop updating thumbnails (so you can do >+ // animations without thumbnail paints causing stutters). >+ // pausePainting can be called multiple times, but every call to >+ // pausePainting needs to be mirrored with a call to <resumePainting>. >+ pausePainting: function TabItems_pausePainting() { >+ this.paintingPaused++; >+ }, >+ >+ // ---------- >+ // Function: resumePainting >+ // Undoes a call to <pausePainting>. For instance, if you called >+ // pausePainting three times in a row, you'll need to call resumePainting >+ // three times before TabItems will start updating thumbnails again. >+ resumePainting: function TabItems_resumePainting() { >+ this.paintingPaused--; >+ if (!this.isPaintingPaused()) >+ this.startHeartbeat(); >+ }, > > // ---------- > // Function: isPaintingPaused > // Returns a boolean indicating whether painting > // is paused or not. > isPaintingPaused: function TabItems_isPaintingPaused() { > return this.paintingPaused > 0; > }, >diff --git a/browser/base/content/tabview/ui.js b/browser/base/content/tabview/ui.js >--- a/browser/base/content/tabview/ui.js >+++ b/browser/base/content/tabview/ui.js >@@ -83,16 +83,22 @@ let UI = { > // Variable: _eventListeners > // Keeps track of event listeners added to the AllTabs object. > _eventListeners: {}, > > // Variable: _cleanupFunctions > // An array of functions to be called at uninit time > _cleanupFunctions: [], > >+ // Constant: _maxInteractiveWait >+ // If the UI is in the middle of an operation, this is the max amount of >+ // milliseconds to wait between input events before we no longer consider >+ // the operation interactive. >+ _maxInteractiveWait: 250, >+ > // ---------- > // Function: init > // Must be called after the object is created. > init: function UI_init() { > try { > let self = this; > > // ___ storage >@@ -284,16 +290,28 @@ let UI = { > // > blurAll: function UI_blurAll() { > iQ(":focus").each(function(element) { > element.blur(); > }); > }, > > // ---------- >+ // Function: isIdle >+ // Returns true if the last interaction was long enough ago to consider the >+ // UI idle. Used to determine whether interactivity would be sacrificed if >+ // the CPU was to become busy. >+ // >+ isIdle: function UI_isIdle() { >+ let time = Date.now(); >+ let maxEvent = Math.max(drag.lastMoveTime, resize.lastMoveTime); >+ return (time - maxEvent) > this._maxInteractiveWait; >+ }, >+ >+ // ---------- > // Function: getActiveTab > // Returns the currently active tab as a <TabItem> > // > getActiveTab: function UI_getActiveTab() { > return this._activeTab; > }, > > // ---------- >diff --git a/browser/base/content/test/tabview/Makefile.in b/browser/base/content/test/tabview/Makefile.in >--- a/browser/base/content/test/tabview/Makefile.in >+++ b/browser/base/content/test/tabview/Makefile.in >@@ -42,16 +42,17 @@ relativesrcdir = browser/base/content/t > > include $(DEPTH)/config/autoconf.mk > include $(topsrcdir)/config/rules.mk > > _BROWSER_FILES = \ > browser_tabview_apptabs.js \ > browser_tabview_bug580412.js \ > browser_tabview_bug587040.js \ >+ browser_tabview_bug587231.js \ > browser_tabview_bug587990.js \ > browser_tabview_bug590606.js \ > browser_tabview_bug591706.js \ > browser_tabview_bug594176.js \ > browser_tabview_bug595191.js \ > browser_tabview_bug595518.js \ > browser_tabview_bug595804.js \ > browser_tabview_bug595943.js \ >diff --git a/browser/base/content/test/tabview/browser_tabview_bug587231.js b/browser/base/content/test/tabview/browser_tabview_bug587231.js >new file mode 100644 >--- /dev/null >+++ b/browser/base/content/test/tabview/browser_tabview_bug587231.js >@@ -0,0 +1,125 @@ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is tabview drag and drop test. >+ * >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (C) 2010 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Sean Dunn <seanedunn@yahoo.com> >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either the GNU General Public License Version 2 or later (the "GPL"), or >+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * and other provisions required by the GPL or the LGPL. If you do not delete >+ * the provisions above, a recipient may use your version of this file under >+ * the terms of any one of the MPL, the GPL or the LGPL. >+ * >+ * ***** END LICENSE BLOCK ***** */ >+ >+let activeTab; >+let testTab; >+let testGroup; >+let contentWindow; >+ >+function test() { >+ waitForExplicitFinish(); >+ contentWindow = document.getElementById("tab-view").contentWindow; >+ >+ // create new tab >+ testTab = gBrowser.addTab("http://mochi.test:8888/"); >+ >+ window.addEventListener("tabviewshown", onTabViewWindowLoaded, false); >+ TabView.toggle(); >+} >+ >+function onTabViewWindowLoaded() { >+ window.removeEventListener("tabviewshown", onTabViewWindowLoaded, false); >+ ok(TabView.isVisible(), "Tab View is visible"); >+ >+ // create group >+ let testGroupRect = new contentWindow.Rect(20, 20, 300, 300); >+ testGroup = new contentWindow.GroupItem([], { bounds: testGroupRect }); >+ ok(testGroup.isEmpty(), "This group is empty"); >+ >+ // place tab in group >+ let testTabItem = testTab.tabItem; >+ >+ if (testTabItem.parent) >+ testTabItem.parent.remove(testTabItem); >+ testGroup.add(testTabItem); >+ >+ // record last update time of tab canvas >+ let initialUpdateTime = testTabItem._lastTabUpdateTime; >+ >+ // simulate resize >+ let resizer = contentWindow.iQ('.iq-resizable-handle', testGroup.container)[0]; >+ let offsetX = 100; >+ let offsetY = 100; >+ let delay = contentWindow.TabItems._heartbeatTiming; >+ >+ let funcChain = new Array(); >+ funcChain.push(function() { >+ EventUtils.synthesizeMouse( >+ resizer, 1, 1, { type: "mousedown" }, contentWindow); >+ setTimeout(funcChain.shift(), delay); >+ }); >+ // drag >+ for (let i = 4; i >= 0; i--) { >+ funcChain.push(function() { >+ EventUtils.synthesizeMouse( >+ resizer, Math.round(offsetX/4), Math.round(offsetY/4), >+ { type: "mousemove" }, contentWindow); >+ setTimeout(funcChain.shift(), delay); >+ }); >+ } >+ funcChain.push(function() { >+ EventUtils.synthesizeMouse(resizer, 0, 0, { type: "mouseup" }, >+ contentWindow); >+ setTimeout(funcChain.shift(), delay); >+ }); >+ funcChain.push(function() { >+ // verify that update time has changed after last update >+ ok((testTabItem._lastTabUpdateTime - initialUpdateTime) > >+ contentWindow.TabItems._heartbeatTiming, >+ "Tab has been updated."); >+ >+ // clean up >+ testGroup.remove(testTab.tabItem); >+ testTab.tabItem.close(); >+ testGroup.close(); >+ >+ let currentTabs = contentWindow.TabItems.getItems(); >+ ok(currentTabs[0], "A tab item exists to make active"); >+ contentWindow.UI.setActiveTab(currentTabs[0]); >+ >+ window.addEventListener("tabviewhidden", finishTest, false); >+ TabView.toggle(); >+ }); >+ setTimeout(funcChain.shift(), delay); >+} >+ >+function finishTest() { >+ window.removeEventListener("tabviewhidden", finishTest, false); >+ ok(!TabView.isVisible(), "Tab View is not visible"); >+ >+ finish(); >+}

Unrotted

Sean, please package for check-in. Also, does it need a try run?

I figured out how to get rid of the "abstract painting" look on Windows machines, which is different than just updating the thumbnail. The drawWindow() call does a nearest-neighbor pixel copy, which looks awful at resolutions < 150px wide. When the tab is smaller than this, I first copy into a temp canvas, and then do a drawImage() call, which does a proper bilinear interpolation. It now looks as good as the mac, and since we're dealing with such small images, the extra copy is very fast. I did a perf measurement, and the difference was less than 1%.

Comment on attachment 490226 [details] [diff] [review] v7 alternate (w/test) Very slick!

Comment on attachment 490226 [details] [diff] [review] v7 alternate (w/test) Dolske, can you review a change I made to this? It's in the TabCanvas.paint() call. It lets us get around the awful looking abstract painting artifacts on small thumbnails.

Try: http://hg.mozilla.org/try/pushloghtml?changeset=22a2cbca851d

Comment on attachment 490226 [details] [diff] [review] v7 alternate (w/test) r+ from me for this new bit. I suppose when packaging, go ahead and say r=dolske+ian since we both reviewed parts.

(In reply to comment #31) > Try: http://hg.mozilla.org/try/pushloghtml?changeset=22a2cbca851d Was the try successful?

Try failed, fixing test now.

I had to add in a bit of delay on the UI synthesis to make sure the update loop gets a chance to run. Pushed to try: http://hg.mozilla.org/try/rev/cc8bf02c2032

Passed try!

I was able to reproduce this issue using the steps in Bug 648530 on: Mozilla/5.0 (Windows NT 5.1; rv:5.0a2) Gecko/20110418 Firefox/5.0a2