Fix chrome.downloads.getFileIcon for hi-dpi and clarify docs.

Fix chrome.downloads.getFileIcon for hi-dpi and clarify docs.

BUG=334358
TEST:
On a Pixel, install a downloads extension such as this.
https://chrome.google.com/webstore/detail/download-manager-button/jglgknilkoejnnjngnmoobbhdckpdmem
Either right-click on the browser action button and Inspect Popup, or open the background page console.
If there are no downloads, download a file so that there's a file icon to inspect. One easy way to download a file is to open this data url and click the link:
data:text/html,<a href="data:application/octet-stream," download=test-137043005.txt>download an empty file
Copy this javascript into the Developer Tools console:
chrome.downloads.search({limit:1}, function(items) {matchMedia('screen and (-webkit-min-device-pixel-ratio: 1.5)').addListener(function() {chrome.downloads.getFileIcon(items[0].id, function(icon) {console.log('dpr', devicePixelRatio, icon);});});});
This javascript finds a DownloadItem and listens for the devicePixelRatio to change, then fetches the file icon for the new devicePixelRatio.
Type Ctrl+Shift+- and Ctrl+Shift+= to decrease and increase the screen resolution until the console logs at least one line that begins with 'dpr 1' and another line that begins with 'dpr 2'. You might need to increase and/or decrease the resolution several times. The rest of the 'dpr 1' line should be different from the rest of the 'dpr 2' line. If the rest of those lines are the same, then this fix is broken.

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

[benjhayden, 2014-01-14 19:09:41]

Asanka, can you review my use of gfx::Image/SkBitmap or point me to somebody
else?

Kalman: downloads.idl

[not at google - send to devlin, 2014-01-14 19:16:23]

lgtm for downloads.idl

https://codereview.chromium.org/137043005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/137043005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:384: // pixels. Valid values are 16
and 32. No other sizes are supported.
just "The only supported sizes are 16 and 32, anything else is an error"? or
similar.

[asanka, 2014-01-14 19:20:24]

The change makes sense, but someone more familiar with high dpi issues should
review.

+oshima: Could you have a look?

Ben: Could you add a BUG= line? Also I'm assuming this doesn't affect native UI?

[benjhayden, 2014-01-14 19:36:02]

On 2014/01/14 19:20:24, asanka wrote:
> The change makes sense, but someone more familiar with high dpi issues should
> review.
> 
> +oshima: Could you have a look?
> 
> Ben: Could you add a BUG= line? Also I'm assuming this doesn't affect native
UI?

Correct, this only affects chrome.downloads.getFileIcon().
By contrast, chrome://downloads uses chrome://fileicon, which is FileIconSource
in chrome/browser/ui/webui. My changes in downloads_api.cc are based on how that
class uses GetRepresentation.

[oshima, 2014-01-14 19:44:46]

On 2014/01/14 19:20:24, asanka wrote:
> The change makes sense, but someone more familiar with high dpi issues should
> review.
> 
> +oshima: Could you have a look?
> 
> Ben: Could you add a BUG= line?
Yes please. I want to understand the context and platforms it is affected (I
assume all)

> Also I'm assuming this doesn't affect native UI?

This isn't quite right because DSF is dynamic (at least on mac and cros), and
may change on the fly.

The right way to handle is to use css dataset
(http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/), and generate the data url
that as inlined images for all supported scale factors.

[benjhayden, 2014-01-14 21:25:32]

On 2014/01/14 19:44:46, oshima wrote:
> On 2014/01/14 19:20:24, asanka wrote:
> > The change makes sense, but someone more familiar with high dpi issues
should
> > review.
> > 
> > +oshima: Could you have a look?
> > 
> > Ben: Could you add a BUG= line?
> Yes please. I want to understand the context and platforms it is affected (I
> assume all)

We had a complaint that the icons looked a little blurry on a retina macbook,
asking for the ability to generate icons larger than 32x32.
When I dug in, I noticed that it's not possible to generate icons larger than
32x32, but getFileIcon wasn't handling devicePixelRatio.

