Added new class gfx::ImageFamily.

ui/gfx/image/image_family.cc

Index: ui/gfx/image/image_family.cc
diff --git a/ui/gfx/image/image_family.cc b/ui/gfx/image/image_family.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d24ce10ec1b5925d95c930d1b7febb00fd4f4f50
--- /dev/null
+++ b/ui/gfx/image/image_family.cc
@@ -0,0 +1,120 @@
+// Copyright 2013 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/image/image_family.h"
+
+#include <cmath>
+
+#include "ui/gfx/image/image.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/size.h"
+
+namespace gfx {
+
+ImageFamily::ImageFamily() {}
+ImageFamily::~ImageFamily() {}
+
+void ImageFamily::Add(const gfx::Image& image) {
+  gfx::Size size = image.Size();
+  if (size.IsEmpty()) {
+    map_[MapKey(1.0f, 0)] = image;

[pkotwicz, 2013/03/29 00:25:35]

Should we just not store |image| if size.IsEmpty() ?

[Matt Giuca, 2013/03/30 00:54:14]

I think that would be more surprising semantics. That would mean that:
image_family.Add(gfx::Image());
image_family.Get(32, 32)
returns NULL, whereas I would expect it to return the empty image I put in. I
would expect it to prioritize any non-empty image, but if I put in only an empty
image, I expect it to come out again.

[pkotwicz, 2013/04/02 16:06:31]

Fair enough

+  } else {
+    float aspect = static_cast<float>(size.width()) / size.height();
+    DCHECK_GT(aspect, 0.0f);
+    map_[MapKey(aspect, size.width())] = image;
+  }
+}
+
+void ImageFamily::Add(const gfx::ImageSkia& image_skia) {
+  Add(gfx::Image(image_skia));
+}
+
+const gfx::Image* ImageFamily::Get(int width, int height) const {
+  if (map_.empty())
+    return NULL;
+
+  // If either width or height is 0, both are.
+  float desired_aspect;
+  if (height == 0 || width == 0) {
+    desired_aspect = 1.0f;
+    height = 0;
+    width = 0;
+  } else {
+    desired_aspect = static_cast<float>(width) / height;
+  }
+  DCHECK_GT(desired_aspect, 0.0f);
+
+  // Find the closest aspect ratio in the map to the desired one.

[pkotwicz, 2013/03/29 00:25:35]

Nit: I am unsure if the comment above says anything that the function name below
does not.

[Matt Giuca, 2013/03/30 00:54:14]

Done.

+  float closest_aspect = GetClosestAspect(desired_aspect);
+
+  // If thinner than desired, search for images with width such that the
+  // corresponding height is greater than or equal to the desired height.
+  int desired_width = closest_aspect <= desired_aspect ? width :

[pkotwicz, 2013/03/29 00:25:35]

Nit, fix alignment like so:
int desired_width = closest_aspect <= desired_aspect ?
    width : static_cast<int>(ceilf(height * closest_aspect));

[Matt Giuca, 2013/03/30 00:54:14]

Done.

+                      static_cast<int>(ceilf(height * closest_aspect));
+
+  // Get the best-sized image with the aspect ratio.
+  return GetWithExactAspect(closest_aspect, desired_width);
+}
+
+float ImageFamily::GetClosestAspect(float desired_aspect) const {
+  // Find the two aspect ratios on either side of desired_aspect.
+  std::map<MapKey, gfx::Image>::const_iterator greater_or_equal =
+      map_.lower_bound(MapKey(desired_aspect, 0.0f));
+  // Early exit optimization if there is an exact match.
+  if (greater_or_equal != map_.end() &&
+      greater_or_equal->first.aspect() == desired_aspect) {
+    return desired_aspect;
+  }
+
+  // No exact match; greater_or_equal will point to the first image with aspect
+  // ratio >= desired_aspect, and less_than will point to the last image with
+  // aspect ratio < desired_aspect.
+  if (greater_or_equal != map_.begin()) {
+    std::map<MapKey, gfx::Image>::const_iterator less_than =
+        greater_or_equal;
+    --less_than;
+    float thinner_aspect = less_than->first.aspect();
+    DCHECK_GT(thinner_aspect, 0.0f);
+    DCHECK_LT(thinner_aspect, desired_aspect);
+    if (greater_or_equal != map_.end()) {
+      float wider_aspect = greater_or_equal->first.aspect();
+      DCHECK_GT(wider_aspect, desired_aspect);
+      if ((wider_aspect / desired_aspect) < (desired_aspect / thinner_aspect))
+        return wider_aspect;
+    }
+    return thinner_aspect;
+  } else {
+    // No aspect ratio is less than or equal to desired_aspect.
+    DCHECK(greater_or_equal != map_.end());
+    float wider_aspect = greater_or_equal->first.aspect();
+    DCHECK_GT(wider_aspect, desired_aspect);
+    return wider_aspect;
+  }

[pkotwicz, 2013/03/29 00:25:35]

Nit, this MIGHT be simpler:

DCHECK(!map_.empty());

float wider_aspect = std::numeric_limits<float>::max();
std::map<MapKey, gfx::Image>::const_iterator greater_or_equal =
    map_.lower_bound(MapKey(desired_aspect, 0.0f));
if (greater_or_equal != map_.end()) {
  wider_aspect = greater_or_equal->first.aspect();
  DCHECK_GE(wider_aspect, desired_aspect);
}

if (wider_aspect == desired_aspect)
  return desired_aspect;

float thinner_aspect = std::numeric_limits<float>::max();

...

I am fine if you keep it as is though.

[Matt Giuca, 2013/03/30 00:54:14]

Using float max to avoid having a special case where there is no wider aspect
ratio works, but I don't think there is a good way to do it on the thinner end.
You set thinner_aspect to float max, but doesn't it have to be 0? (Because
thinner_aspect should be <= desired_aspect.) If thinner_aspect is 0, you have
divide-by-zero to worry about (and I thought that would just be +infinity, but I
checked, and it's actually undefined behaviour). So although my code has a few
special cases, I think it accurately reflects that the logic itself has special
cases, so I'll stick with it.

[pkotwicz, 2013/04/02 16:06:31]

Fair enough

+}
+
+const gfx::Image* ImageFamily::GetWithExactAspect(float aspect, int width)

[pkotwicz, 2013/03/29 00:25:35]

For the sake of wrapping:
const gfx::Image* ImageFamily::GetWithExactAspect(float aspect,
                                                  int width) const {

[Matt Giuca, 2013/03/30 00:54:14]

Done.

+  const {
+  // Find the two images of given aspect ratio on either side of width.
+  std::map<MapKey, gfx::Image>::const_iterator greater_or_equal =
+      map_.lower_bound(MapKey(aspect, width));
+  if (greater_or_equal != map_.end() &&
+      greater_or_equal->first.aspect() == aspect) {
+    // We have found the smallest image of the same size or greater.

[pkotwicz, 2013/03/29 00:25:35]

Nit: I am unsure if this comment says anything than the code above does not. But
I will leave it up to you to decide whether it is needed.

[Matt Giuca, 2013/03/30 00:54:14]

It's not obvious to me as a reader exactly how lower_bound works without having
to look it up, so I think it's a helpful comment.

+    return &greater_or_equal->second;
+  }
+
+  DCHECK(greater_or_equal != map_.begin());
+  std::map<MapKey, gfx::Image>::const_iterator less_than = greater_or_equal;
+  --less_than;
+  // This must be true because there must be at least one image with aspect.

[pkotwicz, 2013/03/29 00:25:35]

Nit: |aspect|.

[Matt Giuca, 2013/03/30 00:54:14]

Done.

+  DCHECK_EQ(less_than->first.aspect(), aspect);
+  // We have found the largest image smaller than desired.
+  return &less_than->second;
+}
+
+ImageFamily::const_iterator::const_iterator(
+    const std::map<MapKey, gfx::Image>::const_iterator& other)
+    : map_iterator_(other) {}
+
+}  // namespace gfx