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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

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

Comment 1

[Mark Hammond [:markh] [: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.

Comment 2

[Drew Willcoxon :adw]

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

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0654c1a3c7f