> 
> > Also I'm assuming this doesn't affect native UI?
> 
> This isn't quite right because DSF is dynamic (at least on mac and cros), and
> may change on the fly.
> 
> The right way to handle is to use css dataset
> (http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/), and generate the data
url
> that as inlined images for all supported scale factors.

I think it's probably too much to expect extension authors to call getFileIcon()
for every possible devicePixelRatio and pre-generate a complete srcset for each
DownloadItem. Especially since chrome://downloads just uses
img.src=chrome://fileicon without a srcset. These icons are not served and
cached, they're generated on the client on demand.
I imagine that devicePixelRatio doesn't change very often, and when it does,
either the extension can call getFileIcon() again or the user can refresh the
tab or browser action popup bubble.
Let's just fix getFileIcon for scale=devicePixelRatio and not worry about
automatically selecting different icons when devicePixelRatio changes.
If users ask for a way to pre-generate different scale icons, then I'll just
make getFileIcon take a scale option alongside the size option.

[oshima, 2014-01-14 21:39:47]

On 2014/01/14 21:25:32, benjhayden_chromium wrote:
> On 2014/01/14 19:44:46, oshima wrote:
> > On 2014/01/14 19:20:24, asanka wrote:
> > > The change makes sense, but someone more familiar with high dpi issues
> should
> > > review.
> > > 
> > > +oshima: Could you have a look?
> > > 
> > > Ben: Could you add a BUG= line?
> > Yes please. I want to understand the context and platforms it is affected (I
> > assume all)
> 
> We had a complaint that the icons looked a little blurry on a retina macbook,
> asking for the ability to generate icons larger than 32x32.
> When I dug in, I noticed that it's not possible to generate icons larger than
> 32x32, but getFileIcon wasn't handling devicePixelRatio.
> 
> > 
> > > Also I'm assuming this doesn't affect native UI?
> > 
> > This isn't quite right because DSF is dynamic (at least on mac and cros),
and
> > may change on the fly.
> > 
> > The right way to handle is to use css dataset
> > (http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/), and generate the data
> url
> > that as inlined images for all supported scale factors.
> 
> I think it's probably too much to expect extension authors to call
getFileIcon()
> for every possible devicePixelRatio and pre-generate a complete srcset for
each
> DownloadItem. Especially since chrome://downloads just uses
> img.src=chrome://fileicon without a srcset. These icons are not served and
> cached, they're generated on the client on demand.
> I imagine that devicePixelRatio doesn't change very often, and when it does,

It can happen often on Pixel with external display because they will most likely
have
different scale factor.

> either the extension can call getFileIcon() again or the user can refresh the
> tab or browser action popup bubble.
> Let's just fix getFileIcon for scale=devicePixelRatio and not worry about
> automatically selecting different icons when devicePixelRatio changes.
> If users ask for a way to pre-generate different scale icons, then I'll just
> make getFileIcon take a scale option alongside the size option.

I want to know a bit more background. Are you saying this isn't just for
chrome://downloads?

[benjhayden, 2014-01-14 22:02:23]

