Closed Bug 779618 Opened 12 years ago Closed 12 years ago

[Page Thumbnails] make tabbrowser use the thumbnail service

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

(Whiteboard: [Snappy])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
This patch introduces a PageThumbs.captureToCanvas() method which provides the API needed by the tabbrowser.
Attachment #648068 - Flags: review?(jaws)
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.
(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.
(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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Reverted the signature change for capture().
Attachment #648068 - Attachment is obsolete: true
Attachment #648068 - Flags: review?(jaws)
Attachment #648648 - Flags: review?(jaws)
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?
Attachment #648648 - Flags: review?(jaws) → feedback+
Attached patch patch v3Splinter Review
(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.
Attachment #648648 - Attachment is obsolete: true
Attachment #649223 - Flags: review?(jaws)
Attachment #649223 - Flags: review?(jaws) → review+
https://hg.mozilla.org/integration/fx-team/rev/3411e98278d6
Whiteboard: [Snappy] → [Snappy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3411e98278d6
https://hg.mozilla.org/mozilla-central/rev/54f9a8c340c8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy][fixed-in-fx-team] → [Snappy]
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.