1058237 - Ctrl-Tab previews should use the PageThumbs API

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Ctrl-Tab preview code currently has custom code calling drawWindow. It should instead use the PageThumbs API. This way it can get the previews cached on disk, which has stopped working ever since browser-thumbnails.js started excluding pages that aren't on about:newtab.

Since it looks like drawWindow called from the parent process isn't going to work for remote tabs (bug 698371), this is also a means to get this code e10s ready such at will benefit automatically from bug 863512.

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8478550 [details] [diff] [review]
patch

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

::: browser/base/content/browser-tabPreviews.js
@@ +59,4 @@
>  
> +      PageThumbs.captureAndStore(browser, function () {
> +        img.src = PageThumbs.getThumbnailURL(uri);
> +      });

So why are we capturing and storing here as well? browser-thumbnails.js takes care of that already for top-site thumbnails. Creating a canvas, capturing and returning it should be sufficient, no?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> Comment on attachment 8478550 [details] [diff] [review]
> patch
> 
> Review of attachment 8478550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-tabPreviews.js
> @@ +59,4 @@
> >  
> > +      PageThumbs.captureAndStore(browser, function () {
> > +        img.src = PageThumbs.getThumbnailURL(uri);
> > +      });
> 
> So why are we capturing and storing here as well?

Because otherwise these thumbnails are blank when restoring a session with unloaded tabs. (Except for pages that also happen to be on about:newtab.)

Comment 3

[Dão Gottwald [:dao]]

That's also why this patch adds an expiration filter. This is pointless if PageThumbs doesn't store these thumbnails in the first place.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8478550 [details] [diff] [review]
patch

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

::: browser/base/content/browser-tabPreviews.js
@@ +52,4 @@
>  
> +    // FIXME: The gBrowserThumbnails._shouldCapture determines whether
> +    //        thumbnails should be written to disk. This should somehow be part
> +    //        of the PageThumbs API.

Agreed, if more places need it, it would make sense to move it there.

::: browser/base/content/browser-thumbnails.js
@@ +123,4 @@
>      this._timeouts.set(aBrowser, timeout);
>    },
>  
>    _shouldCapture: function Thumbnails_shouldCapture(aBrowser) {

Should probably make this a "public" function.

@@ -127,5 @@
>        return false;
>  
> -    // Only capture about:newtab top sites.
> -    if (this._topSiteURLs.indexOf(aBrowser.currentURI.spec) < 0)
> -      return false;

Instead of moving this to _capture(), how about adding a second parameter to .shouldCapture() to take options? We could then pass something like {onlyTopSites: false}.

Comment 5

[Dão Gottwald [:dao]]

> ::: browser/base/content/browser-thumbnails.js
> @@ +123,4 @@
> >      this._timeouts.set(aBrowser, timeout);
> >    },
> >  
> >    _shouldCapture: function Thumbnails_shouldCapture(aBrowser) {
> 
> Should probably make this a "public" function.
> 
> @@ -127,5 @@
> >        return false;
> >  
> > -    // Only capture about:newtab top sites.
> > -    if (this._topSiteURLs.indexOf(aBrowser.currentURI.spec) < 0)
> > -      return false;
> 
> Instead of moving this to _capture(), how about adding a second parameter to
> .shouldCapture() to take options? We could then pass something like
> {onlyTopSites: false}.

Since we seem to agree that this should be in PageThumbs, I'd rather file a bug on that than making this method public or adding further parameters to it.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Since we seem to agree that this should be in PageThumbs, I'd rather file a
> bug on that than making this method public or adding further parameters to
> it.

Sure, wfm too.

Comment 7

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/e4d0c551410e

filed bug 1062414

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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