Let Panorama use the thumbnail service

RESOLVED FIXED in Firefox 14

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 14

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 614948 [details] [diff] [review]
Part 1 - Make Panorama use the thumbnail service
(Assignee)

Comment 2

5 years ago
Created attachment 614949 [details] [diff] [review]
Part 2 - Correct Panorama tests
(Assignee)

Comment 3

5 years ago
Created attachment 614953 [details] [diff] [review]
Part 1 - Make Panorama use the thumbnail service
Attachment #614948 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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.
Attachment #614949 - Flags: review?(dietrich)
(Assignee)

Comment 5

5 years ago
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.
Attachment #614953 - Attachment is obsolete: true
Attachment #614958 - Flags: review?(dietrich)
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.
Attachment #614958 - Flags: review?(dietrich) → review+
Attachment #614949 - Flags: review?(dietrich) → review+
(Assignee)

Comment 7

5 years ago
(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 :)
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/d42051ed9a81
https://hg.mozilla.org/integration/fx-team/rev/15387b04ebf2
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
(Assignee)

Comment 9

5 years ago
Forgot to remove the now empty TabItem.loadThumbnail() method:

https://hg.mozilla.org/integration/fx-team/rev/c2ee16305bc6
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/mozilla-central/rev/d42051ed9a81
https://hg.mozilla.org/mozilla-central/rev/15387b04ebf2
https://hg.mozilla.org/mozilla-central/rev/c2ee16305bc6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/06438370bd3f
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/mozilla-central/rev/06438370bd3f
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.