Requirement for bug 659594: We should let thumbnails have their own update queue and update them separately from the "normal" tabItem data (url, title, favicon, etc).

I moved all heartbeat/queue functionality into the DelayedTabQueue "class". TabItems.update(tab) and TabItems.updateThumbnail(tab) are now called via separated queues. I removed the old TabPriorityQueue because that's not needed anymore. Some tests needed to be fixed.

Comment on attachment 564542 [details] [diff] [review] patch v1 + tabItem._sendToSubscribers("thumbnail-updated"); It would be better to stick with the camel format as other events i.e. thumbnailUpdated.

Comment on attachment 564542 [details] [diff] [review] patch v1 Review of attachment 564542 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabview/tabitems.js @@ +936,5 @@ > + // Even if the page hasn't loaded, display the favicon and title > + > + // ___ icon > + if (UI.shouldLoadFavIcon(tab.linkedBrowser)) { > + let iconUrl = UI.getFavIconUrlForTab(tab); huh, is this done such that favicons can load async into panorama? @@ +985,3 @@ > > + // don't update the thumbnail if the tab hasn't been restored, yet > + if ("__SS_restoreState" in browser && browser.__SS_restoreState == 1) hm, would really like this encapsulated (preferably by session restore, but at least in this code) into a tabIsRestored helper or something like that. @@ +999,5 @@ > + // Updates the thumbnail of a given tab. > + // > + // Parameters: > + // tab - the tab who's thumbnail will be updated > + _updateThumbnail: function TabItems__updateThumbnail(tab) { please rename this in a self-documenting way or at least point out in the comments why updateThumbnail and _updateThumbnail are separate.

I've fixed one of the oranges happened in try related to browser_tabview_bug595601.js. There is still an orange which happens randomly in browser_tabview_bug650573.js test, I have tried many ways to fix it but without luck. Tim: could you have a look at it when you have time please?

Minor update and it passed try fine. https://tbpl.mozilla.org/?tree=Try&rev=e465a964b79a

Comment on attachment 576143 [details] [diff] [review] v3 Review of attachment 576143 [details] [diff] [review]: ----------------------------------------------------------------- Can we please rename TabItems.update/_update and TabItems.updateThumbnail/_updateThumbnail so that they're named in a self-documenting way so that it's clear why these two methods of each exist? r=me with these fixes. Thanks for wrapping this up! ::: browser/components/tabview/tabitems.js @@ +948,5 @@ > + // Check whether a given tab is restored or not. > + // > + // Parameters: > + // tab - the xul tab > + _isTabRestored: function TabItems__isTabRestored(tab) { This method does not check whether the tab is restored but whether it _needs_ restoring. Please rename it accordingly. @@ +968,2 @@ > > + this._delayedTabQueue.push(tab); Let's write it like this: if (this._isTabRestored(tab)) this._delayedTabQueue.push(tab); @@ +1130,5 @@ > // 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++; > + this._delayedTabQueueThumbnails.pause(); We could write it like that: if (0 == this.painingPaused++) this._delayedTabQueueThumbnails.pause(); This way we don't try to pause the queue multiple times.

(In reply to Tim Taubert [:ttaubert] from comment #6) > Comment on attachment 576143 [details] [diff] [review] [diff] [details] [review] > v3 > > Review of attachment 576143 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > Can we please rename TabItems.update/_update and > TabItems.updateThumbnail/_updateThumbnail so that they're named in a > self-documenting way so that it's clear why these two methods of each exist? > Updated the name > r=me with these fixes. Thanks for wrapping this up! > > ::: browser/components/tabview/tabitems.js > @@ +948,5 @@ > > + // Check whether a given tab is restored or not. > > + // > > + // Parameters: > > + // tab - the xul tab > > + _isTabRestored: function TabItems__isTabRestored(tab) { > > This method does not check whether the tab is restored but whether it > _needs_ restoring. Please rename it accordingly. > > @@ +968,2 @@ > > > > + this._delayedTabQueue.push(tab); > > Let's write it like this: > > if (this._isTabRestored(tab)) > this._delayedTabQueue.push(tab); Fixed > > @@ +1130,5 @@ > > // 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++; > > + this._delayedTabQueueThumbnails.pause(); > > We could write it like that: > > if (0 == this.painingPaused++) > this._delayedTabQueueThumbnails.pause(); > > This way we don't try to pause the queue multiple times. Updated. N.B. Please apply patch for bug 659594 before this one.

Passed Try https://tbpl.mozilla.org/?tree=Try&rev=d8f0dd703e3c

Passed Try with the latest patch for bug 659594 https://tbpl.mozilla.org/?tree=Try&rev=2cb308fc079a

(In reply to Raymond Lee [:raymondlee] from comment #10) > Created attachment 577563 [details] [diff] [review] [diff] [details] [review] > Patch for checkin Pushed it to Try and waiting for the results https://tbpl.mozilla.org/?tree=Try&rev=694e09cb17ad

(In reply to Raymond Lee [:raymondlee] from comment #11) > (In reply to Raymond Lee [:raymondlee] from comment #10) > > Created attachment 577563 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] > > Patch for checkin > > Pushed it to Try and waiting for the results > https://tbpl.mozilla.org/?tree=Try&rev=694e09cb17ad Passed Try!

Backed out - https://hg.mozilla.org/integration/fx-team/rev/5cc5c3319fe8 browser_tabview_bug594958.js failed on at least Win dbg. Maybe also on Linux dbg. Unfortunately the log for this build somehow disappeared...

Fixed some error shown while running tests. Some tests in sessionstore components don't pass which I am not sure why it's related. I've spent quite a while to investigate but still can't find the cause by Panorama. Submitted to try again https://tbpl.mozilla.org/?tree=Try&rev=2b270bd9968a

Un-bitrotted with bug 659594 applied.

Comment on attachment 616532 [details] [diff] [review] patch v4 Review of attachment 616532 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/tabview/delayedTabQueue.js @@ +59,5 @@ > + // > + // Parameters: > + // tab - the tab to be added to the queue > + push: function DTQ_push(tab) { > + let prio = this._getTabPriority(tab); not a lot gained by shortening to "prio", so just expand to "priority" here and elsewhere. @@ +105,5 @@ > + }, > + > + // ---------- > + // Function: _getTabPriority > + // Determines the priority for a given tab. please document what criteria are used to determine priority. @@ +109,5 @@ > + // Determines the priority for a given tab. > + // > + // Parameters: > + // tab - the tab for which we want to get the priority > + _getTabPriority: function DTQ_getTabPriority(tab) { tab is not used. stopping here, since this doesn't look right.

Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs. If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality. See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info. We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.