744152 - [Page Thumbnails] Capture thumbnails only for the selected tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

Attachment: 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]]

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] (inactive)]

(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] (inactive)]

Attachment: patch v2

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/fc693ba6ce24

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/fc693ba6ce24

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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]]

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]

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] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/360971f3fcc3

Comment 11

[Matt Brubeck (:mbrubeck)]

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]]

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

[u279076]

(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

[u279076]

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.