Last Comment Bug 779618 - [Page Thumbnails] make tabbrowser use the thumbnail service
: [Page Thumbnails] make tabbrowser use the thumbnail service
Status: RESOLVED FIXED
[Snappy]
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-01 13:36 PDT by Tim Taubert [:ttaubert]
Modified: 2012-08-09 11:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (9.79 KB, patch)
2012-08-01 13:58 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v2 (8.51 KB, patch)
2012-08-03 02:32 PDT, Tim Taubert [:ttaubert]
jaws: feedback+
Details | Diff | Review
patch v3 (8.77 KB, patch)
2012-08-06 02:39 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-08-01 13:36:54 PDT
While playing around with the Gecko profiler I noticed a call to tabPreviews_capture() that caught my attention. I don't use ctrlTab/allTab so I thought it shouldn't be called.

Turned out that's okay because I obviously dragged a tab around:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3410

The real problem here is that this initializes the tabPreviews object which then automatically captures thumbnails on TabSelect and SSTabRestored. That means once the user dragged a tab around, we have two services capturing thumbnails for open tabs.

The easiest solution would probably be to pass some argument around that indicates we don't want to initialize tabPreviews - the real solution should be to make tabbrowser use PageThumbs.jsm.
Comment 1 Tim Taubert [:ttaubert] 2012-08-01 13:58:22 PDT
Created attachment 648068 [details] [diff] [review]
patch v1

This patch introduces a PageThumbs.captureToCanvas() method which provides the API needed by the tabbrowser.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-01 14:18:11 PDT
Comment on attachment 648068 [details] [diff] [review]
patch v1

Why change capture()'s signature (to take a browser instead of a window)? Seems like a gratuitous API change, since the only use of aBrowser is just getting .contentWindow.
Comment 3 Tim Taubert [:ttaubert] 2012-08-01 14:31:59 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Why change capture()'s signature (to take a browser instead of a window)?
> Seems like a gratuitous API change, since the only use of aBrowser is just
> getting .contentWindow.

I thought about not doing this but captureAndStore() takes a browser. So shouldn't captureToCanvas(), too? As well as capture()?

Also, captureAndStore() calls capture(). capture() calls captureToCanvas() which is both used internally and externally. As capture() knows about the window only we'd have to make captureToCanvas() take a window as well.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 00:18:20 PDT
(In reply to Tim Taubert [:ttaubert] from comment #3)
> I thought about not doing this but captureAndStore() takes a browser. So
> shouldn't captureToCanvas(), too? As well as capture()?

I don't think the consistency here is valuable. captureAndStore has a very specific use (capture and store a thumbnail for a given browser, so needs the browser's URI for storage), while the other two are more general purpose (they return the captured data directly, and thus only need a window). I don't see a problem with that.
Comment 5 Tim Taubert [:ttaubert] 2012-08-03 00:42:46 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> (In reply to Tim Taubert [:ttaubert] from comment #3)
> > I thought about not doing this but captureAndStore() takes a browser. So
> > shouldn't captureToCanvas(), too? As well as capture()?
> 
> I don't think the consistency here is valuable. captureAndStore has a very
> specific use (capture and store a thumbnail for a given browser, so needs
> the browser's URI for storage), while the other two are more general purpose
> (they return the captured data directly, and thus only need a window). I
> don't see a problem with that.

I didn't see it that way - make sense to me. Will update the patch accordingly.
Comment 6 Tim Taubert [:ttaubert] 2012-08-03 02:32:01 PDT
Created attachment 648648 [details] [diff] [review]
patch v2

Reverted the signature change for capture().
Comment 7 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-03 16:08:24 PDT
Comment on attachment 648648 [details] [diff] [review]
patch v2

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

::: browser/base/content/tabbrowser.xml
@@ +3410,5 @@
> +        // Create a canvas to which we capture the current tab.
> +        let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> +        canvas.mozOpaque = true;
> +        canvas.width = Math.ceil(screen.availWidth / 5.75);
> +        canvas.height = Math.round(canvas.width * 0.5625);

Where do these magic numbers come from?
Comment 8 Tim Taubert [:ttaubert] 2012-08-06 02:39:44 PDT
Created attachment 649223 [details] [diff] [review]
patch v3

(In reply to Jared Wein [:jaws] from comment #7)
> > +        canvas.width = Math.ceil(screen.availWidth / 5.75);
> > +        canvas.height = Math.round(canvas.width * 0.5625);
> 
> Where do these magic numbers come from?

Sorry, I just copied them from browser-tabPreviews.js. I added some comments and put them in consts to hopefully better describe what they're used for.

I can't say why ~1/6th of the screen was chosen as the drag image width because there wasn't any information in the bugs that implemented it. But it makes sense because it adapts to the available screen size so it doesn't get too small on big screens and feels just right on smaller screens.
Comment 9 Tim Taubert [:ttaubert] 2012-08-08 23:48:26 PDT
https://hg.mozilla.org/integration/fx-team/rev/3411e98278d6
Comment 10 Tim Taubert [:ttaubert] 2012-08-09 05:46:39 PDT
Pushed a little follow-up:

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

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