On 2014/01/14 21:39:47, oshima wrote:
> On 2014/01/14 21:25:32, benjhayden_chromium wrote:
> > On 2014/01/14 19:44:46, oshima wrote:
> > > On 2014/01/14 19:20:24, asanka wrote:
> > > > The change makes sense, but someone more familiar with high dpi issues
> > should
> > > > review.
> > > > 
> > > > +oshima: Could you have a look?
> > > > 
> > > > Ben: Could you add a BUG= line?
> > > Yes please. I want to understand the context and platforms it is affected
(I
> > > assume all)
> > 
> > We had a complaint that the icons looked a little blurry on a retina
macbook,
> > asking for the ability to generate icons larger than 32x32.
> > When I dug in, I noticed that it's not possible to generate icons larger
than
> > 32x32, but getFileIcon wasn't handling devicePixelRatio.
> > 
> > > 
> > > > Also I'm assuming this doesn't affect native UI?
> > > 
> > > This isn't quite right because DSF is dynamic (at least on mac and cros),
> and
> > > may change on the fly.
> > > 
> > > The right way to handle is to use css dataset
> > > (http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/), and generate the
data
> > url
> > > that as inlined images for all supported scale factors.
> > 
> > I think it's probably too much to expect extension authors to call
> getFileIcon()
> > for every possible devicePixelRatio and pre-generate a complete srcset for
> each
> > DownloadItem. Especially since chrome://downloads just uses
> > img.src=chrome://fileicon without a srcset. These icons are not served and
> > cached, they're generated on the client on demand.
> > I imagine that devicePixelRatio doesn't change very often, and when it does,
> 
> It can happen often on Pixel with external display because they will most
likely
> have
> different scale factor.
> 
> > either the extension can call getFileIcon() again or the user can refresh
the
> > tab or browser action popup bubble.
> > Let's just fix getFileIcon for scale=devicePixelRatio and not worry about
> > automatically selecting different icons when devicePixelRatio changes.
> > If users ask for a way to pre-generate different scale icons, then I'll just
> > make getFileIcon take a scale option alongside the size option.
> 
> I want to know a bit more background. Are you saying this isn't just for
> chrome://downloads?

getFileIcon isn't for chrome://downloads at all. It's just for extensions.
chrome://downloads is webui, not an extension.
http://developer.chrome.com/extensions/downloads.html#method-getFileIcon
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
https://chrome.google.com/webstore/detail/download-manager-button/jglgknilkoe...
The file icons aren't in the screenshots for that extension because
http://crbug.com/102211. I should take new screenshots now that I installed
gnome-icon-theme-full and they're showing up again.

[oshima, 2014-01-14 22:15:52]

On 2014/01/14 22:02:23, benjhayden_chromium wrote:
> On 2014/01/14 21:39:47, oshima wrote:
> > On 2014/01/14 21:25:32, benjhayden_chromium wrote:
> > > On 2014/01/14 19:44:46, oshima wrote:
> > > > On 2014/01/14 19:20:24, asanka wrote:
> > > > > The change makes sense, but someone more familiar with high dpi issues
> > > should
> > > > > review.
> > > > > 
> > > > > +oshima: Could you have a look?
> > > > > 
> > > > > Ben: Could you add a BUG= line?
> > > > Yes please. I want to understand the context and platforms it is
affected
> (I
> > > > assume all)
> > > 
> > > We had a complaint that the icons looked a little blurry on a retina
> macbook,
> > > asking for the ability to generate icons larger than 32x32.
> > > When I dug in, I noticed that it's not possible to generate icons larger
> than
> > > 32x32, but getFileIcon wasn't handling devicePixelRatio.
> > > 
> > > > 
> > > > > Also I'm assuming this doesn't affect native UI?
> > > > 
> > > > This isn't quite right because DSF is dynamic (at least on mac and
cros),
> > and
> > > > may change on the fly.
> > > > 
> > > > The right way to handle is to use css dataset
> > > > (http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/), and generate the
> data
> > > url
> > > > that as inlined images for all supported scale factors.
> > > 
> > > I think it's probably too much to expect extension authors to call
> > getFileIcon()
> > > for every possible devicePixelRatio and pre-generate a complete srcset for
> > each
> > > DownloadItem. Especially since chrome://downloads just uses
> > > img.src=chrome://fileicon without a srcset. These icons are not served and
> > > cached, they're generated on the client on demand.
> > > I imagine that devicePixelRatio doesn't change very often, and when it
does,
> > 
> > It can happen often on Pixel with external display because they will most
> likely
> > have
> > different scale factor.
> > 
> > > either the extension can call getFileIcon() again or the user can refresh
> the
> > > tab or browser action popup bubble.
> > > Let's just fix getFileIcon for scale=devicePixelRatio and not worry about
> > > automatically selecting different icons when devicePixelRatio changes.
> > > If users ask for a way to pre-generate different scale icons, then I'll
just
> > > make getFileIcon take a scale option alongside the size option.
> > 
> > I want to know a bit more background. Are you saying this isn't just for
> > chrome://downloads?
> 
> getFileIcon isn't for chrome://downloads at all. It's just for extensions.
> chrome://downloads is webui, not an extension.
> http://developer.chrome.com/extensions/downloads.html#method-getFileIcon
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
>
https://chrome.google.com/webstore/detail/download-manager-button/jglgknilkoe...
> The file icons aren't in the screenshots for that extension because
> http://crbug.com/102211. I should take new screenshots now that I installed
> gnome-icon-theme-full and they're showing up again.

