Closed Bug 744152 Opened 11 years ago Closed 11 years ago

[Page Thumbnails] Capture thumbnails only for the selected tab


(Firefox :: Tabbed Browser, defect)

Not set



Firefox 14
Tracking Status
firefox13 --- verified


(Reporter: ttaubert, Assigned: ttaubert)


(Blocks 1 open bug)


(Whiteboard: [Snappy:P1][qa+:virgil])


(1 file, 1 obsolete file)

To minimize drawWindow's performance impact for thumbnails being captured in the background we should do it only for selected tabs.
Attached patch patch v1 (obsolete) — Splinter Review
This patch captures thumbnails only if the given browser is the currently selected browser. Additionally, I decreased the capture timeout from 2s to 1s to not wait too long before taking a screenshot. This should increase chances when quickly navigating, switching tabs and scrolling.
Attachment #613833 - Flags: review?(dietrich)
Comment on attachment 613833 [details] [diff] [review]
patch v1

>   _shouldCapture: function Thumbnails_shouldCapture(aBrowser) {
>     let doc = aBrowser.contentDocument;
>+    // Capture only if it's the currently selected tab.
>+    if (aBrowser != gBrowser.selectedBrowser)
>+      return false;

This should be above let doc = ...;
(In reply to Dão Gottwald [:dao] from comment #2)
> This should be above let doc = ...;

Um, yeah of course. Will fix, thanks.
Attached patch patch v2Splinter Review
Attachment #613833 - Attachment is obsolete: true
Attachment #613833 - Flags: review?(dietrich)
Attachment #613914 - Flags: review?(dietrich)
Whiteboard: [Snappy] → [Snappy:P1]
Attachment #613914 - Flags: review?(dietrich) → review+
Whiteboard: [Snappy:P1] → [Snappy:P1][fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P1][fixed-in-fx-team] → [Snappy:P1]
Comment on attachment 613914 [details] [diff] [review]
patch v2

[Approval Request Comment]
User impact if declined: thumbnail service might be too much of a performance impact
Risk to taking this patch (and alternatives if risky): very low risk, tiny patch
String changes made by this patch: none
Attachment #613914 - Flags: approval-mozilla-aurora?
I'd like to see us take this as part of the remaining few clean-up bugs before Net Tab gets uplifted to Beta.
Comment on attachment 613914 [details] [diff] [review]
patch v2

[Triage Comment]
Approving for landing in support of Asa's request for Net Tab feature support for upcoming beta.
Attachment #613914 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This bug caused performance regressions in a11y 2 MozAfterPaint on Linux and tscroll 2 on Mac, and possibly other benchmarks like Sunspider 2 MozAfterPaint.  (The regression cause wasn't identified on inbound/central because of a problem with the graph server, but it is clear in the Aurora graphs.)
Whiteboard: [Snappy:P1] → [Snappy:P1][qa+]
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0

Verified on 13 beta 3 with Ubuntu 12.04, Mac OS 10.6 and Windows 7. Thumbnails are only saved for focused tabs.

Are there any reasons why this shouldn't be set to verified yet due to comment 11?
(In reply to Virgil Dicu [:virgil] [QA] from comment #12)
> Are there any reasons why this shouldn't be set to verified yet due to
> comment 11?

Given this is a Snappy:P1 and the patch has resulted in a performance regression, I'm wondering that too.
Marking this one verified fixed for Firefox 13. As per Virgil's testing in comment 12, the functionality is working as expected. Please comment if there is something further you want QA to test.
Whiteboard: [Snappy:P1][qa+] → [Snappy:P1][qa+:virgil]
You need to log in before you can comment on or make changes to this bug.