Closed Bug 831440 Opened 8 years ago Closed 7 years ago

Work - Switch back to static thumbnails for the tab bar

Categories

(Firefox for Metro Graveyard :: Browser, defect)

All
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: fryn, Assigned: rsilveira)

References

Details

(Whiteboard: feature=work)

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → fyan
Status: NEW → ASSIGNED
No longer blocks: 777749
Depends on: 777749
OS: Windows 8 → Windows 8 Metro
Hardware: x86_64 → All
Whiteboard: [metro-mvp][LOE:2]
Blocks: 831924
Whiteboard: [metro-mvp][LOE:2] → [metro-mvp][LOE:2] feature=work
Whiteboard: [metro-mvp][LOE:2] feature=work → feature=work
Summary: Switch back to static thumbnails for the tab bar → Work - Switch back to static thumbnails for the tab bar
No longer blocks: 831924
Duplicate of this bug: 784851
Blocks: 836007
No longer blocks: 836007
I'm not going to be working on this.

Jonathan, would you be interested in taking this?
Assignee: fyan → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jwilde)
Likely not happening this iteration, but sure. :D
Flags: needinfo?(jwilde)
Assignee: nobody → jwilde
Status: NEW → ASSIGNED
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.
Assignee: jwilde → sfoster
Attachment #774212 - Flags: review?(mbrubeck)
Attachment #774212 - Flags: review?(mbrubeck) → review+
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.
Whiteboard: feature=work → feature=work [leave-open]
Whiteboard: feature=work [leave-open] → feature=work [leave open]
p=2
Assignee: sfoster → rsilveira
Blocks: 801088
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #787015 - Flags: review?(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.
Attachment #787015 - Flags: review?(mbrubeck) → review-
(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?
(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.
(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?
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #787015 - Attachment is obsolete: true
Attachment #787768 - Flags: review?(mbrubeck)
Attachment #787768 - Flags: review?(mbrubeck) → review+
Attached patch Patch v2.1Splinter Review
Forgot to clear the timeout after closing the tab. It was causing some exceptions while running the tests.
Attachment #787768 - Attachment is obsolete: true
Attachment #787861 - Flags: review?(mbrubeck)
Attachment #787861 - Flags: review?(mbrubeck) → review+
Whiteboard: feature=work [leave open] → feature=work
Nit: this is missing an r=mbrubeck in the changeset description, please add it next time.
(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.
Note to self: bug 720575 could make it possible to update these thumbnails more often without much perf impact.
https://hg.mozilla.org/mozilla-central/rev/a009409e3ab2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.