831440 - Work - Switch back to static thumbnails for the tab bar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Frank Yan (:fryn)]

While live thumbnails are cool for quick demos, they cause problems for performance and they actually are worse for usability, because the most recognizable thumbnail is one that is taken of the state of the page at the moment the user switched away from that page.

Let's switch back to static thumbnails for these reasons and to fix bug 829782 and bug 790111.

I might pick up this bug. Firefox desktop recently switched to using full-page-width, top-of-page thumbnails, which I like, so I might reuse some of that code. A possibly better alternative is to take full-page-width, scroll-position-preserving thumbnails.

Comment 2

[Frank Yan (:fryn)]

I'm not going to be working on this.

Jonathan, would you be interested in taking this?

Comment 3

[Jonathan Wilde [:jwilde]]

Likely not happening this iteration, but sure. :D

Comment 4

[Sam Foster [:sfoster] (he/him)]

Attachment: Explicitly unassign the -moz-element tab backgrounds when tabstrip is not visible

Before we tackle the larger task of creating and maintaining static thumbnails for tabs, maybe this smaller fix mitigates the perf issue? It seems like the thumbnails are live and continually update even when the tabstrip is hidden. This patch fixes that. With some limited here I think I'm seeing a more stable perf. behavior as you add lots of tabs. 
Showing and panning across the tabstrip is still pretty janky. 
Even if this is only part of a fix, I think it doesn't hurt and we can build on it.

Comment 5

[Sam Foster [:sfoster] (he/him)]

On inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5d46fceb26

Lets leave this ticket open for now, and make further plans after we see how this affects things.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b5d46fceb26

Comment 7

[Rodrigo Silveira [:rsilveira] [:rodms]]

p=2

Comment 8

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v1

Using PageThumbs.jsm and WebProgress' "Content:StateChange" message like topsites. It doesn't work well for sites that do client-side rendering. For example on bing.com the thumbnail doesn't have the bg image and on gmail you get the progress bar.

I don't know if there is a better event to start the capture of the thumbnail, but maybe once we move to BackgroundPageThumbs we can periodically update.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 787015 [details] [diff] [review]
Patch v1

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

(In reply to Rodrigo Silveira [:rsilveira] from comment #8)
> Using PageThumbs.jsm and WebProgress' "Content:StateChange" message like
> topsites. It doesn't work well for sites that do client-side rendering. For
> example on bing.com the thumbnail doesn't have the bg image and on gmail you
> get the progress bar.

The desktop Firefox new tab page code uses a 1-second delay after the StateChange event to mitigate this:
http://hg.mozilla.org/mozilla-central/file/47bd850cb89b/browser/base/content/browser-thumbnails.js#l16

We should probably do something similar, though maybe with a shorter delay because our use case is different.

> I don't know if there is a better event to start the capture of the
> thumbnail, but maybe once we move to BackgroundPageThumbs we can
> periodically update.

I don't think we want to use BackgroundPageThumbs for the tab strip, since they can (intentionally) capture something different than what the user sees.  Though that does bring up the privacy issue:  PageThumbs.captureAndStore writes its thumbnails to disk.  How can we avoid potential data leaks of content that is not supposed to be cacheable?  Could we call PageThumbs.captureToCanvas instead?

Alternately, we could call PageThumbsStorage.remove when closing a tab or navigating away from a page... but I guess this could fail to happen in cases where the browser is killed/suspended by the OS.

r- until we work out the privacy issues.

Comment 10

[Asa Dotzler [:asa]]

(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> Though that does bring up the privacy issue: 
> PageThumbs.captureAndStore writes its thumbnails to disk.  How can we avoid
> potential data leaks of content that is not supposed to be cacheable?
> 
> r- until we work out the privacy issues.

What's the privacy issue here? Do we really have a policy that says we cannot capture and store thumbnails of what ever we want when ever we want? What are the specific constraints of that policy?

Comment 11

[Matt Brubeck (:mbrubeck)]

(In reply to Asa Dotzler [:asa] from comment #10)
> What's the privacy issue here? Do we really have a policy that says we
> cannot capture and store thumbnails of what ever we want when ever we want?
> What are the specific constraints of that policy?

The HTTP spec (RFC 2616, sec. 14.9) requires browsers to respect headers such as "Cache-Control: no-store":  *"If sent in a request, a cache MUST NOT store any part of either this request or any response to it."*  This is an important part of the web security model.

Also, we should do this in memory for performance reasons -- We don't want to write to a slow disk and then read from that slow disk for a tab strip thumbnail, since we aren't actually saving these thumbnails to use later.  We only draw them to the tab strip while the page is open, so saving them to disk does not buy us anything.

Comment 12

[Asa Dotzler [:asa]]

(In reply to Matt Brubeck (:mbrubeck) from comment #11)
> (In reply to Asa Dotzler [:asa] from comment #10)
> > What's the privacy issue here? Do we really have a policy that says we
> > cannot capture and store thumbnails of what ever we want when ever we want?
> > What are the specific constraints of that policy?
> 
> The HTTP spec (RFC 2616, sec. 14.9) requires browsers to respect headers
> such as "Cache-Control: no-store":  *"If sent in a request, a cache MUST NOT
> store any part of either this request or any response to it."*  This is an
> important part of the web security model.

Ah, I thought you were talking about Firefox privacy policy, not Web policy. 

Is this really true? Capturing thumbnails of content is the same as caching the document or part of it?  IMO, a thumbnail is not part of the document, it's an artifact the app creates and uses independently of the network. It seems odd to mme that browser UI must conform to a networking spec. Are we sure this is the case?  

> Also, we should do this in memory for performance reasons -- We don't want
> to write to a slow disk and then read from that slow disk for a tab strip
> thumbnail, since we aren't actually saving these thumbnails to use later. 
> We only draw them to the tab strip while the page is open, so saving them to
> disk does not buy us anything.

I'm all for performance and simple implementations. Let's get this crap off the disk ;)  

Still, are we really required to adhere to the HTTP spec when we're not talking about any of the actual content of the files that came over the wire?

Comment 13

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v2

Capturing to canvas now.

Also added a .5 sec delay after loading to refresh the thumbnail to be more likely to get things rendered after load like bing.com's image.

Comment 14

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v2.1

Forgot to clear the timeout after closing the tab. It was causing some exceptions while running the tests.

Comment 15

[Rodrigo Silveira [:rsilveira] [:rodms]]

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

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Nit: this is missing an r=mbrubeck in the changeset description, please add it next time.

Comment 17

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Tim Taubert [:ttaubert] from comment #16)
> Nit: this is missing an r=mbrubeck in the changeset description, please add
> it next time.

Will do, thanks.

Comment 18

[Matt Brubeck (:mbrubeck)]

Note to self: bug 720575 could make it possible to update these thumbnails more often without much perf impact.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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