The default bug view has changed. See this FAQ.

[Page Thumbnails] make tabbrowser use the thumbnail service

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 648068 [details] [diff] [review]
patch v1

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.
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 648648 [details] [diff] [review]
patch v2

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+
(Assignee)

Comment 8

5 years ago
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.
Attachment #648648 - Attachment is obsolete: true
Attachment #649223 - Flags: review?(jaws)
Attachment #649223 - Flags: review?(jaws) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/3411e98278d6
Whiteboard: [Snappy] → [Snappy][fixed-in-fx-team]
(Assignee)

Comment 10

5 years ago
Pushed a little follow-up:

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

Comment 11

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3411e98278d6
https://hg.mozilla.org/mozilla-central/rev/54f9a8c340c8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.