Fixed BubbleBorder sizing problems & added unit tests.

ui/views/bubble/bubble_border_unittest.cc

Index: ui/views/bubble/bubble_border_unittest.cc
diff --git a/ui/views/bubble/bubble_border_unittest.cc b/ui/views/bubble/bubble_border_unittest.cc
index b8838f656ba177e70a6cac271754729b726c250b..8161826deab1229e9a1d46c7b9e02fb753c08381 100644
--- a/ui/views/bubble/bubble_border_unittest.cc
+++ b/ui/views/bubble/bubble_border_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "ui/views/bubble/bubble_border.h"
 
+#include "base/memory/scoped_ptr.h"
 #include "ui/views/test/views_test_base.h"
 
 namespace views {
@@ -202,39 +203,267 @@ TEST_F(BubbleBorderTest, IsArrowAtCenter) {
   EXPECT_FALSE(BubbleBorder::is_arrow_at_center(BubbleBorder::FLOAT));
 }
 
-TEST_F(BubbleBorderTest, TestMinimalSize) {
-  gfx::Rect anchor = gfx::Rect(100, 100, 20, 20);
-  gfx::Size contents = gfx::Size(10, 10);
-  BubbleBorder b1(BubbleBorder::RIGHT_TOP, BubbleBorder::NO_SHADOW, 0);
+class BubbleBorderGetBoundsSizeTest : public views::ViewsTestBase {

[msw, 2014/08/11 22:26:33]

Merge the two test classes, they're similar enough.
(optionally replace the BubbleBorderTest typedef)

[bruthig, 2014/08/19 19:02:13]

Done.

+ public:
+  const BubbleBorder::Arrow kHorizontalArrow = BubbleBorder::TOP_CENTER;

[msw, 2014/08/11 22:26:32]

nit: inline these constants, they're only used once each.

[bruthig, 2014/08/19 19:02:13]

Done.

+  const BubbleBorder::Arrow kVerticalArrow = BubbleBorder::LEFT_CENTER;
 
-  // The height should be much bigger then the requested size + border and
-  // padding since it needs to be able to include the tip bitmap.
-  gfx::Rect visible_tip_1 = b1.GetBounds(anchor, contents);
-  EXPECT_GE(visible_tip_1.height(), 30);
-  EXPECT_LE(visible_tip_1.width(), 30);
+  const gfx::Size kSmallSize = gfx::Size(1, 1);
+  const gfx::Size kMediumSize = gfx::Size(50, 50);
 
-  // With the tip being invisible the height should now be much smaller.
-  b1.set_paint_arrow(BubbleBorder::PAINT_TRANSPARENT);
-  gfx::Rect invisible_tip_1 = b1.GetBounds(anchor, contents);
-  EXPECT_LE(invisible_tip_1.height(), 30);
-  EXPECT_LE(invisible_tip_1.width(), 30);
+  BubbleBorderGetBoundsSizeTest() {}
+  virtual ~BubbleBorderGetBoundsSizeTest() {}
 
-  // When the orientation of the tip changes, the above mentioned tests need to
-  // be reverse for width and height.
-  BubbleBorder b2(BubbleBorder::TOP_RIGHT, BubbleBorder::NO_SHADOW, 0);
+  virtual gfx::Size GetSize(gfx::Size contents_size) {

[msw, 2014/08/11 22:26:33]

Make these member functions protected and non-virtual.

[bruthig, 2014/08/19 19:02:13]

Done.

+    DCHECK(bubble_border_);

[msw, 2014/08/11 22:26:32]

Remove DCHECKs before accessing the member.

[bruthig, 2014/08/19 19:02:13]

Done.

+    return bubble_border_->GetBounds(gfx::Rect(100, 100, 0, 0),
+                                     contents_size).size();
+  }
 
-  // The width should be much bigger then the requested size + border and
-  // padding since it needs to be able to include the tip bitmap.
-  gfx::Rect visible_tip_2 = b2.GetBounds(anchor, contents);
-  EXPECT_GE(visible_tip_2.width(), 30);
-  EXPECT_LE(visible_tip_2.height(), 30);
+  virtual void CreateBubbleBorder(BubbleBorder::Arrow arrow) {
+    bubble_border_.reset(new BubbleBorder(arrow,
+                                          BubbleBorder::NO_SHADOW,
+                                          0 /* color */));

[msw, 2014/08/11 22:26:33]

nit: just use SK_ColorWHITE or similar and remove the comment.

[bruthig, 2014/08/19 19:02:13]

Done.

+  }
 
-  // With the tip being invisible the width should now be much smaller.
-  b2.set_paint_arrow(BubbleBorder::PAINT_TRANSPARENT);
-  gfx::Rect invisible_tip_2 = b2.GetBounds(anchor, contents);
-  EXPECT_LE(invisible_tip_2.width(), 30);
-  EXPECT_LE(invisible_tip_2.height(), 30);
+  virtual bool HasArrow() {
+    DCHECK(bubble_border_);
+    return BubbleBorder::has_arrow(bubble_border_->arrow());
+  }
+
+  virtual void SetPaintArrow(BubbleBorder::ArrowPaintType value) {
+    DCHECK(bubble_border_);
+    bubble_border_->set_paint_arrow(value);
+  }
+
+ private:
+  scoped_ptr<BubbleBorder> bubble_border_;
+
+  DISALLOW_COPY_AND_ASSIGN(BubbleBorderGetBoundsSizeTest);
+};
+
+TEST_F(BubbleBorderGetBoundsSizeTest, WithHorizontalArrow) {
+  CreateBubbleBorder(kHorizontalArrow);
+
+  ASSERT_TRUE(HasArrow());
+
+  SetPaintArrow(BubbleBorder::PAINT_NORMAL);
+  EXPECT_EQ(gfx::Size(45, 29), GetSize(kSmallSize));

[msw, 2014/08/11 22:26:32]

These values won't be robust to bubble asset changes, etc. Can you break them up
into components that would be easier to update, then ensure that the resulting
sizes match some combination, like:
  const int kBorder = x;
  ...
  EXPECT_EQ(2 * kBorder + kSmallSize.width(), GetSize(kSmallSize).width())

[bruthig, 2014/08/19 19:02:13]

Done.

+  EXPECT_EQ(gfx::Size(60, 67), GetSize(kMediumSize));
+
+  SetPaintArrow(BubbleBorder::PAINT_TRANSPARENT);
+  EXPECT_EQ(gfx::Size(45, 29), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 67), GetSize(kMediumSize));
+
+  SetPaintArrow(BubbleBorder::PAINT_NONE);
+  EXPECT_EQ(gfx::Size(22, 22), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 60), GetSize(kMediumSize));
 }
 
