Closed
Bug 736028
Opened 13 years ago
Closed 13 years ago
[Page Thumbnails] default thumbnail size should be 1/9th of the user's screen size
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 14
People
(Reporter: ttaubert, Assigned: jaws)
References
Details
(Whiteboard: [qa+])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
ttaubert
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Thumbnails are currently captured at 400x225. They should rather be 1/9th of the user's screen size so that we (almost) never have to upscale them. Also, 1/9th should be the maximum size of sites/cells in the new tab page grid.
Comment 1•13 years ago
|
||
Isn't the number of tiles ever going to be customizable?
Reporter | ||
Comment 2•13 years ago
|
||
If it ever is, we can take that into account when doing it.
Assignee | ||
Comment 4•13 years ago
|
||
Tim: When saying 1/9th of screen size, are you indirectly saying that they will be 1/3 * height and 1/3 * width? If so, cool. Just wanted to make sure I understand the goal of this bug correctly. :)
Reporter | ||
Comment 5•13 years ago
|
||
Exactly :)
Comment 6•13 years ago
|
||
(In reply to Siddhartha Dugar [:sdrocking] from comment #1)
> Isn't the number of tiles ever going to be customizable?
For now, there are no specific plans to customize the number of tiles. Definitely something we can explore down the road.
Assignee | ||
Comment 7•13 years ago
|
||
What do you think about this Tim? I imagine there will are some tests that will need fixing up.
Also, if PageThumbs_createCanvas is called before PageThumbs_determineCropSize then the thumbnail height & width won't be set, so this patch isn't perfect yet.
Attachment #612047 -
Flags: feedback?(ttaubert)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland-try]
Updated•13 years ago
|
Whiteboard: [autoland-try] → [autoland-in-queue]
Reporter | ||
Comment 8•13 years ago
|
||
Comment on attachment 612047 [details] [diff] [review]
WIP patch for bug
Review of attachment 612047 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. Thanks for doing that!
::: browser/components/thumbnails/PageThumbs.jsm
@@ +159,5 @@
> let sw = aWindow.innerWidth;
> let sh = aWindow.innerHeight;
>
> + this._thumbnailWidth = this._thumbnailWidth || aWindow.screen.width / 3;
> + this._thumbnailHeight = this._thumbnailHeight || aWindow.screen.height / 3;
We should probably pass those values to Math.round() as they're also used for the canvas attributes.
I'd actually like to make this a lazy getter of PageThumbs but looks like we need the aWindow. Couldn't find a way to retrieve the screen size from JS without referencing a DOMWindow.
Attachment #612047 -
Flags: feedback?(ttaubert) → feedback+
Comment 9•13 years ago
|
||
Autoland Patchset:
Patches: 612047
Branch: mozilla-central => try
Destination: http://hg.mozilla.org/try/pushloghtml?changeset=c650f0b807ae
Try run started, revision c650f0b807ae. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=c650f0b807ae
Comment 10•13 years ago
|
||
Try run for c650f0b807ae is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=c650f0b807ae
Results (out of 15 total builds):
success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-c650f0b807ae
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland-in-queue]
Assignee | ||
Updated•13 years ago
|
Assignee: ttaubert → jwein
Assignee | ||
Comment 11•13 years ago
|
||
Attachment #612047 -
Attachment is obsolete: true
Attachment #612778 -
Flags: review?(ttaubert)
Reporter | ||
Comment 12•13 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #7)
> Also, if PageThumbs_createCanvas is called before
> PageThumbs_determineCropSize then the thumbnail height & width won't be set,
Didn't see this before but it's a valid concern even thought those methods are only part of the "private" API.
I think we should turn these properties into a method like ._getThumbnailSize(aWindow) which lazily determines the thumbnail size and always returns [width, height].
Reporter | ||
Comment 13•13 years ago
|
||
Comment on attachment 612778 [details] [diff] [review]
Patch for bug
Review of attachment 612778 [details] [diff] [review]:
-----------------------------------------------------------------
Please add a method _getThumbnailSize() like described in comment #12.
Attachment #612778 -
Flags: review?(ttaubert)
Assignee | ||
Comment 14•13 years ago
|
||
Added _getThumbnailSize function and kept the constants as fallbacks.
Attachment #612778 -
Attachment is obsolete: true
Attachment #613733 -
Flags: review?(ttaubert)
Reporter | ||
Comment 15•13 years ago
|
||
Comment on attachment 613733 [details] [diff] [review]
Patch for bug v2
Review of attachment 613733 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you, almost there :)
::: browser/components/thumbnails/PageThumbs.jsm
@@ +177,5 @@
> _determineCropSize: function PageThumbs_determineCropSize(aWindow) {
> let sw = aWindow.innerWidth;
> let sh = aWindow.innerHeight;
>
> + let thumbnailSize = this._getThumbnailSize(aWindow);
Nit: We could use destructuring assignment here:
let [thumbnailWidth, thumbnailHeight] = this._getThumbnailSize(aWindow);
@@ +183,5 @@
> let scaledWidth = sw * scale;
> let scaledHeight = sh * scale;
>
> + if (scaledHeight > this._thumbnailHeight)
> + sh -= Math.floor(Math.abs(scaledHeight - this._thumbnailHeight) * scale);
You're still accessing the property directly here. While this will work we shouldn't do it that way.
@@ +188,3 @@
>
> + if (scaledWidth > this._thumbnailWidth)
> + sw -= Math.floor(Math.abs(scaledWidth - this._thumbnailWidth) * scale);
Same here.
@@ +200,5 @@
> let doc = Services.appShell.hiddenDOMWindow.document;
> let canvas = doc.createElementNS(HTML_NAMESPACE, "canvas");
> canvas.mozOpaque = true;
> canvas.mozImageSmoothingEnabled = true;
> + let thumbnailSize = this._getThumbnailSize();
Nit: destructuring assignment.
Attachment #613733 -
Flags: review?(ttaubert)
Comment 16•13 years ago
|
||
You should be able to use nsIScreenManager instead of depending on the content window's screen property.
Reporter | ||
Comment 17•13 years ago
|
||
That's way better, didn't know it existed. nsIScreenManager is exactly what we want here.
Assignee | ||
Comment 18•13 years ago
|
||
Thanks Tim and Dao for your feedback. I've fixed the other uses of _thumbnailWidth/_thumbnailHeight and am now nsIScreenManager.
Attachment #613733 -
Attachment is obsolete: true
Attachment #614206 -
Flags: review?(ttaubert)
Reporter | ||
Comment 19•13 years ago
|
||
Comment on attachment 614206 [details] [diff] [review]
Patch for bug v3
Review of attachment 614206 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you, this looks great!
r=me with the question answered
::: browser/components/thumbnails/PageThumbs.jsm
@@ +219,5 @@
> + screenManager.primaryScreen.GetRect(left, top, width, height);
> + this._thumbnailWidth = Math.round(width.value / 3);
> + this._thumbnailHeight = Math.round(height.value / 3);
> + }
> + if (this._thumbnailWidth && this._thumbnailHeight)
Do we still need this check and the default values? Looks like the screen manager should be infallible and _thumbnailWidth/Height never equal to zero.
Attachment #614206 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 20•13 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #19)
> Do we still need this check and the default values? Looks like the screen
> manager should be infallible and _thumbnailWidth/Height never equal to zero.
No, I guess we don't need this check. Thanks for noticing that. I'll remove the constants and the check.
Assignee | ||
Comment 21•13 years ago
|
||
status-firefox13:
--- → affected
status-firefox14:
--- → fixed
tracking-firefox13:
--- → ?
Flags: in-testsuite-
Target Milestone: --- → Firefox 14
Comment 22•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 23•13 years ago
|
||
Comment on attachment 614206 [details] [diff] [review]
Patch for bug v3
I would like to get this patch landed on Aurora since the new tab page is a new feature for Firefox 13.
[Approval Request Comment]
Regression caused by (bug #): new feature introduced by bug 729878
User impact if declined: lower quality thumbnails on new tab page
Testing completed (on m-c, etc.): landed on m-c in 4-12
Risk to taking this patch (and alternatives if risky): none anticipated
String changes made by this patch: none
Attachment #614206 -
Flags: approval-mozilla-aurora?
Comment 24•13 years ago
|
||
This makes a dramatic difference in the new feature and I think we should take it into aurora.
Comment 25•13 years ago
|
||
Comment on attachment 614206 [details] [diff] [review]
Patch for bug v3
[Triage Comment]
Low risk and in support of a new FF13 feature. Approved for Aurora 13.
Attachment #614206 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 26•13 years ago
|
||
tracking-firefox13:
? → ---
Comment 27•13 years ago
|
||
This caused a perma-orange crash in Fx13 when it landed on Aurora (bug 749010). We might need to back it out of Fx13 on beta.
Comment 28•13 years ago
|
||
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120530 Firefox/14.0a2
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/15.0 Firefox/15.0a1
Setting to Verified across platforms. bug 749010 tracked separately.Thumbnails are captured at a higher resolution compared to F12.
Status: RESOLVED → VERIFIED
status-firefox15:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•