Fixes cases where we incorrectly convert from RectF to Rect by flooring. In all cases we should be taking the enclosing or enclosed int rect as appropriate.

This mainly affects bits of code using the old Rect Rect::Scale(float) function. There are, thankfully, not too many. I've replaced this legacy function with Rect Rect::ScaleUnsafe(float) and when this lands, I will open a bug for switching from ScaleUnsafe to a Scale followed by a ToEnclosedRect or ToEnclosingRect.

BUG=152596

Review URL: https://chromiumcodereview.appspot.com/10996037

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@159256 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 296 insertions, 72 deletions

diff --git a/ui/gfx/image/image_skia_operations.cc b/ui/gfx/image/image_skia_operations.cc
index a9809b3..f190d8c 100644
--- a/ui/gfx/image/image_skia_operations.cc
+++ b/ui/gfx/image/image_skia_operations.cc
@@ -17,6 +17,7 @@
 #include "ui/gfx/image/image_skia_source.h"
 #include "ui/gfx/insets.h"
 #include "ui/gfx/rect.h"
+#include "ui/gfx/rect_conversions.h"
 #include "ui/gfx/size.h"
 #include "ui/gfx/skbitmap_operations.h"
 #include "ui/gfx/skia_util.h"
@@ -286,8 +287,9 @@ class ExtractSubsetImageSource: public gfx::ImageSkiaSource {
   // gfx::ImageSkiaSource overrides:
   virtual ImageSkiaRep GetImageForScale(ui::ScaleFactor scale_factor) OVERRIDE {
     ImageSkiaRep image_rep = image_.GetRepresentation(scale_factor);
-    SkIRect subset_bounds_in_pixel = RectToSkIRect(subset_bounds_.Scale(
-        ui::GetScaleFactorScale(image_rep.scale_factor())));
+    SkIRect subset_bounds_in_pixel = RectToSkIRect(
+        ToEnclosingRect(subset_bounds_.Scale(
+            ui::GetScaleFactorScale(image_rep.scale_factor()))));
     SkBitmap dst;
     bool success = image_rep.sk_bitmap().extractSubset(&dst,
                                                        subset_bounds_in_pixel);
diff --git a/ui/gfx/rect.h b/ui/gfx/rect.h
index 652c019..64f35b1 100644
--- a/ui/gfx/rect.h
+++ b/ui/gfx/rect.h
@@ -16,6 +16,7 @@
 
 #include "ui/gfx/point.h"
 #include "ui/gfx/rect_base.h"
+#include "ui/gfx/rect_f.h"
 #include "ui/gfx/size.h"
 
 #if defined(OS_WIN)
@@ -67,6 +68,18 @@ class UI_EXPORT Rect : public RectBase<Rect, Point, Size, Insets, int> {
   CGRect ToCGRect() const;
 #endif
 
+  RectF ToRectF() const WARN_UNUSED_RESULT {
+    return RectF(origin().x(), origin().y(), size().width(), size().height());
+  }
+
+  RectF Scale(float scale) const WARN_UNUSED_RESULT {
+    return Scale(scale, scale);
+  }
+
+  RectF Scale(float x_scale, float y_scale) const WARN_UNUSED_RESULT {
+    return ToRectF().Scale(x_scale, y_scale);
+  }
+
   std::string ToString() const;
 };
 
diff --git a/ui/gfx/rect_base.h b/ui/gfx/rect_base.h
index 2219306..b6b5c99 100644
--- a/ui/gfx/rect_base.h
+++ b/ui/gfx/rect_base.h
@@ -65,16 +65,6 @@ class UI_EXPORT RectBase {
     Offset(point.x(), point.y());
   }
 
-  /// Scales the rectangle by |scale|.
-  Class Scale(float scale) const WARN_UNUSED_RESULT {
-    return Scale(scale, scale);
-  }
-
-  Class Scale(float x_scale, float y_scale) const WARN_UNUSED_RESULT {
-    return Class(origin_.Scale(x_scale, y_scale),
-                 size_.Scale(x_scale, y_scale));
-  }
-
   InsetsClass InsetsFrom(const Class& inner) const {
     return InsetsClass(inner.y() - y(),
                        inner.x() - x(),
diff --git a/ui/gfx/rect_conversions.cc b/ui/gfx/rect_conversions.cc
new file mode 100644
index 0000000..0d8fdb5
--- /dev/null
+++ b/ui/gfx/rect_conversions.cc
@@ -0,0 +1,32 @@
+// 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/rect_conversions.h"
+
+#include "ui/gfx/safe_floor_ceil.h"
+
+namespace gfx {
+
+Rect ToEnclosingRect(const RectF& rect) {
+  int min_x = ToFlooredInt(rect.origin().x());
+  int min_y = ToFlooredInt(rect.origin().y());
+  float max_x = rect.origin().x() + rect.size().width();
+  float max_y = rect.origin().y() + rect.size().height();
+  int width = std::max(ToCeiledInt(max_x) - min_x, 0);
+  int height = std::max(ToCeiledInt(max_y) - min_y, 0);
+  return Rect(min_x, min_y, width, height);
+}
+
+Rect ToEnclosedRect(const RectF& rect) {
+  int min_x = ToCeiledInt(rect.origin().x());
+  int min_y = ToCeiledInt(rect.origin().y());
+  float max_x = rect.origin().x() + rect.size().width();
+  float max_y = rect.origin().y() + rect.size().height();
+  int width = std::max(ToFlooredInt(max_x) - min_x, 0);
+  int height = std::max(ToFlooredInt(max_y) - min_y, 0);
+  return Rect(min_x, min_y, width, height);
+}
+
+}  // namespace gfx
+
diff --git a/ui/gfx/rect_conversions.h b/ui/gfx/rect_conversions.h
new file mode 100644
index 0000000..84e0d93
--- /dev/null
+++ b/ui/gfx/rect_conversions.h
@@ -0,0 +1,21 @@
+// 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.
+
+#ifndef UI_GFX_RECT_CONVERSIONS_H_
+#define UI_GFX_RECT_CONVERSIONS_H_
+
+#include "ui/gfx/rect.h"
+#include "ui/gfx/rect_f.h"
+
+namespace gfx {
+
+// Returns the smallest Rect that encloses the given RectF.
+UI_EXPORT Rect ToEnclosingRect(const RectF& rect);
+
+// Returns the largest Rect that is enclosed by the given Rect.
+UI_EXPORT Rect ToEnclosedRect(const RectF& rect);
+
+}  // namespace gfx
+
+#endif  // UI_GFX_RECT_CONVERSIONS_H_
diff --git a/ui/gfx/rect_f.cc b/ui/gfx/rect_f.cc
index 6677c29..8503924 100644
--- a/ui/gfx/rect_f.cc
+++ b/ui/gfx/rect_f.cc
@@ -36,10 +36,6 @@ RectF::RectF(const gfx::PointF& origin, const gfx::SizeF& size)
 
 RectF::~RectF() {}
 
-Rect RectF::ToRect() const {
-  return Rect(origin().ToPoint(), size().ToSize());
-}
-
 std::string RectF::ToString() const {
   return base::StringPrintf("%s %s",
                             origin().ToString().c_str(),
diff --git a/ui/gfx/rect_f.h b/ui/gfx/rect_f.h
index 4a2d642..4395b52 100644
--- a/ui/gfx/rect_f.h
+++ b/ui/gfx/rect_f.h
@@ -8,7 +8,6 @@
 #include <string>
 
 #include "ui/gfx/point_f.h"
-#include "ui/gfx/rect.h"
 #include "ui/gfx/rect_base.h"
 #include "ui/gfx/size_f.h"
 
@@ -27,7 +26,16 @@ class UI_EXPORT RectF : public RectBase<RectF, PointF, SizeF, InsetsF, float> {
 
   ~RectF();
 
-  Rect ToRect() const;
+  /// Scales the rectangle by |scale|.
+  RectF Scale(float scale) const WARN_UNUSED_RESULT {
+    return Scale(scale, scale);
+  }
+
+  RectF Scale(float x_scale, float y_scale) const WARN_UNUSED_RESULT {
+    SizeF newSize = size().Scale(x_scale, y_scale);
+    newSize.ClampToNonNegative();
+    return RectF(origin().Scale(x_scale, y_scale), newSize);
+  }
 
   std::string ToString() const;
 };
@@ -38,4 +46,4 @@ extern template class RectBase<RectF, PointF, SizeF, InsetsF, float>;
 
 }  // namespace gfx
 
-#endif  // UI_GFX_RECT_H_
+#endif  // UI_GFX_RECT_F_H_
diff --git a/ui/gfx/rect_unittest.cc b/ui/gfx/rect_unittest.cc
index 4eefdd6..abc94c1 100644
--- a/ui/gfx/rect_unittest.cc
+++ b/ui/gfx/rect_unittest.cc
@@ -5,8 +5,11 @@
 #include "base/basictypes.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/rect.h"
+#include "ui/gfx/rect_conversions.h"
 #include "ui/gfx/skia_util.h"
 
+#include <limits>
+
 namespace ui {
 
 TEST(RectTest, Contains) {
@@ -344,6 +347,158 @@ TEST(RectTest, SkRectToRect) {
   EXPECT_EQ(src, gfx::SkRectToRect(skrect));
 }
 
+// Similar to EXPECT_FLOAT_EQ, but lets NaN equal NaN
+#define EXPECT_FLOAT_AND_NAN_EQ(a, b) \
+  { if (a == a || b == b) { EXPECT_FLOAT_EQ(a, b); } }
+
+
+TEST(RectTest, ScaleRect) {
+  static const struct Test {
+    int x1;  // source
+    int y1;
+    int w1;
+    int h1;
+    float scale;
+    float x2;  // target
+    float y2;
+    float w2;
+    float h2;
+  } tests[] = {
+    { 3, 3, 3, 3,
+      1.5f,
+      4.5f, 4.5f, 4.5f, 4.5f },
+    { 3, 3, 3, 3,
+      0.0f,
+      0.0f, 0.0f, 0.0f, 0.0f },
+    { 3, 3, 3, 3,
+      std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN() },
+    { 3, 3, 3, 3,
+      std::numeric_limits<float>::max(),
+      std::numeric_limits<float>::max(),
+      std::numeric_limits<float>::max(),
+      std::numeric_limits<float>::max(),
+      std::numeric_limits<float>::max() },
+    { 3, 3, 3, 3,
+      -1.0f,
+      -3.0f, -3.0f, 0.0f, 0.0f }
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    gfx::Rect r1(tests[i].x1, tests[i].y1, tests[i].w1, tests[i].h1);
+    gfx::RectF r2(tests[i].x2, tests[i].y2, tests[i].w2, tests[i].h2);
+
+    gfx::RectF scaled = r1.Scale(tests[i].scale);
+    EXPECT_FLOAT_AND_NAN_EQ(r2.x(), scaled.x());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.y(), scaled.y());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.width(), scaled.width());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.height(), scaled.height());
+  }
+}
+
+TEST(RectTest, ToEnclosedRect) {
+  static const struct Test {
+    float x1; // source
+    float y1;
+    float w1;
+    float h1;
+    int x2; // target
+    int y2;
+    int w2;
+    int h2;
+  } tests [] = {
+    { 0.0f, 0.0f, 0.0f, 0.0f,
+      0, 0, 0, 0 },
+    { -1.5f, -1.5f, 3.0f, 3.0f,
+      -1, -1, 2, 2 },
+    { -1.5f, -1.5f, 3.5f, 3.5f,
+      -1, -1, 3, 3 },
+    { std::numeric_limits<float>::max(),
+      std::numeric_limits<float>::max(),
+      2.0f, 2.0f,
+      std::numeric_limits<int>::max(),
+      std::numeric_limits<int>::max(),
+      0, 0 },
+    { 0.0f, 0.0f,
+      std::numeric_limits<float>::max(),
+      std::numeric_limits<float>::max(),
+      0, 0,
+      std::numeric_limits<int>::max(),
+      std::numeric_limits<int>::max() },
+    { 20000.5f, 20000.5f, 0.5f, 0.5f,
+      20001, 20001, 0, 0 },
+    { std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      0, 0, 0, 0 }
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    gfx::RectF r1(tests[i].x1, tests[i].y1, tests[i].w1, tests[i].h1);
+    gfx::Rect r2(tests[i].x2, tests[i].y2, tests[i].w2, tests[i].h2);
+
+    gfx::Rect enclosed = ToEnclosedRect(r1);
+    EXPECT_FLOAT_AND_NAN_EQ(r2.x(), enclosed.x());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.y(), enclosed.y());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.width(), enclosed.width());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.height(), enclosed.height());
+  }
+}
+
+TEST(RectTest, ToEnclosingRect) {
+  static const struct Test {
+    float x1; // source
+    float y1;
+    float w1;
+    float h1;
+    int x2; // target
+    int y2;
+    int w2;
+    int h2;
+  } tests [] = {
+    { 0.0f, 0.0f, 0.0f, 0.0f,
+      0, 0, 0, 0 },
+    { -1.5f, -1.5f, 3.0f, 3.0f,
+      -2, -2, 4, 4 },
+    { -1.5f, -1.5f, 3.5f, 3.5f,
+      -2, -2, 4, 4 },
+    { std::numeric_limits<float>::max(),
+      std::numeric_limits<float>::max(),
+      2.0f, 2.0f,
+      std::numeric_limits<int>::max(),
+      std::numeric_limits<int>::max(),
+      0, 0 },
+    { 0.0f, 0.0f,
+      std::numeric_limits<float>::max(),
+      std::numeric_limits<float>::max(),
+      0, 0,
+      std::numeric_limits<int>::max(),
+      std::numeric_limits<int>::max() },
+    { 20000.5f, 20000.5f, 0.5f, 0.5f,
+      20000, 20000, 1, 1 },
+    { std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      std::numeric_limits<float>::quiet_NaN(),
+      0, 0, 0, 0 }
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    gfx::RectF r1(tests[i].x1, tests[i].y1, tests[i].w1, tests[i].h1);
+    gfx::Rect r2(tests[i].x2, tests[i].y2, tests[i].w2, tests[i].h2);
+
+    gfx::Rect enclosed = ToEnclosingRect(r1);
+    EXPECT_FLOAT_AND_NAN_EQ(r2.x(), enclosed.x());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.y(), enclosed.y());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.width(), enclosed.width());
+    EXPECT_FLOAT_AND_NAN_EQ(r2.height(), enclosed.height());
+  }
+}
+
 #if defined(OS_WIN)
 TEST(RectTest, ConstructAndAssign) {
   const RECT rect_1 = { 0, 0, 10, 10 };
diff --git a/ui/gfx/safe_floor_ceil.cc b/ui/gfx/safe_floor_ceil.cc
new file mode 100644
index 0000000..c8ac9e1
--- /dev/null
+++ b/ui/gfx/safe_floor_ceil.cc
@@ -0,0 +1,29 @@
+// 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 <cmath>
+#include <limits>
+
+namespace gfx {
+
+int ClampToInt(float value) {
+  if (value != value)
+     return 0; // no int NaN.
+  if (value > std::numeric_limits<int>::max())
+    return std::numeric_limits<int>::max();
+  if (value < std::numeric_limits<int>::min())
+    return std::numeric_limits<int>::min();
+  return static_cast<int>(value);
+}
+
+int ToFlooredInt(float value) {
+  return ClampToInt(std::floor(value));
+}
+
+int ToCeiledInt(float value) {
+  return ClampToInt(std::ceil(value));
+}
+
+}  // namespace gfx
+
diff --git a/ui/gfx/safe_floor_ceil.h b/ui/gfx/safe_floor_ceil.h
new file mode 100644
index 0000000..c2d80c6
--- /dev/null
+++ b/ui/gfx/safe_floor_ceil.h
@@ -0,0 +1,16 @@
+// 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.
+
+#ifndef UI_GFX_SAFE_FLOOR_CEIL_H_
+#define UI_GFX_SAFE_FLOOR_CEIL_H_
+
+namespace gfx {
+
+UI_EXPORT int ClampToInt(float value);
+UI_EXPORT int ToFlooredInt(float value);
+UI_EXPORT int ToCeiledInt(float value);
+
+}  // namespace gfx
+
+#endif  // UI_GFX_SAFE_FLOOR_CEIL_H_
diff --git a/ui/gfx/size.cc b/ui/gfx/size.cc
index de21a70..6d28ded 100644
--- a/ui/gfx/size.cc
+++ b/ui/gfx/size.cc
@@ -11,7 +11,6 @@
 #include "base/logging.h"
 #include "base/stringprintf.h"
 #include "ui/gfx/size_base.h"
-#include "ui/gfx/size_base_impl.h"
 
 namespace gfx {
 
diff --git a/ui/gfx/size_base.h b/ui/gfx/size_base.h
index ef687c6..788120b 100644
--- a/ui/gfx/size_base.h
+++ b/ui/gfx/size_base.h
@@ -41,8 +41,8 @@ class UI_EXPORT SizeBase {
                  static_cast<Type>(height_ * y_scale));
   }
 
-  void set_width(Type width);
-  void set_height(Type height);
+  void set_width(Type width) { width_ = width; }
+  void set_height(Type height) { height_ = height; }
 
   bool operator==(const Class& s) const {
     return width_ == s.width_ && height_ == s.height_;
@@ -53,15 +53,24 @@ class UI_EXPORT SizeBase {
   }
 
   bool IsEmpty() const {
-    // Size doesn't allow negative dimensions, so testing for 0 is enough.
-    return (width_ == 0) || (height_ == 0);
+    return (width_ <= 0) || (height_ <= 0);
+  }
+
+  void ClampToNonNegative() {
+    if (width_ < 0)
+      width_ = 0;
+    if (height_ < 0)
+      height_ = 0;
   }
 
  protected:
-  SizeBase(Type width, Type height);
+  SizeBase(Type width, Type height)
+      : width_(width),
+        height_(height) {}
+
   // Destructor is intentionally made non virtual and protected.
   // Do not make this public.
-  ~SizeBase();
+  ~SizeBase() {}
 
  private:
   Type width_;
diff --git a/ui/gfx/size_base_impl.h b/ui/gfx/size_base_impl.h
deleted file mode 100644
index 4eca741..0000000
--- a/ui/gfx/size_base_impl.h
+++ /dev/null
@@ -1,45 +0,0 @@
-// 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/size_base.h"
-
-#include "base/logging.h"
-#include "base/stringprintf.h"
-
-// This file provides the implementation for SizeBaese template and
-// used to instantiate the base class for Size and SizeF classes.
-#if !defined(UI_IMPLEMENTATION)
-#error "This file is intended for UI implementation only"
-#endif
-
-namespace gfx {
-
-template<typename Class, typename Type>
-SizeBase<Class, Type>::SizeBase(Type width, Type height) {
-  set_width(width);
-  set_height(height);
-}
-
-template<typename Class, typename Type>
-SizeBase<Class, Type>::~SizeBase() {}
-
-template<typename Class, typename Type>
-void SizeBase<Class, Type>::set_width(Type width) {
-  if (width < 0) {
-    NOTREACHED() << "negative width:" << width;
-    width = 0;
-  }
-  width_ = width;
-}
-
-template<typename Class, typename Type>
-void SizeBase<Class, Type>::set_height(Type height) {
-  if (height < 0) {
-    NOTREACHED() << "negative height:" << height;
-    height = 0;
-  }
-  height_ = height;
-}
-
-}  // namespace gfx
diff --git a/ui/gfx/size_f.cc b/ui/gfx/size_f.cc
index 53645eb..862c992 100644
--- a/ui/gfx/size_f.cc
+++ b/ui/gfx/size_f.cc
@@ -9,7 +9,6 @@
 #include "base/logging.h"
 #include "base/stringprintf.h"
 #include "ui/gfx/size.h"
-#include "ui/gfx/size_base_impl.h"
 
 namespace gfx {