Ok, thank you for clarification. lgtm for now, but I just want to repeat that
DSF change isn't uncommon on Pixel (and win8.1 does support DPI change).
This will be more and more important as we have more high DPI devices/displays,
so you better have a solution in near future.

[benjhayden, 2014-01-14 22:44:26]

On 2014/01/14 22:15:52, oshima wrote:
> On 2014/01/14 22:02:23, benjhayden_chromium wrote:
> > On 2014/01/14 21:39:47, oshima wrote:
> > > On 2014/01/14 21:25:32, benjhayden_chromium wrote:
> > > > On 2014/01/14 19:44:46, oshima wrote:
> > > > > On 2014/01/14 19:20:24, asanka wrote:
> > > > > > The change makes sense, but someone more familiar with high dpi
issues
> > > > should
> > > > > > review.
> > > > > > 
> > > > > > +oshima: Could you have a look?
> > > > > > 
> > > > > > Ben: Could you add a BUG= line?
> > > > > Yes please. I want to understand the context and platforms it is
> affected
> > (I
> > > > > assume all)
> > > > 
> > > > We had a complaint that the icons looked a little blurry on a retina
> > macbook,
> > > > asking for the ability to generate icons larger than 32x32.
> > > > When I dug in, I noticed that it's not possible to generate icons larger
> > than
> > > > 32x32, but getFileIcon wasn't handling devicePixelRatio.
> > > > 
> > > > > 
> > > > > > Also I'm assuming this doesn't affect native UI?
> > > > > 
> > > > > This isn't quite right because DSF is dynamic (at least on mac and
> cros),
> > > and
> > > > > may change on the fly.
> > > > > 
> > > > > The right way to handle is to use css dataset
> > > > > (http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/), and generate
the
> > data
> > > > url
> > > > > that as inlined images for all supported scale factors.
> > > > 
> > > > I think it's probably too much to expect extension authors to call
> > > getFileIcon()
> > > > for every possible devicePixelRatio and pre-generate a complete srcset
for
> > > each
> > > > DownloadItem. Especially since chrome://downloads just uses
> > > > img.src=chrome://fileicon without a srcset. These icons are not served
and
> > > > cached, they're generated on the client on demand.
> > > > I imagine that devicePixelRatio doesn't change very often, and when it
> does,
> > > 
> > > It can happen often on Pixel with external display because they will most
> > likely
> > > have
> > > different scale factor.
> > > 
> > > > either the extension can call getFileIcon() again or the user can
refresh
> > the
> > > > tab or browser action popup bubble.
> > > > Let's just fix getFileIcon for scale=devicePixelRatio and not worry
about
> > > > automatically selecting different icons when devicePixelRatio changes.
> > > > If users ask for a way to pre-generate different scale icons, then I'll
> just
> > > > make getFileIcon take a scale option alongside the size option.
> > > 
> > > I want to know a bit more background. Are you saying this isn't just for
> > > chrome://downloads?
> > 
> > getFileIcon isn't for chrome://downloads at all. It's just for extensions.
> > chrome://downloads is webui, not an extension.
> > http://developer.chrome.com/extensions/downloads.html#method-getFileIcon
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
> >
>
https://chrome.google.com/webstore/detail/download-manager-button/jglgknilkoe...
> > The file icons aren't in the screenshots for that extension because
> > http://crbug.com/102211. I should take new screenshots now that I installed
> > gnome-icon-theme-full and they're showing up again.
> 
> Ok, thank you for clarification. lgtm for now, but I just want to repeat that
> DSF change isn't uncommon on Pixel (and win8.1 does support DPI change).
> This will be more and more important as we have more high DPI
devices/displays,
> so you better have a solution in near future.

