Last Comment Bug 736028 - [Page Thumbnails] default thumbnail size should be 1/9th of the user's screen size
: [Page Thumbnails] default thumbnail size should be 1/9th of the user's screen...
Status: VERIFIED FIXED
[qa+]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
: 736018 (view as bug list)
Depends on: 749010
Blocks: 729878
  Show dependency treegraph
 
Reported: 2012-03-15 03:28 PDT by Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
Modified: 2012-05-30 08:00 PDT (History)
13 users (show)
jaws: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified


Attachments
WIP patch for bug (3.57 KB, patch)
2012-04-03 17:50 PDT, Jared Wein [:jaws] (please needinfo? me)
ttaubert: feedback+
Details | Diff | Splinter Review
Patch for bug (3.59 KB, patch)
2012-04-05 19:03 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v2 (4.09 KB, patch)
2012-04-10 13:19 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v3 (4.30 KB, patch)
2012-04-11 15:53 PDT, Jared Wein [:jaws] (please needinfo? me)
ttaubert: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-15 03:28:55 PDT
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] 2012-03-15 03:31:44 PDT
Isn't the number of tiles ever going to be customizable?
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-15 03:47:39 PDT
If it ever is, we can take that into account when doing it.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-03-15 12:32:55 PDT
*** Bug 736018 has been marked as a duplicate of this bug. ***
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-03-15 16:22:35 PDT
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] (on PTO, back Aug 29th) 2012-03-16 00:59:35 PDT
Exactly :)
Comment 6 Chris Lee [:clee] 2012-03-16 10:15:18 PDT
(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) 2012-04-03 17:50:58 PDT
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.
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-04-05 04:16:10 PDT
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 2012-04-05 07:22:03 PDT
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 2012-04-05 11:47:15 PDT
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
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-04-05 19:03:30 PDT
Created attachment 612778 [details] [diff] [review]
Patch for bug
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-04-10 06:31:25 PDT
(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] (on PTO, back Aug 29th) 2012-04-10 06:32:42 PDT
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) 2012-04-10 13:19:24 PDT
Created attachment 613733 [details] [diff] [review]
Patch for bug v2

Added _getThumbnailSize function and kept the constants as fallbacks.
Comment 15 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-04-11 02:12:09 PDT
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] 2012-04-11 02:23:31 PDT
You should be able to use nsIScreenManager instead of depending on the content window's screen property.
Comment 17 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-04-11 02:26:47 PDT
That's way better, didn't know it existed. nsIScreenManager is exactly what we want here.
Comment 18 Jared Wein [:jaws] (please needinfo? me) 2012-04-11 15:53:57 PDT
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.
Comment 19 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-04-11 23:49:06 PDT
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) 2012-04-11 23:57:33 PDT
(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) 2012-04-12 00:04:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd7dbc2653a
Comment 22 :Ms2ger (⌚ UTC+1/+2) 2012-04-12 10:19:46 PDT
https://hg.mozilla.org/mozilla-central/rev/1bd7dbc2653a
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2012-04-13 22:24:05 PDT
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] 2012-04-13 23:15:35 PDT
This makes a dramatic difference in the new feature and I think we should take it into aurora.
Comment 25 Alex Keybl [:akeybl] 2012-04-16 13:28:42 PDT
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) 2012-04-16 13:45:16 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d0f70b87fba
Comment 27 Matt Brubeck (:mbrubeck) 2012-04-25 16:04:30 PDT
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] 2012-05-30 08:00:33 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.