Closed Bug 908060 Opened 11 years ago Closed 11 years ago

Choose a better size for the background thumbnail service's browser

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
(In reply to Thomas Stache from bug 879546 comment #7)
> According to bug 870105 the [background] thumbnail service uses the display
> screen size for the [size of the thumbnail browser], which looks strange on
> a 24" or 27" desktop LCD, especially when Firefox is actually used in a much
> smaller windowed size.

Tim suggests using a fixed size in bug 870105 comment 8, and I think that's a valid choice because it should always produce legible thumbnails.

I'd like to try something a little different first (well, second): find the biggest browser window that the user has opened, and use that size.  That way the thumbnail contents should be layed out at a scale that looks familiar to the user.  It's especially true for people who only ever use a single window and people who use multiple maximized windows.  Bound the size to a minimum of 1064x768 to avoid pathologically small or misshapen cases.
Attachment #793855 - Flags: review?(mhammond)
Comment on attachment 793855 [details] [diff] [review]
patch

Review of attachment 793855 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks fine (although I wonder if the "min size" calculation should attempt to keep the same aspect ratio as the window that we chose to base the size on?), but I'm concerned with the general idea - doesn't this mean some users will end up with multiple aspect ratios, apparently at random?

(In reply to Thomas Stache from bug 879546 comment #7)
> According to bug 870105 the [background] thumbnail service uses the display
> screen size for the [size of the thumbnail browser], which looks strange on
> a 24" or 27" desktop LCD, especially when Firefox is actually used in a much
> smaller windowed size.

I'm not sure how the size of the "desktop LCD" matters - but I do see how the aspect ratio of the Fx window versus the aspect ratio of the screen.  But let's say the user spends part the time with Fx maximized and part of the time with it taking 1/2 the width of the screen - such a user would end up with the 2 aspect ratios stored.  People who use a variety of sizes depending on the work they are doing may end up with the corresponding variety of aspect ratios of thumbnails.  I'm not sure that's good.  If we could somehow determine the most common size used by the user I'd be more comfortable.

[I guess the same basic problem would exist with users who use multiple screens of different sizes and regularly switch which one Fx is on, but that's even more of an edge case]

So declining review at this stage as I think the approach needs more discussion.
Attachment #793855 - Flags: review?(mhammond)
OK.  I'll note that the foreground service can draw with different aspect ratios, too, but it's true that we can do better since we choose the size of our browser.

If you capture google.com in a big browser and a small browser and view the resulting thumbnails at the same physical size (e.g., measured in centimeters), the thumbnail captured in the small browser will look better: the logo will be bigger and fill more of the thumbnail, and there won't be as much negative space.  Maybe Thomas didn't actually do a comparison like that, but if you have a high resolution screen and the thumbnail browser is therefore big, you might still think your relatively small thumbnail is ugly with all this negative space and tiny logo.  It's subjective I guess, and it's probably especially true for you if you never view google.com in a maximized window, although at some physical size things do become illegible.

This patch uses the same aspect ratio as the canvases used for drawing (which is the same as the screen's), but uses a fixed width of min(1024, screenWidth).  This way the image will fill 100% of the canvas, the "scale" of the image won't change across different captures of the same URL, and at a width of 1024 the image ought to be very legible.  The downside is that the scale might be different from what the user is used to seeing when he visits the page (e.g., he visits google.com in a maximized window on his 2880x1800 screen), but I think this is a good trade-off for thumbnails.
Attachment #793855 - Attachment is obsolete: true
Attachment #794288 - Flags: review?(mhammond)
Comment on attachment 794288 [details] [diff] [review]
fixed aspect ratio + fixed width

Review of attachment 794288 [details] [diff] [review]:
-----------------------------------------------------------------

> OK.  I'll note that the foreground service can draw with different
> aspect ratios, too

It's likely I'm missing something here, but that's not clear to me.  If I'm reading things correctly, the foreground service bases the canvas size on the screen, and the crop size and scale on the window.  So it looks to me like the aspect will always be the same with the cropping causing parts of the window to be clipped (which is what I see here).

It seems the background service is doing the same thing - the *canvas* size is fixed based on the screen, and the sizing of the bg browser will only determine how it is cropped.  So it almost seems as though the b/g service should size the browser such that it matches the (fixed) size of the resulting canvas (ie, such that _determineCropSize() will indicate no cropping or scaling) and that any tweaks we want to make to aspect ratios etc should be in _getThumbnailSize()

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +145,5 @@
>      browser.setAttribute("type", "content");
>      browser.setAttribute("remote", "true");
>      browser.setAttribute("privatebrowsing", "true");
>  
> +    // Size the browser.  Make its aspect ratio the same as the canvases' that

apostrophe nit :)  (This is just plural rather than possessive, right?)

@@ +146,5 @@
>      browser.setAttribute("remote", "true");
>      browser.setAttribute("privatebrowsing", "true");
>  
> +    // Size the browser.  Make its aspect ratio the same as the canvases' that
> +    // the thumbnails are drawn into; the canvases' aspect ratio is the same as

but I guess this one is possessive, so is ok :)
We may be talking about two different things.  I'm talking about the width-over-height ratio of the resulting cropped image drawn into the thumbnail rectangle by the fg service.  (If you make the browser really tall and narrow or short and wide, the fg draws a cropped image within the thumbnail rectangle.)  The thumbnail rectangle aspect ratio is fixed, but the image drawn inside it isn't.

I think we can agree that bg thumbs shouldn't be cropped at all, meaning they should fill the entire canvas.  Fortunately we can make that happen because we can choose our browser size, unlike the fg service.  I may be misunderstanding you, but this patch should do that: it picks a fixed width and computes a height so that the resulting aspect ratio matches the canvases'.

> So it almost seems as though the b/g service should size the browser such
> that it matches the (fixed) size of the resulting canvas

By matches you mean equal to?  The browser would be way too small.  If your screen has a resolution of 1024x768, the browser would be 341x256, but you'd still be loading and drawing full-sized pages in there (of course).  Load google.com in there and you'd only see white.

> > +    // Size the browser.  Make its aspect ratio the same as the canvases' that
> 
> apostrophe nit :)  (This is just plural rather than possessive, right?)

:-D  Possessive: "Make its aspect ratio the same as the canvases' [aspect ratio]."  An aspect ratio can't be the same as a canvas.
Comment on attachment 794288 [details] [diff] [review]
fixed aspect ratio + fixed width

Resolved over IRC - I think this is good!
Attachment #794288 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/2d0189c228ca
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: