Closed Bug 691740 Opened 9 years ago Closed 4 years ago

Update thumbnails separately in their own queue

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ttaubert, Unassigned)

References

Details

Attachments

(1 file, 7 obsolete files)

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).
Blocks: 676638
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #564542 - Flags: review?(dietrich)
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.
Attachment #564542 - Flags: review?(dietrich) → review+
Blocks: 692716
Blocks: 692706
Attached patch Patch 2 (obsolete) — Splinter Review
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?
No longer blocks: 659594
Depends on: 659594
Attached patch v3 (obsolete) — Splinter Review
Minor update and it passed try fine.

https://tbpl.mozilla.org/?tree=Try&rev=e465a964b79a
Attachment #564542 - Attachment is obsolete: true
Attachment #568860 - Attachment is obsolete: true
Attachment #576143 - Flags: review?(ttaubert)
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.
Attachment #576143 - Flags: review?(ttaubert) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
(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.
Attachment #576143 - Attachment is obsolete: true
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #576962 - Attachment is obsolete: true
(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...
Whiteboard: [fixed-in-fx-team]
Attached patch WIP (obsolete) — Splinter Review
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
Attachment #577563 - Attachment is obsolete: true
Attached patch patch v4 (WIP) (obsolete) — Splinter Review
Un-bitrotted with bug 659594 applied.
Attachment #580274 - Attachment is obsolete: true
Attached patch patch v4Splinter Review
Attachment #615330 - Attachment is obsolete: true
Attachment #616532 - Flags: review?(dietrich)
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.
Attachment #616532 - Flags: review?(dietrich)
Assignee: ttaubert → nobody
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.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.