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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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

[Siddhartha Dugar [:sdrocking]]

Isn't the number of tiles ever going to be customizable?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

If it ever is, we can take that into account when doing it.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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. :)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Exactly :)

Comment 6

[Chris Lee [:clee]]

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

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 9

[Mozilla RelEng Bot]

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

[Mozilla RelEng Bot]

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/[e-mail]-c650f0b807ae

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(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 13

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v2

Added _getThumbnailSize function and kept the constants as fallbacks.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 16

[Dão Gottwald [:dao]]

You should be able to use nsIScreenManager instead of depending on the content window's screen property.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

That's way better, didn't know it existed. nsIScreenManager is exactly what we want here.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v3

Thanks Tim and Dao for your feedback. I've fixed the other uses of _thumbnailWidth/_thumbnailHeight and am now nsIScreenManager.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd7dbc2653a

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/1bd7dbc2653a

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 24

[Asa Dotzler [:asa]]

This makes a dramatic difference in the new feature and I think we should take it into aurora.

Comment 25

[Alex Keybl [:akeybl]]

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.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d0f70b87fba

Comment 27

[Matt Brubeck (:mbrubeck)]

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

[Virgil Dicu [:virgil] [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.