Closed
Bug 779618
Opened 12 years ago
Closed 12 years ago
[Page Thumbnails] make tabbrowser use the thumbnail service
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 17
People
(Reporter: ttaubert, Assigned: ttaubert)
Details
(Whiteboard: [Snappy])
Attachments
(1 file, 2 obsolete files)
8.77 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
This patch introduces a PageThumbs.captureToCanvas() method which provides the API needed by the tabbrowser.
Attachment #648068 -
Flags: review?(jaws)
Comment 2•12 years ago
|
||
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•12 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.
Comment 4•12 years ago
|
||
(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•12 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•12 years ago
|
||
Reverted the signature change for capture().
Attachment #648068 -
Attachment is obsolete: true
Attachment #648068 -
Flags: review?(jaws)
Attachment #648648 -
Flags: review?(jaws)
Comment 7•12 years ago
|
||
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•12 years ago
|
||
(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)
Updated•12 years ago
|
Attachment #649223 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3411e98278d6
Whiteboard: [Snappy] → [Snappy][fixed-in-fx-team]
Assignee | ||
Comment 10•12 years ago
|
||
Pushed a little follow-up: https://hg.mozilla.org/integration/fx-team/rev/54f9a8c340c8
Assignee | ||
Comment 11•12 years ago
|
||
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.
Description
•