Replaced GetAppIconForSize with GetAppIconImageFamily.

chrome/browser/app_icon_win.cc

Index: chrome/browser/app_icon_win.cc
diff --git a/chrome/browser/app_icon_win.cc b/chrome/browser/app_icon_win.cc
index 6f1424e4977b5c54e3ad1045be78dc010264c421..7c67d9a8d73aa3022ae2bcd77810139997e6afb5 100644
--- a/chrome/browser/app_icon_win.cc
+++ b/chrome/browser/app_icon_win.cc
@@ -7,7 +7,9 @@
 #include "chrome/app/chrome_dll_resource.h"
 #include "chrome/common/chrome_constants.h"
 #include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/geometry/size.h"
 #include "ui/gfx/icon_util.h"
+#include "ui/gfx/image/image_family.h"
 
 #if defined(GOOGLE_CHROME_BUILD)
 #include "chrome/installer/util/install_util.h"
@@ -28,6 +30,8 @@ int GetAppIconResourceId() {
 }  // namespace
 
 HICON GetAppIcon() {
+  // TODO(mgiuca): Use GetAppIconForSize instead of LoadIcon, to get correct
+  // scaling. (See http://crbug.com/551256)
   const int icon_id = GetAppIconResourceId();
   // HICON returned from LoadIcon do not leak and do not have to be destroyed.
   return LoadIcon(GetModuleHandle(chrome::kBrowserResourcesDll),
@@ -35,16 +39,43 @@ HICON GetAppIcon() {
 }
 
 HICON GetSmallAppIcon() {
+  // TODO(mgiuca): Use GetAppIconForSize instead of LoadImage, to get correct
+  // scaling. (See http://crbug.com/551256)
   const int icon_id = GetAppIconResourceId();
+  gfx::Size size = GetSmallAppIconSize();
   // HICON returned from LoadImage must be released using DestroyIcon.
   return static_cast<HICON>(LoadImage(
       GetModuleHandle(chrome::kBrowserResourcesDll), MAKEINTRESOURCE(icon_id),
-      IMAGE_ICON, GetSystemMetrics(SM_CXSMICON), GetSystemMetrics(SM_CYSMICON),
-      LR_DEFAULTCOLOR | LR_SHARED));
+      IMAGE_ICON, size.width(), size.height(), LR_DEFAULTCOLOR | LR_SHARED));
 }
 
-scoped_ptr<SkBitmap> GetAppIconForSize(int size) {
+gfx::Size GetAppIconSize() {
+  return gfx::Size(GetSystemMetrics(SM_CXICON), GetSystemMetrics(SM_CYICON));
+}
+
+gfx::Size GetSmallAppIconSize() {
+  return gfx::Size(GetSystemMetrics(SM_CXSMICON),
+                   GetSystemMetrics(SM_CYSMICON));
+}
+
+scoped_ptr<gfx::ImageFamily> GetAppIconImageFamily() {
   const int icon_id = GetAppIconResourceId();
-  return IconUtil::CreateSkBitmapFromIconResource(
-      GetModuleHandle(chrome::kBrowserResourcesDll), icon_id, size);
+  return IconUtil::CreateImageFamilyFromIconResource(
+      GetModuleHandle(chrome::kBrowserResourcesDll), icon_id);
+}
+
+scoped_ptr<SkBitmap> GetAppIconForSize(const gfx::Size& size) {
+  scoped_ptr<gfx::ImageFamily> family = GetAppIconImageFamily();
+  if (!family)
+    return scoped_ptr<SkBitmap>();
+
+  // Note: We could just use the LoadImage function from the Windows API, but
+  // that does a *terrible* job scaling images. Therefore, we fetch the original
+  // images and do our own high-quality scaling.
+  gfx::Image image = family->CreateExact(size);

[Nico, 2015/11/10 07:13:31]

gfx::Image can contain several versions of the same bitmap at different scaling
factors. Do we use this on Windows? (On Mac, this is used to let the OS pick the
right version of a bitmap when you drag a chrome window from a retina screen to
a regular screen and back.) Since you only care about one bitmap in the image
here, you're doing more computation here than needed.

[Matt Giuca, 2015/11/11 02:36:22]

Short answer: no, we don't use gfx::Image scale factors because we don't know
what density Windows wants, only what size it wants. (Therefore, ImageFamily is
appropriate.)

In this case, we are not dealing with low/high DPI versions of the same logical
image. We are dealing with multiple images of different logical sizes. (A 32x32
icon image is NOT a high-DPI version of a 16x16 icon image; it is a larger
image.) This is the reason I created ImageFamily in the first place: to capture
the distinction between low/high DPI versus small/large logical size. (At the
time, we had code putting icon images of all sizes into an ImageSkia as
different "densities" which was a hack.)

So the nice theory (which I think is fairly clear on Mac) is that you have a
"logical size" (e.g., 16x16) and then a "density" (e.g., 150%), in which case
you'd pick the 16x16 Image in the ImageFamily an then get its 150%
representation from the ImageSkia or NSImage. Unfortunately, Windows blurs the
concept of size and density, because at least for these GetSystemMetrics APIs,
we are just given a size. If the user's display is set to 150%,
GetSystemMetrics(SM_CXSMICON) just returns 24.

So all we can really do is put it in an ImageFamily, and then take the best
sized icon and scale it to 24.
Yeah, I am. This is the price I'm paying for use of the nice ImageFamily
abstraction. The logic (using this abstraction) is:
1. Parse the ICO header table to find out what sizes are available.
2. Load all the images into an ImageFamily.
3. Choose the best one for the target size.
4. Scale it to the target size.

Without ImageFamily, we'd have to do this:
1. Parse the ICO header table to find out what sizes are available.
2. Choose the best one for the target size.
3. Load that specific image.
4. Scale it to the target size.

The alternative approach avoids loading unnecessary images, but it means
duplicating or significantly refactoring most of the logic in ImageFamily. I've
thought a lot about how to do that, and I did benchmarks above which showed that
by only loading the image we need, we could reduce a 6ms operation to 3ms. I
don't think that's worth the added complexity in the code, given that this will
only be used once on startup, in a delayed task.

+  return make_scoped_ptr(new SkBitmap(image.AsBitmap()));
+}
+
+scoped_ptr<SkBitmap> GetAppIconForSize(int size) {
+  return GetAppIconForSize(gfx::Size(size, size));
 }