Closed Bug 677310 Opened 8 years ago Closed 8 years ago

Thumbnails are lost when switching to private browsing mode

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file, 3 obsolete files)

Panorama loses thumbnails of the currently displayed pages when switching to and from private browsing mode.
Attached patch patch v1 (obsolete) — Splinter Review
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
Attached patch patch v2 (obsolete) — Splinter Review
(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)
Attached patch patch v3 (obsolete) — Splinter Review
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+
Attached patch patch v4Splinter Review
(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+
Status: ASSIGNED → RESOLVED
Closed: 8 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.