+TEST_F(BubbleBorderGetBoundsSizeTest, WithVerticalArrow) {
+  CreateBubbleBorder(kVerticalArrow);
+
+  ASSERT_TRUE(HasArrow());
+
+  SetPaintArrow(BubbleBorder::PAINT_NORMAL);
+  EXPECT_EQ(gfx::Size(29, 45), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(67, 60), GetSize(kMediumSize));
+
+  SetPaintArrow(BubbleBorder::PAINT_TRANSPARENT);
+  EXPECT_EQ(gfx::Size(29, 45), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(67, 60), GetSize(kMediumSize));
+
+  SetPaintArrow(BubbleBorder::PAINT_NONE);
+  EXPECT_EQ(gfx::Size(22, 22), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 60), GetSize(kMediumSize));
+}
+
+TEST_F(BubbleBorderGetBoundsSizeTest, WithoutArrow) {
+  CreateBubbleBorder(BubbleBorder::NONE);
+
+  ASSERT_FALSE(HasArrow());
+
+  SetPaintArrow(BubbleBorder::PAINT_NORMAL);
+  EXPECT_EQ(gfx::Size(22, 22), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 60), GetSize(kMediumSize));
+
+  SetPaintArrow(BubbleBorder::PAINT_TRANSPARENT);
+  EXPECT_EQ(gfx::Size(22, 22), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 60), GetSize(kMediumSize));
+
+  SetPaintArrow(BubbleBorder::PAINT_NONE);
+  EXPECT_EQ(gfx::Size(22, 22), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 60), GetSize(kMediumSize));
+}
+
+TEST_F(BubbleBorderGetBoundsSizeTest, WithFloatArrow) {
+  CreateBubbleBorder(BubbleBorder::FLOAT);
+
+  ASSERT_FALSE(HasArrow());
+
+  SetPaintArrow(BubbleBorder::PAINT_NORMAL);
+  EXPECT_EQ(gfx::Size(22, 22), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 60), GetSize(kMediumSize));
+
+  SetPaintArrow(BubbleBorder::PAINT_TRANSPARENT);
+  EXPECT_EQ(gfx::Size(22, 22), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 60), GetSize(kMediumSize));
+
+  SetPaintArrow(BubbleBorder::PAINT_NONE);
+  EXPECT_EQ(gfx::Size(22, 22), GetSize(kSmallSize));
+  EXPECT_EQ(gfx::Size(60, 60), GetSize(kMediumSize));
+}
+
+class BubbleBorderGetBoundsOriginTest : public views::ViewsTestBase {
+ public:
+  const gfx::Rect kEmptyAnchor = gfx::Rect(100, 100, 0, 0);
+  const gfx::Rect kNonEmptyAnchor = gfx::Rect(100, 100, 20, 20);
+
+  gfx::Size kContentsSize = gfx::Size(50, 50);
+
+  BubbleBorderGetBoundsOriginTest() {}
+  virtual ~BubbleBorderGetBoundsOriginTest() {}
+
+  virtual void CreateBubbleBorder(BubbleBorder::Arrow arrow) {
+    bubble_border_.reset(new BubbleBorder(arrow,
+                                          BubbleBorder::NO_SHADOW,
+                                          0 /* color */));
+  }
+
+  virtual void SetAlignment(BubbleBorder::BubbleAlignment alignment) {
+    DCHECK(bubble_border_);
+    bubble_border_->set_alignment(alignment);
+  }
+
+  virtual gfx::Point GetOrigin(gfx::Rect anchor, gfx::Size contents_size) {

[msw, 2014/08/11 22:26:33]

nit: remove |contents_size| and send |kContentsSize| to GetBounds.

[bruthig, 2014/08/19 19:02:13]

Done.

+    DCHECK(bubble_border_);
+    return bubble_border_->GetBounds(anchor, contents_size).origin();
+  }
+
+ private:
+  scoped_ptr<BubbleBorder> bubble_border_;
+
+  DISALLOW_COPY_AND_ASSIGN(BubbleBorderGetBoundsOriginTest);
+};
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsHorizontalLeft) {
+  CreateBubbleBorder(BubbleBorder::TOP_LEFT);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(78, 96), GetOrigin(kEmptyAnchor, kContentsSize));

