[Page Thumbnails] default thumbnail size should be 1/9th of the user's screen size

VERIFIED FIXED in Firefox 13

Status

()

Firefox
General
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: jaws)

Tracking

Trunk
Firefox 14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox13 verified, firefox14 verified, firefox15 verified)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment, 3 obsolete attachments)

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.
Isn't the number of tiles ever going to be customizable?
If it ever is, we can take that into account when doing it.
Duplicate of this bug: 736018
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. :)
Exactly :)

Comment 6

5 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.
Created attachment 612047 [details] [diff] [review]
WIP patch for bug

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)
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]
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

5 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

5 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
Whiteboard: [autoland-in-queue]
Assignee: ttaubert → jwein
Created attachment 612778 [details] [diff] [review]
Patch for bug
Attachment #612047 - Attachment is obsolete: true
Attachment #612778 - Flags: review?(ttaubert)
(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].
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)
Created attachment 613733 [details] [diff] [review]
Patch for bug v2

Added _getThumbnailSize function and kept the constants as fallbacks.
Attachment #612778 - Attachment is obsolete: true
Attachment #613733 - Flags: review?(ttaubert)
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)
You should be able to use nsIScreenManager instead of depending on the content window's screen property.
That's way better, didn't know it existed. nsIScreenManager is exactly what we want here.
Created attachment 614206 [details] [diff] [review]
Patch for bug v3

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)
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+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd7dbc2653a
status-firefox13: --- → affected
status-firefox14: --- → fixed
tracking-firefox13: --- → ?
Flags: in-testsuite-
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/1bd7dbc2653a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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

5 years ago
This makes a dramatic difference in the new feature and I think we should take it into aurora.
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d0f70b87fba
status-firefox13: affected → fixed
tracking-firefox13: ? → ---
Depends on: 749010
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.
Whiteboard: [qa+]
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-firefox13: fixed → verified
status-firefox14: fixed → verified
status-firefox15: --- → verified
You need to log in before you can comment on or make changes to this bug.