http://crbug.com/123694
Extensions can use matchMedia('screen and (-webkit-min-device-pixel-ratio:
1.5)').addListener(handler) to detect when devicePixelRatio changes and call
getFileIcon, which will see the new devicePixelRatio and return the new icon
data url. This way, they don't need to call getFileIcon for ratios that might
never be used, and the getFileIcon function doesn't really need to take a scale
option. They could store both the 1x and 2x icons in srcset if they want, or
they can call getFileIcon again if the ratio changes again if they want to keep
their page light.

chrome://downloads currently does not respond to devicePixelRatio changes.
Perhaps it should use that matchMedia listener?

[oshima, 2014-01-14 22:50:19]

On 2014/01/14 22:44:26, benjhayden_chromium wrote:
> On 2014/01/14 22:15:52, oshima wrote:
> > On 2014/01/14 22:02:23, benjhayden_chromium wrote:
> > > On 2014/01/14 21:39:47, oshima wrote:
> > > > On 2014/01/14 21:25:32, benjhayden_chromium wrote:
> > > > > On 2014/01/14 19:44:46, oshima wrote:
> > > > > > On 2014/01/14 19:20:24, asanka wrote:
> > > > > > > The change makes sense, but someone more familiar with high dpi
> issues
> > > > > should
> > > > > > > review.
> > > > > > > 
> > > > > > > +oshima: Could you have a look?
> > > > > > > 
> > > > > > > Ben: Could you add a BUG= line?
> > > > > > Yes please. I want to understand the context and platforms it is
> > affected
> > > (I
> > > > > > assume all)
> > > > > 
> > > > > We had a complaint that the icons looked a little blurry on a retina
> > > macbook,
> > > > > asking for the ability to generate icons larger than 32x32.
> > > > > When I dug in, I noticed that it's not possible to generate icons
larger
> > > than
> > > > > 32x32, but getFileIcon wasn't handling devicePixelRatio.
> > > > > 
> > > > > > 
> > > > > > > Also I'm assuming this doesn't affect native UI?
> > > > > > 
> > > > > > This isn't quite right because DSF is dynamic (at least on mac and
> > cros),
> > > > and
> > > > > > may change on the fly.
> > > > > > 
> > > > > > The right way to handle is to use css dataset
> > > > > > (http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/), and generate
> the
> > > data
> > > > > url
> > > > > > that as inlined images for all supported scale factors.
> > > > > 
> > > > > I think it's probably too much to expect extension authors to call
> > > > getFileIcon()
> > > > > for every possible devicePixelRatio and pre-generate a complete srcset
> for
> > > > each
> > > > > DownloadItem. Especially since chrome://downloads just uses
> > > > > img.src=chrome://fileicon without a srcset. These icons are not served
> and
> > > > > cached, they're generated on the client on demand.
> > > > > I imagine that devicePixelRatio doesn't change very often, and when it
> > does,
> > > > 
> > > > It can happen often on Pixel with external display because they will
most
> > > likely
> > > > have
> > > > different scale factor.
> > > > 
> > > > > either the extension can call getFileIcon() again or the user can
> refresh
> > > the
> > > > > tab or browser action popup bubble.
> > > > > Let's just fix getFileIcon for scale=devicePixelRatio and not worry
> about
> > > > > automatically selecting different icons when devicePixelRatio changes.
> > > > > If users ask for a way to pre-generate different scale icons, then
I'll
> > just
> > > > > make getFileIcon take a scale option alongside the size option.
> > > > 
> > > > I want to know a bit more background. Are you saying this isn't just for
> > > > chrome://downloads?
> > > 
> > > getFileIcon isn't for chrome://downloads at all. It's just for extensions.
> > > chrome://downloads is webui, not an extension.
> > > http://developer.chrome.com/extensions/downloads.html#method-getFileIcon
> > >
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
> > >
> >
>
https://chrome.google.com/webstore/detail/download-manager-button/jglgknilkoe...
> > > The file icons aren't in the screenshots for that extension because
> > > http://crbug.com/102211. I should take new screenshots now that I
installed
> > > gnome-icon-theme-full and they're showing up again.
> > 
> > Ok, thank you for clarification. lgtm for now, but I just want to repeat
that
> > DSF change isn't uncommon on Pixel (and win8.1 does support DPI change).
> > This will be more and more important as we have more high DPI
> devices/displays,
> > so you better have a solution in near future.
> 
> http://crbug.com/123694
> Extensions can use matchMedia('screen and (-webkit-min-device-pixel-ratio:
> 1.5)').addListener(handler) to detect when devicePixelRatio changes and call
> getFileIcon, which will see the new devicePixelRatio and return the new icon
> data url. This way, they don't need to call getFileIcon for ratios that might
> never be used, and the getFileIcon function doesn't really need to take a
scale
> option. They could store both the 1x and 2x icons in srcset if they want, or
> they can call getFileIcon again if the ratio changes again if they want to
keep
> their page light.

