options image grid: Make sure the images are requested with the appropriate scale.

options image grid: Make sure the images are requested with the appropriate scale.

BUG=141146
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151954

[sadrul, 2012-08-15 20:31:18]

I have tested this with the userimage-picker in the settings page and login
page, and the wallpaper picker in the settings page, and this seems to work OK
(with http://codereview.chromium.org/10857019/ for wallpaper picker). But I am
not sure if this is the right way of doing this (e.g. do we need to check
whether the url already has @Xx set?)

[Rick Byers, 2012-08-15 20:40:01]

On 2012/08/15 20:31:18, sadrul wrote:
> I have tested this with the userimage-picker in the settings page and login
> page, and the wallpaper picker in the settings page, and this seems to work OK
> (with http://codereview.chromium.org/10857019/ for wallpaper picker). But I am
> not sure if this is the right way of doing this (e.g. do we need to check
> whether the url already has @Xx set?)

I haven't been following in detail what they've done about high-DPI profile
images.  This should probably be consistent with the login screen though. 
+nkostylev@ who is the expert here.

[sadrul, 2012-08-15 22:35:29]

nkostylev@ seems to be on vacation. Perhaps ivankr@ would know?

[sadrul, 2012-08-16 06:47:15]

+ivankr@

[Ivan Korotkov, 2012-08-16 08:31:50]

There is no actual need for 2x images in the grid since they are displayed as
tiny thumbnails (less than half of 1x image size) and this would only increase
loading time, so I suggest abandoning the CL.

The only image that really needs @factor is the full-sized preview image. I have
some WIP to do so and other DPI-related fixes for user images grid.

http://codereview.chromium.org/10830341/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

http://codereview.chromium.org/10830341/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/user_images_grid.js:67: imageEl.src =
this.dataItem.url + '@' + window.devicePixelRatio + 'x';
This wouldn't work with all URLs in this grid (there are data: URLs that don't
support @<factor> scheme.

[Ivan Korotkov, 2012-08-16 10:44:10]

On second thought, this makes sense for wallpaper thumbnails, if they're always
served of the minimum required size.

Okay, let's submit this then and the preview images will have proper DPI
automatically. Please add a check that URL has chrome:// scheme, though, and
verify this with user images when user has a custom image from file.

LGTM

[sadrul, 2012-08-16 16:49:28]

Added the check to make sure it appends the scale only for chrome:// urls

[Ivan Korotkov, 2012-08-16 17:47:53]

http://codereview.chromium.org/10830341/diff/4002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

http://codereview.chromium.org/10830341/diff/4002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/user_images_grid.js:68: if
(this.dataItem.url.slice(0, scheme.length) == scheme)
Optional: using a regexp or url.indexOf(scheme) == 0 is somewhat slower but more
clear.

[sadrul, 2012-08-16 19:36:16]

Rick: Can I get an owners stamp?

http://codereview.chromium.org/10830341/diff/4002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

http://codereview.chromium.org/10830341/diff/4002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/user_images_grid.js:68: if
(this.dataItem.url.slice(0, scheme.length) == scheme)
On 2012/08/16 17:47:53, Ivan Korotkov wrote:
> Optional: using a regexp or url.indexOf(scheme) == 0 is somewhat slower but
more
> clear. 

I considered using indexOf at first. But since this could be datauri, perhaps it
makes sense to go with slice (thanks for flackr@)

[Rick Byers, 2012-08-16 20:09:05]

lgtm

[Ivan Korotkov, 2012-08-17 08:01:43]

http://codereview.chromium.org/10830341/diff/4002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

http://codereview.chromium.org/10830341/diff/4002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/user_images_grid.js:68: if
(this.dataItem.url.slice(0, scheme.length) == scheme)
On 2012/08/16 19:36:16, sadrul wrote:
> On 2012/08/16 17:47:53, Ivan Korotkov wrote:
> > Optional: using a regexp or url.indexOf(scheme) == 0 is somewhat slower but
> more
> > clear. 
> 
> I considered using indexOf at first. But since this could be datauri, perhaps
it
> makes sense to go with slice (thanks for flackr@)

Right, makes sense.