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..27f100d97036c6512a189b3d26e177e2703439b4 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();

[danakj, 2017/03/23 20:12:06]

I don't think a kTrivial should be part of the gfx:: namespace, it's not a gfx
concept, it's a detail of gfx::SizeF. You could make it public in gfx::SizeF,
but I think I'd prefer if you made it private and either redefine it or use
other small constants in unit tests.

(You need to define storage in the .cc file for static variables even if they
are constexpr)

[Peter Mayo, 2017/03/23 23:58:41]

I was trying to get the compilers to be able to inline the constant when (or if)
they inline the code.  In order to be used in part of a constexpr function (like
it is) kTrivial then also has to be a visible constexpr when the declaration
(and definition) are read.

I agree it doesn't need gfx:: visibility, but when I put it inside the class,
each instance got a copy, which is bad.  As a compromise, what would you think
about a well named namespace like SizeFPrivate?

Forcing all of the constructors of size to never inline might be costly
considering how widely used they are.  Should I measure this on a few platforms
to make a more informed decision?

[danakj, 2017/03/24 14:55:35]

Sorry, I meant inside the class as a private static. There would still only be a
single instance.

[Peter Mayo, 2017/03/24 21:05:41]

I don't think you addressed this part. If kTrivial is a static const it is not a
constexpr, and so we have to ditch constexpr from the SizeF constructor.

+
 // 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; }

[danakj, 2017/03/23 20:12:06]

When I wrote the fmaxf I tried >?: vs std::max vs fmaxf and found fmaxf was the
best on binary size. Can you keep it and just swap the 0 with kTrivial? Changing
it probably means you get to explore the binary size on android before/after
which sounds unfun and easier to just leave it alone.

[Peter Mayo, 2017/03/23 23:58:41]

Ack - I'll put it back.

[Peter Mayo, 2017/03/24 01:17:45]

Ick - um, no, we want to clamp to 0, not kTrivial.

[danakj, 2017/03/24 14:55:35]

Oh, I see. The comparison but output are different now. Good point.

+  void set_height(float height) { height_ = height > kTrivial ? height : 0.f; }
 
   float GetArea() const;
 
@@ -43,7 +46,7 @@ class GFX_EXPORT SizeF {
   void SetToMin(const SizeF& other);
   void SetToMax(const SizeF& other);
 
-  bool IsEmpty() const { return !width() || !height(); }
+  bool IsEmpty() const { return !height_ || !width_; }
 
   void Scale(float scale) {
     Scale(scale, scale);