Thumbnails are lost when switching to private browsing mode

RESOLVED FIXED in Firefox 9

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 9

Details

Attachments

(1 attachment, 3 obsolete attachments)

Panorama loses thumbnails of the currently displayed pages when switching to and from private browsing mode.
Created attachment 551612 [details] [diff] [review]
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?
Attachment #551612 - Flags: feedback?(dietrich)
Depends on: 677423
Created attachment 551640 [details] [diff] [review]
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.
Attachment #551612 - Attachment is obsolete: true
Attachment #551612 - Flags: feedback?(dietrich)
Attachment #551640 - Flags: feedback?(dietrich)
Created attachment 551818 [details] [diff] [review]
patch v3

Patch updated because it got a little bitrot.
Attachment #551640 - Attachment is obsolete: true
Attachment #551640 - Flags: feedback?(dietrich)
Attachment #551818 - Flags: review?(dietrich)
Attachment #551818 - Flags: review?(dietrich) → feedback?(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?
Attachment #551818 - Flags: feedback?(dietrich) → feedback+
Created attachment 553795 [details] [diff] [review]
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.
Attachment #551818 - Attachment is obsolete: true
Attachment #553795 - Flags: review?(dietrich)
Comment on attachment 553795 [details] [diff] [review]
patch v4

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

r=me
Attachment #553795 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/integration/fx-team/rev/b5bb731a58a9
Whiteboard: [fixed-in-fx-team]
Comment on attachment 553795 [details] [diff] [review]
patch v4

http://hg.mozilla.org/mozilla-central/rev/b5bb731a58a9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Whiteboard: [fixed-in-fx-team]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.