Stabilize empty rect handling in EnclosingRect.

ui/gfx/geometry/size_f.h

 // 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_GEOMETRY_SIZE_F_H_
 #define UI_GFX_GEOMETRY_SIZE_F_H_
 
 #include <iosfwd>
 #include <string>
 
 #include "base/compiler_specific.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/gfx_export.h"
 
 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()),
               static_cast<float>(size.height())) {}
 
   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;
 
   void SetSize(float width, float height) {
     set_width(width);
     set_height(height);
   }
 
   void Enlarge(float grow_width, float grow_height);
 
   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);
   }
 
   void Scale(float x_scale, float y_scale) {
     SetSize(width() * x_scale, height() * y_scale);
   }
 
   std::string ToString() const;
@@ skipping 18 matching lines @@
 }
 
 // This is declared here for use in gtest-based unit tests but is defined in
 // the //ui/gfx:test_support target. Depend on that to use this in your unit
 // test. This should not be used in production code - call ToString() instead.
 void PrintTo(const SizeF& size, ::std::ostream* os);
 
 }  // namespace gfx
 
 #endif  // UI_GFX_GEOMETRY_SIZE_F_H_