Added ImageFamily::CreateExact, which scales the best image to size.

ui/gfx/icon_util.cc

Index: ui/gfx/icon_util.cc
diff --git a/ui/gfx/icon_util.cc b/ui/gfx/icon_util.cc
index f9758d39f67626b02bf8a1f470ff8338452ed793..68ab27922f576668e61079d538dc8f20bafb8bb5 100644
--- a/ui/gfx/icon_util.cc
+++ b/ui/gfx/icon_util.cc
@@ -49,42 +49,28 @@ bool BuildResizedImageFamily(const gfx::ImageFamily& image_family,
   DCHECK(resized_image_family);
   DCHECK(resized_image_family->empty());
 
+  // Determine whether there is an image bigger than 48x48 (kMediumIconSize).
+  const gfx::Image* biggest =
+      image_family.GetBest(IconUtil::kLargeIconSize, IconUtil::kLargeIconSize);
+  bool bigger_than_medium = biggest->Width() > IconUtil::kMediumIconSize ||

[danakj, 2015/11/05 19:03:10]

Do you need to worry about biggest being null (or empty)? l56 used to return
false in that case before dereffing/using GetBest result.

If it was empty would Width/Height be 0? Then bigger_than_medium would be false
which could cause the for loop to break (if dimension > MediumIconSize) and we'd
return true.

Previously if the image_family was empty we'd return false.

Is it possible to unit test stuff like this?

[Matt Giuca, 2015/11/06 04:15:18]

Yes, good catch. (Actually this was already caught by
IconUtilTest.TestCreateIconFileInvalidParameters.)
Yep, we should be returning false in this case.
Yep so as I said, this was already caught by
IconUtilTest.TestCreateIconFileInvalidParameters. Unfortunately, gfx_unittests
are NOT running on the waterfall (http://crbug.com/468902#c8). But at least we
have tests for it and I have verified it locally.

+                            biggest->Height() > IconUtil::kMediumIconSize;
+
   for (size_t i = 0; i < IconUtil::kNumIconDimensions; ++i) {
     int dimension = IconUtil::kIconDimensions[i];
-    gfx::Size size(dimension, dimension);
-    const gfx::Image* best = image_family.GetBest(size);
-    if (!best || best->IsEmpty()) {
-      // Either |image_family| is empty, or all images have size 0x0.
-      return false;
-    }
-
-    // Optimize for the "Large icons" view in Windows Vista+. This view displays

[danakj, 2015/11/05 19:03:10]

Perhaps this comment is worth saving? Or merging into the "There is no source
icon larger than..." comment?

[Matt Giuca, 2015/11/06 04:15:18]

Done. (Removed the mention of "Windows Vista+", since soon that will refer to
all supported versions of Windows.)

-    // icons at full size if only if there is a 256x256 (kLargeIconSize) image
-    // in the .ico file. Otherwise, it shrinks icons to 48x48 (kMediumIconSize).
-    if (dimension > IconUtil::kMediumIconSize &&
-        best->Width() <= IconUtil::kMediumIconSize &&
-        best->Height() <= IconUtil::kMediumIconSize) {
+    if (!bigger_than_medium && dimension > IconUtil::kMediumIconSize) {

[danakj, 2015/11/05 19:03:10]

nit: bigger_than_medium_exists? makes this read clearer  to me

[Matt Giuca, 2015/11/06 04:15:18]

Done (close enough).

       // There is no source icon larger than 48x48, so do not create any
       // images larger than 48x48. kIconDimensions is sorted in ascending
       // order, so it is safe to break here.
       break;
     }
 
-    if (best->Size() == size) {
-      resized_image_family->Add(*best);
-    } else {
-      // There is no |dimension|x|dimension| source image.
-      // Resize this one to the desired size, and insert it.
-      SkBitmap best_bitmap = best->AsBitmap();
-      // Only kARGB_8888 images are supported.
-      // This will also filter out images with no pixels.
-      if (best_bitmap.colorType() != kN32_SkColorType)
-        return false;
-      SkBitmap resized_bitmap = skia::ImageOperations::Resize(
-          best_bitmap, skia::ImageOperations::RESIZE_LANCZOS3,
-          dimension, dimension);
-      resized_image_family->Add(gfx::Image::CreateFrom1xBitmap(resized_bitmap));
+    gfx::Image best = image_family.CreateExact(dimension, dimension);
+    if (best.IsEmpty()) {

[danakj, 2015/11/05 19:03:10]

If you null/empty check biggest above, then would be be guaranteed to have a
valid best here?

[Matt Giuca, 2015/11/06 04:15:18]

Not quite -- CreateExact can still fail due to an image having the wrong image
format (this was also caught by
IconUtilTest.TestCreateIconFileInvalidParameters). Updated the comment to
explain why this might fail.

+      // Either |image_family| is empty, or all images have size 0x0.
+      return false;
     }
+
+    resized_image_family->Add(best);
   }
   return true;
 }