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..9b78f768eff43a771d23337376e9b28dd6d94d48 100644
--- a/ui/views/bubble/bubble_border_unittest.cc
+++ b/ui/views/bubble/bubble_border_unittest.cc
@@ -4,11 +4,24 @@
 
 #include "ui/views/bubble/bubble_border.h"
 
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/stringprintf.h"
 #include "ui/views/test/views_test_base.h"
 
 namespace views {
 
-typedef ViewsTestBase BubbleBorderTest;
+class BubbleBorderTest : public views::ViewsTestBase {

[msw, 2014/08/22 20:54:45]

Change this subclass to a typedef.

[bruthig, 2014/08/22 22:55:03]

Done.

+ public:
+  const int kStroke = views::BubbleBorder::kStroke;

[msw, 2014/08/22 20:54:45]

Have GetBoundsOriginTest grab this instead.

[bruthig, 2014/08/22 22:55:03]

Done.

+
+  BubbleBorderTest() {}
+  virtual ~BubbleBorderTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BubbleBorderTest);
+};
+
+typedef BubbleBorderTest BubbleBorderGetBoundsOriginTest;

[msw, 2014/08/22 20:54:45]

Remove this.

[bruthig, 2014/08/22 22:55:03]

Done.

 
 TEST_F(BubbleBorderTest, GetMirroredArrow) {
   // Horizontal mirroring.
@@ -202,39 +215,274 @@ 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);
-
-  // 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);
-
-  // 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);
-
-  // 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);
-
-  // 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);
-
-  // 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);
+TEST_F(BubbleBorderTest, GetSizeForContentsSizeTest) {
+  views::BubbleBorder bb(BubbleBorder::NONE,

[msw, 2014/08/22 20:54:45]

nit: abbreviations are discouraged, consider |border|.

[bruthig, 2014/08/22 22:55:03]

Done.

+                         BubbleBorder::NO_SHADOW,
+                         SK_ColorWHITE);
+
+  const views::internal::BorderImages* kImages = bb.GetImagesForTest();
+
+  const gfx::Size kSmallSize = gfx::Size(1, 1);
+  const gfx::Size kMediumSize = gfx::Size(50, 50);
+  const gfx::Rect kAnchor = gfx::Rect(100, 100, 20, 20);
+
+  const gfx::Size kSmallHorizArrow(
+      2 * kImages->border_thickness + kImages->top_arrow.width(),
+      kImages->border_thickness + kImages->arrow_thickness +
+        kImages->border_interior_thickness);

[msw, 2014/08/22 20:54:45]

nit: indent two more spaces.

[bruthig, 2014/08/22 22:55:04]

Done.

+
+  const gfx::Size kSmallVertArrow(

[msw, 2014/08/22 20:54:45]

nit: Since the content sizes are square, could this be simplified as
kSmallVertArrow(kSmallHorizArrow.height(), kSmallHorizArrow.width()) (or just
share one expected size and flip the width/height expectations based on
is_arrow_on_horizontal). Ditto for Medium.

[bruthig, 2014/08/22 22:55:03]

Actually I meant to change those sizes so they aren't square so as to avoid
errors in the test.  Fixed now and found 1 error!

Simplifying only works for the kSmall case because the contents size is too
small and a minimum value is used.

+      kImages->border_thickness + kImages->arrow_thickness
+        + kImages->border_interior_thickness,

[msw, 2014/08/22 20:54:45]

nit: move '+' to line above, indent two more spaces.

[bruthig, 2014/08/22 22:55:03]

Done.

+      2 * kImages->border_thickness + kImages->top_arrow.width());
+
+  const gfx::Size kSmallNoArrow(2 * kImages->border_thickness,
+                                2 * kImages->border_thickness);
+
+  const gfx::Size kMediumHorizArrow(
+      kMediumSize.width() + 2 * bb.GetBorderThickness(),

[msw, 2014/08/22 20:54:45]

Are the medium/small calculations different because the smaller bubble's content
doesn't actually contribute to the bubble size? Can you add an short comment?

[bruthig, 2014/08/22 22:55:03]

Done.

+      kMediumSize.height() + bb.GetBorderThickness()
+        + kImages->arrow_thickness);

[msw, 2014/08/22 20:54:45]

nit: move '+' to line above, indent two more spaces.

[bruthig, 2014/08/22 22:55:03]

Done.

+
+  const gfx::Size kMediumVertArrow(
+      kMediumSize.width() + bb.GetBorderThickness() + kImages->arrow_thickness,
+      kMediumSize.height() + 2 * bb.GetBorderThickness());
+
+  const gfx::Size kMediumNoArrow(
+      kMediumSize.width() + 2 * bb.GetBorderThickness(),
+      kMediumSize.width() + 2 * bb.GetBorderThickness());
+
+  struct TestCase {
+    BubbleBorder::Arrow arrow;
+    bool has_arrow;

[msw, 2014/08/22 20:54:44]

Remove this and the check; BubbleBorderTest.HasArrow covers this.

[bruthig, 2014/08/22 22:55:03]

Done.

+    BubbleBorder::ArrowPaintType paint_arrow;
+    gfx::Size content;
+    gfx::Size expected;
+  };
+
+  TestCase cases[] = {
+    // Content size: kSmallSize
+
+    // BubbleBorder::Arrow::TOP_CENTER

[msw, 2014/08/22 20:54:44]

nit: Remove these comments; they aren't as helpful as the others "// Content
size: kSmallSize".

[bruthig, 2014/08/22 22:55:03]

Done.

+    {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_NORMAL,

[msw, 2014/08/22 20:54:44]

nit: Sorry for iterating on my earlier suggested design, but it'd be better to
make a test case simply contain the two expected sizes for the three paint
types, and have the loop below check all three paint types for each test case.
For example:
  { BubbleBorder::TOP_CENTER, kSmallSize, kSmallHorizArrow, kSmallNoArrow },
  { BubbleBorder::BOTTOM_CENTER, kSmallSize, kSmallHorizArrow, kSmallNoArrow },
  { BubbleBorder::LEFT_TOP, kSmallSize, kSmallVertArrow, kSmallNoArrow },
  ...
  for (size_t i = 0; i < arraysize(cases); ++i) {
    ...
    bb.set_paint_arrow(BubbleBorder::PAINT_NORMAL);
    EXPECT_EQ(cases[i].expected_with_arrow, 
              bb.GetSizeForContentsSize(cases[i].content));
    bb.set_paint_arrow(BubbleBorder::PAINT_TRANSPARENT);
    EXPECT_EQ(cases[i].expected_with_arrow, 
              bb.GetSizeForContentsSize(cases[i].content));
    bb.set_paint_arrow(BubbleBorder::PAINT_NONE);
    EXPECT_EQ(cases[i].expected_without_arrow, 
              bb.GetSizeForContentsSize(cases[i].content));

[bruthig, 2014/08/22 22:55:03]

Done.

+        kSmallSize, kSmallHorizArrow},
+    {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_TRANSPARENT,

[msw, 2014/08/22 20:54:45]

nit: use spaces inside of curly braces.

[bruthig, 2014/08/22 22:55:04]

Done.

+        kSmallSize, kSmallHorizArrow},
+    {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_NONE,
+        kSmallSize, kSmallNoArrow},
+
+    // BubbleBorder::Arrow::BOTTOM_CENTER
+    {BubbleBorder::BOTTOM_CENTER, true, BubbleBorder::PAINT_NORMAL,
+        kSmallSize, kSmallHorizArrow},
+    {BubbleBorder::BOTTOM_CENTER, true, BubbleBorder::PAINT_TRANSPARENT,
+        kSmallSize, kSmallHorizArrow},
+    {BubbleBorder::BOTTOM_CENTER, true, BubbleBorder::PAINT_NONE,
+        kSmallSize, kSmallNoArrow},
+
+    // BubbleBorder::Arrow::LEFT_TOP
+    {BubbleBorder::LEFT_TOP, true, BubbleBorder::PAINT_NORMAL,
+        kSmallSize, kSmallVertArrow},
+    {BubbleBorder::LEFT_TOP, true, BubbleBorder::PAINT_TRANSPARENT,
+        kSmallSize, kSmallVertArrow},
+    {BubbleBorder::LEFT_TOP, true, BubbleBorder::PAINT_NONE,
+        kSmallSize, kSmallNoArrow},
+
+    // BubbleBorder::Arrow::RIGHT_TOP
+    {BubbleBorder::RIGHT_TOP, true, BubbleBorder::PAINT_NORMAL,
+        kSmallSize, kSmallVertArrow},
+    {BubbleBorder::RIGHT_TOP, true, BubbleBorder::PAINT_TRANSPARENT,
+        kSmallSize, kSmallVertArrow},
+    {BubbleBorder::RIGHT_TOP, true, BubbleBorder::PAINT_NONE,
+        kSmallSize, kSmallNoArrow},
+
+    // BubbleBorder::Arrow::NONE
+    {BubbleBorder::NONE, false, BubbleBorder::PAINT_NORMAL,
+        kSmallSize, kSmallNoArrow},
+    {BubbleBorder::NONE, false, BubbleBorder::PAINT_TRANSPARENT,
+        kSmallSize, kSmallNoArrow},
+    {BubbleBorder::NONE, false, BubbleBorder::PAINT_NONE,
+        kSmallSize, kSmallNoArrow},
+
+    // BubbleBorder::Arrow::FLOAT
+    {BubbleBorder::FLOAT, false, BubbleBorder::PAINT_NORMAL,
+        kSmallSize, kSmallNoArrow},
+    {BubbleBorder::FLOAT, false, BubbleBorder::PAINT_TRANSPARENT,
+        kSmallSize, kSmallNoArrow},
+    {BubbleBorder::FLOAT, false, BubbleBorder::PAINT_NONE,
+        kSmallSize, kSmallNoArrow},
+
+    // Content size: kMediumSize
+
+    // BubbleBorder::Arrow::TOP_CENTER
+    {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_NORMAL,
+        kMediumSize, kMediumHorizArrow},
+    {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_TRANSPARENT,
+        kMediumSize, kMediumHorizArrow},
+    {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_NONE,
+        kMediumSize, kMediumNoArrow},
+
+    // BubbleBorder::Arrow::BOTTOM_CENTER
+    {BubbleBorder::BOTTOM_CENTER, true, BubbleBorder::PAINT_NORMAL,
+        kMediumSize, kMediumHorizArrow},
+    {BubbleBorder::BOTTOM_CENTER, true, BubbleBorder::PAINT_TRANSPARENT,
+        kMediumSize, kMediumHorizArrow},
+    {BubbleBorder::BOTTOM_CENTER, true, BubbleBorder::PAINT_NONE,
+        kMediumSize, kMediumNoArrow},
+
+    // BubbleBorder::Arrow::LEFT_TOP
+    {BubbleBorder::LEFT_TOP, true, BubbleBorder::PAINT_NORMAL,
+        kMediumSize, kMediumVertArrow},
+    {BubbleBorder::LEFT_TOP, true, BubbleBorder::PAINT_TRANSPARENT,
+        kMediumSize, kMediumVertArrow},
+    {BubbleBorder::LEFT_TOP, true, BubbleBorder::PAINT_NONE,
+        kMediumSize, kMediumNoArrow},
+
+    // BubbleBorder::Arrow::RIGHT_TOP
+    {BubbleBorder::RIGHT_TOP, true, BubbleBorder::PAINT_NORMAL,
+        kMediumSize, kMediumVertArrow},
+    {BubbleBorder::RIGHT_TOP, true, BubbleBorder::PAINT_TRANSPARENT,
+        kMediumSize, kMediumVertArrow},
+    {BubbleBorder::RIGHT_TOP, true, BubbleBorder::PAINT_NONE,
+        kMediumSize, kMediumNoArrow},
+
+    // BubbleBorder::Arrow::NONE
+    {BubbleBorder::NONE, false, BubbleBorder::PAINT_NORMAL,
+        kMediumSize, kMediumNoArrow},
+    {BubbleBorder::NONE, false, BubbleBorder::PAINT_TRANSPARENT,
+        kMediumSize, kMediumNoArrow},
+    {BubbleBorder::NONE, false, BubbleBorder::PAINT_NONE,
+        kMediumSize, kMediumNoArrow},
+
+    // BubbleBorder::Arrow::FLOAT
+    {BubbleBorder::FLOAT, false, BubbleBorder::PAINT_NORMAL,
+        kMediumSize, kMediumNoArrow},
+    {BubbleBorder::FLOAT, false, BubbleBorder::PAINT_TRANSPARENT,
+        kMediumSize, kMediumNoArrow},
+    {BubbleBorder::FLOAT, false, BubbleBorder::PAINT_NONE,
+        kMediumSize, kMediumNoArrow}
+  };
+
+  for (size_t i = 0; i < arraysize(cases); ++i) {
+    SCOPED_TRACE(base::StringPrintf("i=%d arrow=%d paint_arrow=%d",
+        static_cast<int>(i), cases[i].arrow, cases[i].paint_arrow));
+
+    bb.set_arrow(cases[i].arrow);
+    ASSERT_EQ(cases[i].has_arrow, BubbleBorder::has_arrow(bb.arrow()));
+    bb.set_paint_arrow(cases[i].paint_arrow);
+
+    EXPECT_EQ(cases[i].expected,
+              bb.GetSizeForContentsSize(cases[i].content));
+  }
 }
 
+TEST_F(BubbleBorderTest, GetBoundsOriginTest) {
+  views::BubbleBorder bb(BubbleBorder::TOP_LEFT,

[msw, 2014/08/22 20:54:45]

ditto nit: |border|

[bruthig, 2014/08/22 22:55:03]

Done.

+                         BubbleBorder::NO_SHADOW,
+                         SK_ColorWHITE);
+
+  const gfx::Rect kAnchor(100, 100, 20, 20);
+  const gfx::Size kContentSize(50, 50);
+
+  const views::internal::BorderImages* kImages = bb.GetImagesForTest();
+
+  bb.set_arrow(BubbleBorder::TOP_LEFT);
+  const gfx::Size kTotalSizeWithHorizArrow =
+      bb.GetSizeForContentsSize(kContentSize);
+
+  bb.set_arrow(BubbleBorder::RIGHT_BOTTOM);
+  const gfx::Size kTotalSizeWithVertArrow =
+      bb.GetSizeForContentsSize(kContentSize);
+
+  bb.set_arrow(BubbleBorder::NONE);
+  const gfx::Size kTotalSizeWithNoArrow =
+      bb.GetSizeForContentsSize(kContentSize);
+
+  const int kBorderThickness = bb.GetBorderThickness();
+
+  const int kMidAnchorWidth = kAnchor.width() / 2;
+  const int kMidAnchorHeight = kAnchor.height() / 2;
+
+  const int kArrowOffsetForHorizCentre = kTotalSizeWithHorizArrow.width() / 2;

[msw, 2014/08/22 20:54:45]

nit: use the EN-US spelling "Center"

[bruthig, 2014/08/22 22:55:03]

Done.

+  const int kArrowOffsetForVertCentre = kTotalSizeWithVertArrow.height() / 2;
+  const int kArrowOffsetForNotCentre =
+      kImages->border_thickness + (kImages->top_arrow.width() / 2);
+
+  const int kArrowSize =
+      kImages->arrow_interior_thickness + kStroke - kImages->arrow_thickness;
+
+  const int kTopHorizArrowY = kAnchor.y() + kAnchor.height() + kArrowSize;
+  const int kBottomHorizArrowY =
+      kAnchor.y() - kArrowSize - kTotalSizeWithHorizArrow.height();
+
+  const int kLeftVertArrowX = kAnchor.x() + kAnchor.width() + kArrowSize;
+  const int kRightVertArrowX =
+      kAnchor.x() - kArrowSize - kTotalSizeWithVertArrow.width();
+
+  struct TestCase {
+    BubbleBorder::Arrow arrow;
+    BubbleBorder::BubbleAlignment alignment;
+    int expectedX;
+    int expectedY;
+  };
+
+  TestCase cases[] = {

[msw, 2014/08/22 20:54:45]

Ditto, if you see some way to simplify this test, it'd be appreciated.

[bruthig, 2014/08/22 22:55:03]

The existing tests exhaust all the code paths through the function and are not
exhaustive of all the input parameters.  I can't think of a way to simplify it
unless I add all input parameter combinations.  See new patch for example.

+    // Horizontal arrow tests.
+    {BubbleBorder::TOP_LEFT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,

[msw, 2014/08/22 20:54:44]

ditto nit: spaces inside braces.

[bruthig, 2014/08/22 22:55:03]

Done.

+        kAnchor.x() + kMidAnchorWidth - kArrowOffsetForNotCentre,
+        kTopHorizArrowY},
+    {BubbleBorder::TOP_LEFT, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
+        kAnchor.x() + kStroke - kBorderThickness,
+        kTopHorizArrowY},
+    {BubbleBorder::TOP_CENTER, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+        kAnchor.x() + kMidAnchorWidth - kArrowOffsetForHorizCentre,
+        kTopHorizArrowY},
+    {BubbleBorder::BOTTOM_RIGHT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+        kAnchor.x() + kMidAnchorWidth + kArrowOffsetForNotCentre
+            - kTotalSizeWithHorizArrow.width(),
+        kBottomHorizArrowY},
+    {BubbleBorder::BOTTOM_RIGHT, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
+        kAnchor.x() + kAnchor.width() - kTotalSizeWithHorizArrow.width()
+            + kBorderThickness - kStroke,
+        kBottomHorizArrowY},
+
+    // Vertical arrow tests.
+    {BubbleBorder::LEFT_TOP, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+        kLeftVertArrowX,
+        kAnchor.y() + kMidAnchorHeight - kArrowOffsetForNotCentre},
+    {BubbleBorder::LEFT_TOP, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
+        kLeftVertArrowX,
+        kAnchor.y() + kStroke - kBorderThickness},
+    {BubbleBorder::LEFT_CENTER, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+        kLeftVertArrowX,
+        kAnchor.y() + kMidAnchorHeight - kArrowOffsetForVertCentre},
+    {BubbleBorder::RIGHT_BOTTOM, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+        kRightVertArrowX,
+        kAnchor.y() + kMidAnchorHeight + kArrowOffsetForNotCentre
+            - kTotalSizeWithVertArrow.height()},
+    {BubbleBorder::RIGHT_BOTTOM, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
+        kRightVertArrowX,
+        kAnchor.y() + kAnchor.height() - kTotalSizeWithVertArrow.height()
+            + kBorderThickness - kStroke},
+
+    // No arrow tests.
+    {BubbleBorder::NONE, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+        kAnchor.x() + (kAnchor.width() - kTotalSizeWithNoArrow.width()) / 2,
+        kAnchor.y() + kAnchor.height()},
+    {BubbleBorder::FLOAT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+        kAnchor.x() + (kAnchor.width() - kTotalSizeWithNoArrow.width()) / 2,
+        kAnchor.y() + (kAnchor.height() - kTotalSizeWithNoArrow.height()) / 2},
+  };
+
+  for (size_t i = 0; i < arraysize(cases); ++i) {
+    SCOPED_TRACE(base::StringPrintf("i=%d arrow=%d alignment=%d",
+        static_cast<int>(i), cases[i].arrow, cases[i].alignment));
+    bb.set_arrow(cases[i].arrow);
+    bb.set_alignment(cases[i].alignment);
+
+    gfx::Point origin = bb.GetBounds(kAnchor, kContentSize).origin();
+    EXPECT_EQ(cases[i].expectedX, origin.x());
+    EXPECT_EQ(cases[i].expectedY, origin.y());
+  }
+}
 
 }  // namespace views