Last Comment Bug 677310 - Thumbnails are lost when switching to private browsing mode
: Thumbnails are lost when switching to private browsing mode
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 9
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 677423
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-08 11:20 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (47.99 KB, patch)
2011-08-08 16:29 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (41.81 KB, patch)
2011-08-08 17:39 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v3 (41.98 KB, patch)
2011-08-09 10:28 PDT, Tim Taubert [:ttaubert]
dietrich: feedback+
Details | Diff | Splinter Review
patch v4 (42.43 KB, patch)
2011-08-17 09:07 PDT, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-08-08 11:20:10 PDT
Panorama loses thumbnails of the currently displayed pages when switching to and from private browsing mode.
Comment 1 Tim Taubert [:ttaubert] 2011-08-08 16:29:29 PDT
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?
Comment 2 Tim Taubert [:ttaubert] 2011-08-08 17:39:29 PDT
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.
Comment 3 Tim Taubert [:ttaubert] 2011-08-09 10:28:51 PDT
Created attachment 551818 [details] [diff] [review]
patch v3

Patch updated because it got a little bitrot.
Comment 4 Dietrich Ayala (:dietrich) 2011-08-15 20:29:56 PDT
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] 2011-08-17 09:07:59 PDT
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.
Comment 6 Dietrich Ayala (:dietrich) 2011-08-22 14:28:04 PDT
Comment on attachment 553795 [details] [diff] [review]
patch v4

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

r=me
Comment 7 Tim Taubert [:ttaubert] 2011-09-01 06:22:36 PDT
http://hg.mozilla.org/integration/fx-team/rev/b5bb731a58a9
Comment 8 Rob Campbell [:rc] (:robcee) 2011-09-02 05:38:18 PDT
Comment on attachment 553795 [details] [diff] [review]
patch v4

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

Note You need to log in before you can comment on or make changes to this bug.