677310 - Thumbnails are lost when switching to private browsing mode

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Panorama loses thumbnails of the currently displayed pages when switching to and from private browsing mode.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Here's what I did:

1) All thumbnails are now saved synchronously when entering PB mode.
2) I refactored the thumbnail storage so that .loadThumbnail() and .saveThumbnail() expect urls only and not tabs anymore. I didn't like that the thumbnail storage itself had to know so much about tabs when its only purpose is to store image data in a cache (SRP).
3) Thumbnails are now saved right after the canvas got updated (but delayed for 2secs - like session store does). This ensures that thumbnails get saved early and asynchronously and we don't have to do massive amounts of sync saving when quitting or switching to PB mode.
4) I added/moved all thumbnail storage related test to a named test - browser_tabview_thubmnail_storage.js
5) I created a togglePrivateBrowsing function in head.js because we use that in a lot of places.

I know this is quite a lot but the scope crept a bit while working on it... Do you think I should split this into multiple patches or even multiple dependent bugs?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to Tim Taubert [:ttaubert] from comment #1)
> 5) I created a togglePrivateBrowsing function in head.js because we use that
> in a lot of places.

Moved to bug 677423.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Patch updated because it got a little bitrot.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 551818 [details] [diff] [review]
patch v3

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

a couple of nits. fix them or don't, but patch looks fine otherwise.

::: browser/base/content/tabview/tabitems.js
@@ +78,5 @@
>    this.$cachedThumb = iQ('img.cached-thumb', $div);
>    this.$favImage = iQ('.favicon>img', $div);
>    this.$close = iQ('.close', $div);
>  
> +  let self = this;

super nit: move down to where first used

@@ +249,5 @@
>    // ----------
> +  // Function: loadThumbnail
> +  // Loads the tabItems thumbnail.
> +  loadThumbnail: function TabItem_loadThumbnail(tabData) {
> +    Utils.assert(tabData, "tabData");

better assertion msg plz.

@@ +253,5 @@
> +    Utils.assert(tabData, "tabData");
> +
> +    let self = this;
> +
> +    ThumbnailStorage.loadThumbnail(tabData.url, function (error, imageData) {

give anon func a name?

@@ +281,5 @@
> +    if (!this._thumbnailNeedsSaving)
> +      return;
> +
> +    // check the storage policy to see if we're allowed to store the thumbnail
> +    if (!StoragePolicy.canStoreThumbnailFor(this.tab)) {

hm, do you actually need the tab here? or would just the URL be enough?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

(In reply to Dietrich Ayala (:dietrich) from comment #4)
> > +  let self = this;
> 
> super nit: move down to where first used

Fixed.

> > +  loadThumbnail: function TabItem_loadThumbnail(tabData) {
> > +    Utils.assert(tabData, "tabData");
> 
> better assertion msg plz.

Fixed.

> > +    ThumbnailStorage.loadThumbnail(tabData.url, function (error, imageData) {
> 
> give anon func a name?

Fixed.

> > +    // check the storage policy to see if we're allowed to store the thumbnail
> > +    if (!StoragePolicy.canStoreThumbnailFor(this.tab)) {
> 
> hm, do you actually need the tab here? or would just the URL be enough?

We could let the StoragePolicy use the url as the key for denied browsers/tabs but I think we shouldn't do that because in the future there might be a per-tab or per-window private browsing mode where the same urls could require a different handling.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 553795 [details] [diff] [review]
patch v4

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

r=me

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/b5bb731a58a9

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 553795 [details] [diff] [review]
patch v4

http://hg.mozilla.org/mozilla-central/rev/b5bb731a58a9