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

ui/gfx/icon_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/icon_util.h"
 
 #include "base/files/file_util.h"
 #include "base/files/important_file_writer.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 31 matching lines @@
 // appropriate image for that size, and resizes it to the desired size.
 // Returns true on success, false on failure. Failure can occur if
 // |image_family| is empty, all images in the family have size 0x0, or an image
 // has no allocated pixel data.
 // |resized_image_family| must be empty.
 bool BuildResizedImageFamily(const gfx::ImageFamily& image_family,
                              gfx::ImageFamily* resized_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;
 }
 
 // Creates a set of bitmaps from an image family.
 // All images smaller than 256x256 are converted to SkBitmaps, and inserted into
 // |bitmaps| in order of aspect ratio (thinnest to widest), and then ascending
 // size order. If an image of exactly 256x256 is specified, it is converted into
 // PNG format and stored in |png_bytes|. Images with width or height larger than
 // 256 are ignored.
@@ skipping 596 matching lines @@
   // Once we compute the size for a singe AND mask scan line, we multiply that
   // number by the image height in order to get the total number of bytes for
   // the AND mask. Thus, for a 15X15 image, we need 15 * 4 which is 60 bytes
   // for the monochrome bitmap representing the AND mask.
   size_t and_line_length = (bitmap.width() + 7) >> 3;
   and_line_length = (and_line_length + 3) & ~3;
   size_t and_mask_size = and_line_length * bitmap.height();
   size_t masks_size = *xor_mask_size + and_mask_size;
   *bytes_in_resource = masks_size + sizeof(BITMAPINFOHEADER);
 }