Last Comment Bug 745303 - Let Panorama use the thumbnail service
: Let Panorama use the thumbnail service
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 14
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on:
Blocks: 738128
  Show dependency treegraph
 
Reported: 2012-04-13 12:48 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1 - Make Panorama use the thumbnail service (37.22 KB, patch)
2012-04-13 16:23 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 2 - Correct Panorama tests (19.54 KB, patch)
2012-04-13 16:23 PDT, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review
Part 1 - Make Panorama use the thumbnail service (37.72 KB, patch)
2012-04-13 16:37 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 1 - Make Panorama use the thumbnail service (37.62 KB, patch)
2012-04-13 16:59 PDT, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-04-13 12:48:17 PDT
Panorama should use the thumbnail service to display thumbnails for not-yet-restored tabs. We can rip out the whole thumbnail storage and other code parts with this.

Also, this will fix bug 738128.
Comment 1 Tim Taubert [:ttaubert] 2012-04-13 16:23:12 PDT
Created attachment 614948 [details] [diff] [review]
Part 1 - Make Panorama use the thumbnail service
Comment 2 Tim Taubert [:ttaubert] 2012-04-13 16:23:43 PDT
Created attachment 614949 [details] [diff] [review]
Part 2 - Correct Panorama tests
Comment 3 Tim Taubert [:ttaubert] 2012-04-13 16:37:13 PDT
Created attachment 614953 [details] [diff] [review]
Part 1 - Make Panorama use the thumbnail service
Comment 4 Tim Taubert [:ttaubert] 2012-04-13 16:47:17 PDT
Comment on attachment 614949 [details] [diff] [review]
Part 2 - Correct Panorama tests

Explaining some changes:

>--- a/browser/components/tabview/test/Makefile.in
>                  browser_tabview_bug596781.js \
>-                 browser_tabview_bug597248.js \
>                  browser_tabview_bug597360.js \

Removing the test for bug 597248. No need to make sure Panorama's thumbnail cache is solid anymore.

>                  browser_tabview_bug626791.js \
>-                 browser_tabview_bug627288.js \
>                  browser_tabview_bug627736.js \

Removing test for bug 627288 because TabItem.showCachedData() retrieves its data now from TabItem.getTabState() which is never stale. Title and URL (and therefore the thumbnail) are always up-to-date.

>                  browser_tabview_bug673729.js \
>-                 browser_tabview_bug677310.js \
>                  browser_tabview_bug678374.js \

Removing test for bug 677310 because we can't control whether all thumbnails have been captured before transitioning into private browsing mode. This is okay since we shouldn't execute any sync operations and most thumbnails should have been stored anyway before entering PB mode.
Comment 5 Tim Taubert [:ttaubert] 2012-04-13 16:59:47 PDT
Created attachment 614958 [details] [diff] [review]
Part 1 - Make Panorama use the thumbnail service

And again explaining some changes:

>--- a/browser/components/tabview/content.js

There's a lot of stuff we can remove from the Panorama content script. The majority of the code was there to figure out the storage policy for thumbnails.

>diff --git a/browser/components/tabview/storagePolicy.js b/browser/components/tabview/storagePolicy.js
>diff --git a/browser/components/tabview/thumbnailStorage.js b/browser/components/tabview/thumbnailStorage.js

We don't the thumbnail storage and its storage policy anymore.

>--- a/browser/components/tabview/tabitems.js
>   _reconnect: function TabItem__reconnect(options) {
>     Utils.assertThrow(!this._reconnected, "shouldn't already be reconnected");
>     Utils.assertThrow(this.tab, "should have a xul:tab");
> 
>     let tabData = Storage.getTabData(this.tab);
>     let groupItem;
> 
>     if (tabData && TabItems.storageSanity(tabData)) {
>-      this.loadThumbnail();
>+      // Show the cached data while we're waiting for the tabItem to be updated.
>+      // If the tab isn't restored yet this acts as a placeholder until it is.
>+      this.showCachedData();
> 

We never show stale data, we just create a placeholder while the tab is loading/restoring or waiting to be restored. So we can just unconditionally call showCachedData() because this gets removed when the TabItem is updated.
Comment 6 Dietrich Ayala (:dietrich) 2012-04-17 14:34:15 PDT
Comment on attachment 614958 [details] [diff] [review]
Part 1 - Make Panorama use the thumbnail service

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

yay code removal. boo on making the tooltip changes on an unrelated patch :P but they look ok, so r=me.
Comment 7 Tim Taubert [:ttaubert] 2012-04-18 11:24:37 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #6)
> yay code removal. boo on making the tooltip changes on an unrelated patch :P
> but they look ok, so r=me.

Sorry, I couldn't resist :)
Comment 9 Tim Taubert [:ttaubert] 2012-04-18 23:36:04 PDT
Forgot to remove the now empty TabItem.loadThumbnail() method:

https://hg.mozilla.org/integration/fx-team/rev/c2ee16305bc6
Comment 11 Tim Taubert [:ttaubert] 2012-05-14 06:33:55 PDT
https://hg.mozilla.org/integration/fx-team/rev/06438370bd3f
Comment 12 Tim Taubert [:ttaubert] 2012-05-14 14:38:12 PDT
https://hg.mozilla.org/mozilla-central/rev/06438370bd3f

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