[msw, 2014/08/11 22:26:33]

Ditto; can you make this more robust with something like:
  const int kAnchorOffset = x;
  ...
  EXPECT_EQ(kAnchorOffset + kEmptyAnchor.x(), GetOrigin(kEmptyAnchor).x());

[bruthig, 2014/08/19 19:02:13]

Done.

+  EXPECT_EQ(gfx::Point(88, 116), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(96, 96), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(96, 116), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsHorizontalCenter) {
+  CreateBubbleBorder(BubbleBorder::TOP_CENTER);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(70, 96), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(80, 116), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(70, 96), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(80, 116), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsHorizontalRight) {
+  CreateBubbleBorder(BubbleBorder::TOP_RIGHT);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(62, 96), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(72, 116), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(44, 96), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(64, 116), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsHorizontalBottom) {
+  CreateBubbleBorder(BubbleBorder::BOTTOM_LEFT);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(78, 37), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(88, 37), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(96, 37), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(96, 37), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsVerticalTop) {
+  CreateBubbleBorder(BubbleBorder::LEFT_TOP);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(96, 78), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(116, 88), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(96, 96), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(116, 96), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsVerticalCenter) {
+  CreateBubbleBorder(BubbleBorder::LEFT_CENTER);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(96, 70), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(116, 80), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(96, 70), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(116, 80), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsVerticalBottom) {
+  CreateBubbleBorder(BubbleBorder::LEFT_BOTTOM);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(96, 62), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(116, 72), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(96, 44), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(116, 64), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsVerticalRight) {
+  CreateBubbleBorder(BubbleBorder::RIGHT_CENTER);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(37, 70), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(37, 80), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(37, 70), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(37, 80), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsNone) {
+  CreateBubbleBorder(BubbleBorder::NONE);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(70, 100), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(80, 120), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(70, 100), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(80, 120), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
+
+TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsFloat) {
+  CreateBubbleBorder(BubbleBorder::FLOAT);
+
+  SetAlignment(BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR);
+  EXPECT_EQ(gfx::Point(70, 70), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(80, 80), GetOrigin(kNonEmptyAnchor, kContentsSize));
+
+  SetAlignment(BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
+  EXPECT_EQ(gfx::Point(70, 70), GetOrigin(kEmptyAnchor, kContentsSize));
+  EXPECT_EQ(gfx::Point(80, 80), GetOrigin(kNonEmptyAnchor, kContentsSize));
+}
 
 }  // namespace views