Thank you for the info. That's informative.
> 
> chrome://downloads currently does not respond to devicePixelRatio changes.
> Perhaps it should use that matchMedia listener?

I believe chrome//downloads already solved this issue. I agree that it sucks if
you have to pregenerate and we were trying to avoid that.

[benjhayden, 2014-01-14 22:58:11]

On 2014/01/14 22:50:19, oshima wrote:
> On 2014/01/14 22:44:26, benjhayden_chromium wrote:
> > On 2014/01/14 22:15:52, oshima wrote:
> > > On 2014/01/14 22:02:23, benjhayden_chromium wrote:
> > > > On 2014/01/14 21:39:47, oshima wrote:
> > > > > On 2014/01/14 21:25:32, benjhayden_chromium wrote:
> > > > > > On 2014/01/14 19:44:46, oshima wrote:
> > > > > > > On 2014/01/14 19:20:24, asanka wrote:
> > > > > > > > The change makes sense, but someone more familiar with high dpi
> > issues
> > > > > > should
> > > > > > > > review.
> > > > > > > > 
> > > > > > > > +oshima: Could you have a look?
> > > > > > > > 
> > > > > > > > Ben: Could you add a BUG= line?
> > > > > > > Yes please. I want to understand the context and platforms it is
> > > affected
> > > > (I
> > > > > > > assume all)
> > > > > > 
> > > > > > We had a complaint that the icons looked a little blurry on a retina
> > > > macbook,
> > > > > > asking for the ability to generate icons larger than 32x32.
> > > > > > When I dug in, I noticed that it's not possible to generate icons
> larger
> > > > than
> > > > > > 32x32, but getFileIcon wasn't handling devicePixelRatio.
> > > > > > 
> > > > > > > 
> > > > > > > > Also I'm assuming this doesn't affect native UI?
> > > > > > > 
> > > > > > > This isn't quite right because DSF is dynamic (at least on mac and
> > > cros),
> > > > > and
> > > > > > > may change on the fly.
> > > > > > > 
> > > > > > > The right way to handle is to use css dataset
> > > > > > > (http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/), and
generate
> > the
> > > > data
> > > > > > url
> > > > > > > that as inlined images for all supported scale factors.
> > > > > > 
> > > > > > I think it's probably too much to expect extension authors to call
> > > > > getFileIcon()
> > > > > > for every possible devicePixelRatio and pre-generate a complete
srcset
> > for
> > > > > each
> > > > > > DownloadItem. Especially since chrome://downloads just uses
> > > > > > img.src=chrome://fileicon without a srcset. These icons are not
served
> > and
> > > > > > cached, they're generated on the client on demand.
> > > > > > I imagine that devicePixelRatio doesn't change very often, and when
it
> > > does,
> > > > > 
> > > > > It can happen often on Pixel with external display because they will
> most
> > > > likely
> > > > > have
> > > > > different scale factor.
> > > > > 
> > > > > > either the extension can call getFileIcon() again or the user can
> > refresh
> > > > the
> > > > > > tab or browser action popup bubble.
> > > > > > Let's just fix getFileIcon for scale=devicePixelRatio and not worry
> > about
> > > > > > automatically selecting different icons when devicePixelRatio
changes.
> > > > > > If users ask for a way to pre-generate different scale icons, then
> I'll
> > > just
> > > > > > make getFileIcon take a scale option alongside the size option.
> > > > > 
> > > > > I want to know a bit more background. Are you saying this isn't just
for
> > > > > chrome://downloads?
> > > > 
> > > > getFileIcon isn't for chrome://downloads at all. It's just for
extensions.
> > > > chrome://downloads is webui, not an extension.
> > > > http://developer.chrome.com/extensions/downloads.html#method-getFileIcon
> > > >
> > >
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
> > > >
> > >
> >
>
https://chrome.google.com/webstore/detail/download-manager-button/jglgknilkoe...
> > > > The file icons aren't in the screenshots for that extension because
> > > > http://crbug.com/102211. I should take new screenshots now that I
> installed
> > > > gnome-icon-theme-full and they're showing up again.
> > > 
> > > Ok, thank you for clarification. lgtm for now, but I just want to repeat
> that
> > > DSF change isn't uncommon on Pixel (and win8.1 does support DPI change).
> > > This will be more and more important as we have more high DPI
> > devices/displays,
> > > so you better have a solution in near future.
> > 
> > http://crbug.com/123694
> > Extensions can use matchMedia('screen and (-webkit-min-device-pixel-ratio:
> > 1.5)').addListener(handler) to detect when devicePixelRatio changes and call
> > getFileIcon, which will see the new devicePixelRatio and return the new icon
> > data url. This way, they don't need to call getFileIcon for ratios that
might
> > never be used, and the getFileIcon function doesn't really need to take a
> scale
> > option. They could store both the 1x and 2x icons in srcset if they want, or
> > they can call getFileIcon again if the ratio changes again if they want to
> keep
> > their page light.
> 
> Thank you for the info. That's informative.
> > 
> > chrome://downloads currently does not respond to devicePixelRatio changes.
> > Perhaps it should use that matchMedia listener?
> 
> I believe chrome//downloads already solved this issue. I agree that it sucks
if
> you have to pregenerate and we were trying to avoid that.

