Stabilize empty rect handling in EnclosingRect.

ui/gfx/geometry/size_f.h

Index: ui/gfx/geometry/size_f.h
diff --git a/ui/gfx/geometry/size_f.h b/ui/gfx/geometry/size_f.h
index 9d1bb5d7e3ed6c18d4ff3d3193a510ae85c70c48..1009ac47df61420c6ebd298bf0c016d153bfc241 100644
--- a/ui/gfx/geometry/size_f.h
+++ b/ui/gfx/geometry/size_f.h
@@ -14,12 +14,15 @@
 
 namespace gfx {
 
+static constexpr float kTrivial = 8.f * std::numeric_limits<float>::epsilon();
+
 // A floating version of gfx::Size.
 class GFX_EXPORT SizeF {
  public:
   constexpr SizeF() : width_(0.f), height_(0.f) {}
   constexpr SizeF(float width, float height)
-      : width_(width >= 0 ? width : 0), height_(height >= 0 ? height : 0) {}
+      : width_(width >= kTrivial ? width : 0.f),
+        height_(height >= kTrivial ? height : 0.f) {}
 
   constexpr explicit SizeF(const Size& size)
       : SizeF(static_cast<float>(size.width()),
@@ -28,8 +31,8 @@ class GFX_EXPORT SizeF {
   constexpr float width() const { return width_; }
   constexpr float height() const { return height_; }
 
-  void set_width(float width) { width_ = fmaxf(0, width); }
-  void set_height(float height) { height_ = fmaxf(0, height); }
+  void set_width(float width) { width_ = width >= kTrivial ? width : 0.f; }
+  void set_height(float height) { height_ = height >= kTrivial ? height : 0.f; }
 
   float GetArea() const;
 
@@ -43,7 +46,9 @@ class GFX_EXPORT SizeF {
   void SetToMin(const SizeF& other);
   void SetToMax(const SizeF& other);
 
-  bool IsEmpty() const { return !width() || !height(); }
+  bool HasHeight() const { return height() > kTrivial; }

[danakj, 2017/03/21 16:16:36]

How would height be < kTrivial? This is now just checking if height() ==
kTrivial which seems strange. These don't seem needed once you're clamping to 0
at the setters?

[Peter Mayo, 2017/03/21 16:45:57]

The best thing about these is that they make it clear that the decision about
whether a Size/Width/Height is empty/trivial belongs here, rather than
distributed all over the codebase.

Clamping them in the setters keeps us from further accumulating small parts of
errors, and causing scaling to stay sticky at empty.

I guess I've lived in too many projects where being robust to someone adding a
new method that modifies an internal (for example, by passing it by reference to
a skia transform method).

Regarding the point that the test is asymmetric for == kTrivial, I would propose
changing the setters.

I'm also open to not clamping in the setters.  My least favorite option is
removing the boolean tests and letting clients implicitly test against zero.

[danakj, 2017/03/21 17:42:05]

I think I'm okay with clamping at the setters. But I'm just questioning why
you'd want HasHeight/HasWidth then. It seems like IsEmpty() does what you want
then.

[Peter Mayo, 2017/03/23 19:29:22]

Dana Wrote:
OK, removed.

[Peter Mayo, 2017/03/23 19:29:22]

I think the decision as to whether HasHeight() is { return height() > 0; } or {
return height() > kTrivial; } belongs in one place, rather than spread through
the calling code as the magic numbers, even between, 0. or epsilon.  e.g. if
someone were to decide clamping is bad, removing it should all be in size_f.cc
[h] not rect.cc and friends/distant relations.

I'm cool with inlining it in the header for the optimizer to work it's magic on
Today's flavor.

+  bool HasWidth() const { return width() > kTrivial; }
+  bool IsEmpty() const { return !HasHeight() || !HasWidth(); }
 
   void Scale(float scale) {
     Scale(scale, scale);