Closed Bug 960941 Opened 8 years ago Closed 8 years ago

gBrowserThumbnails expiration filter should be based on top sites, not open pages

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Bug 809056 made gBrowserThumbnails capture and store only top sites, but it kept its expiration filter, which prevents the thumbnails of all open browsers from expiring.  The filter should instead prevent top sites from expiring.  Right now, if you happen not to have a top site open when expiration runs, its thumbnail is unnecessarily removed (and then possibly replaced with a bad background thumbnail the next time you open newtab, which is a likely explanation for bug 952460 but not the only explanation).
Attachment #8361569 - Flags: review?(mhammond)
Comment on attachment 8361569 [details] [diff] [review]
patch

Review of attachment 8361569 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-thumbnails.js
@@ +121,5 @@
>      this._timeouts.set(aBrowser, timeout);
>    },
>  
>    _shouldCapture: function Thumbnails_shouldCapture(aBrowser) {
> +    // Capture only if it's the currently selected tab.

I'd have thought we could now drop this check - if it's a top site, it seems we should just capture it whether it's currently selected or not.  It means there's more chance of the top-site's thumbnail being current.

But if the concern here is performance and the impact of thumbnailing, then it's fine to keep this.
Attachment #8361569 - Flags: review?(mhammond) → review+
Yeah, ideally we'd capture regardless, but given all our work on reducing thumbnailing impact, I'm hesitant to make that change at this time in this bug.  I'm imagining starting the browser with pending tabs disabled (i.e., so that all restored tabs load immediately) and triggering nine captures all at once.  Probably not a huge performance impact, but probably an impact.  The fact that we capture on tab select, which goes some way toward mitigating not capturing background tabs, makes me a tiny bit even more hesitant.

Thanks for the quick review.

https://tbpl.mozilla.org/?tree=Try&rev=ab23482af30b
https://hg.mozilla.org/integration/fx-team/rev/b0654c1a3c7f
https://hg.mozilla.org/mozilla-central/rev/b0654c1a3c7f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Depends on: 1350781
You need to log in before you can comment on or make changes to this bug.