I just played with the resolution on my Pixel. chrome://downloads does select
the correct scale factor when the tab starts or a download finishes, but it
doesn't use srcset so changing the resolution does not automatically change the
icon scale factors. The tab needs to be refreshed manually in order to reflect
the new scale factor.

[oshima, 2014-01-14 23:34:58]

I see, thank you for testing. +

+pkotwicz@ who worked on it before.

[pkotwicz, 2014-01-15 02:33:36]

LGTM. Can you please add some manual testing steps to the CL description or the
bug? This is useful at least to Q/A.

https://codereview.chromium.org/137043005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/137043005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:328:
base::Bind(&DownloadFileIconExtractorImpl::OnIconLoadComplete,
Would it be possible to pass |scale| as a parameter to OnIconLoadComplete() and
avoid the member variable?

[benjhayden, 2014-01-15 23:58:19]

https://codereview.chromium.org/137043005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/137043005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/downloads/downloads_api.cc:328:
base::Bind(&DownloadFileIconExtractorImpl::OnIconLoadComplete,
On 2014/01/15 02:33:37, pkotwicz wrote:
> Would it be possible to pass |scale| as a parameter to OnIconLoadComplete()
and
> avoid the member variable?

Done.

https://codereview.chromium.org/137043005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/137043005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/downloads.idl:384: // pixels. Valid values are 16
and 32. No other sizes are supported.
On 2014/01/14 19:16:24, kalman wrote:
> just "The only supported sizes are 16 and 32, anything else is an error"? or
> similar.

Done.

[commit-bot: I haz the power, 2014-01-16 17:40:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/137043005/310001

[commit-bot: I haz the power, 2014-01-17 16:31:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/137043005/310001

[commit-bot: I haz the power, 2014-01-17 17:58:24]

Change committed as 245550