Closed Bug 960941 Opened 8 years ago Closed 8 years ago

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


(Firefox :: General, defect)

Not set



Firefox 29


(Reporter: adw, Assigned: adw)


(Blocks 1 open bug)



(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]

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.
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.