Last Comment Bug 744152 - [Page Thumbnails] Capture thumbnails only for the selected tab
: [Page Thumbnails] Capture thumbnails only for the selected tab
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 14
Assigned To: Tim Taubert [:ttaubert]
: Dão Gottwald [:dao]
Depends on:
Blocks: 742594
  Show dependency treegraph
Reported: 2012-04-10 13:29 PDT by Tim Taubert [:ttaubert]
Modified: 2012-09-22 11:12 PDT (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1 (1.38 KB, patch)
2012-04-10 17:31 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (1.35 KB, patch)
2012-04-11 01:56 PDT, Tim Taubert [:ttaubert]
dietrich: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-04-10 13:29:32 PDT
To minimize drawWindow's performance impact for thumbnails being captured in the background we should do it only for selected tabs.
Comment 1 Tim Taubert [:ttaubert] 2012-04-10 17:31:48 PDT
Created attachment 613833 [details] [diff] [review]
patch v1

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.
Comment 2 Dão Gottwald [:dao] 2012-04-11 01:40:33 PDT
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 = ...;
Comment 3 Tim Taubert [:ttaubert] 2012-04-11 01:52:57 PDT
(In reply to Dão Gottwald [:dao] from comment #2)
> This should be above let doc = ...;

Um, yeah of course. Will fix, thanks.
Comment 4 Tim Taubert [:ttaubert] 2012-04-11 01:56:41 PDT
Created attachment 613914 [details] [diff] [review]
patch v2
Comment 5 Tim Taubert [:ttaubert] 2012-04-13 15:19:55 PDT
Comment 6 Tim Taubert [:ttaubert] 2012-04-14 02:44:49 PDT
Comment 7 Tim Taubert [:ttaubert] 2012-04-14 03:58:38 PDT
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
Comment 8 Asa Dotzler [:asa] 2012-04-14 21:19:02 PDT
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 9 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-16 15:32:20 PDT
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.
Comment 10 Tim Taubert [:ttaubert] 2012-04-17 12:49:22 PDT
Comment 11 Matt Brubeck (:mbrubeck) 2012-04-19 08:39:17 PDT
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.)
Comment 12 Virgil Dicu [:virgil] [QA] 2012-05-16 04:49:09 PDT
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?
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-16 09:41:54 PDT
(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.
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-02 09:13:12 PDT
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.

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