Makes vertical alignment of location bar bubbles same

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 19f6968fba5b2dc47bf3f9e79af0ccbde618b3a9..b45e9a80ab4fcce0ef72dd5f16d0dd2e87243467 100644
--- a/ui/views/bubble/bubble_border_unittest.cc
+++ b/ui/views/bubble/bubble_border_unittest.cc
@@ -309,7 +309,7 @@ TEST_F(BubbleBorderTest, GetSizeForContentsSizeTest) {
               border.GetSizeForContentsSize(cases[i].content));
 
     border.set_paint_arrow(BubbleBorder::PAINT_TRANSPARENT);
-    EXPECT_EQ(cases[i].expected_with_arrow,
+    EXPECT_EQ(cases[i].expected_without_arrow,
               border.GetSizeForContentsSize(cases[i].content));
 
     border.set_paint_arrow(BubbleBorder::PAINT_NONE);
@@ -347,62 +347,136 @@ TEST_F(BubbleBorderTest, GetBoundsOriginTest) {
   const int kArrowOffsetForNotCenter =
       kImages->border_thickness + (kImages->arrow_width / 2);
 
-  const int kArrowSize =
-      kImages->arrow_interior_thickness + BubbleBorder::kStroke -
-          kImages->arrow_thickness;
+  const int kArrowShift = kImages->arrow_interior_thickness +

[Peter Kasting, 2016/03/17 06:16:15]

We have a lot of code below this point that explicitly computes all the shift
values and specifies them for every case.

I think we could ditch all of this, and return to something like the original
code, and simply calculate these on the fly within the loop.  Basically, we know
what the delta is between a shift for a visible arrow and an invisible arrow; in
the loop we would look at the arrow position and then add this delta to the
appropriate value from the with-arrow amount.  Etc.

[varkha, 2016/03/18 01:21:13]

Yes, this is indeed shorter and made it easier to reason. Thanks!

+                          BubbleBorder::kStroke - kImages->arrow_thickness;
+  const int kNoArrowShift = kArrowShift + kImages->arrow_interior_thickness;
 
-  const int kTopHorizArrowY = kAnchor.y() + kAnchor.height() + kArrowSize;
+  const int kTopHorizArrowY = kAnchor.y() + kAnchor.height() + kArrowShift;
   const int kBottomHorizArrowY =
-      kAnchor.y() - kArrowSize - kTotalSizeWithHorizArrow.height();
-
-  const int kLeftVertArrowX = kAnchor.x() + kAnchor.width() + kArrowSize;
+      kAnchor.y() - kArrowShift - kTotalSizeWithHorizArrow.height();
+  const int kLeftVertArrowX = kAnchor.x() + kAnchor.width() + kArrowShift;
   const int kRightVertArrowX =
-      kAnchor.x() - kArrowSize - kTotalSizeWithVertArrow.width();
+      kAnchor.x() - kArrowShift - kTotalSizeWithVertArrow.width();
+
+  const int kTopHorizTransparentArrowY =
+      kAnchor.y() + kAnchor.height() + kNoArrowShift;
+  const int kBottomHorizTransparentArrowY =
+      kAnchor.y() - kNoArrowShift - kTotalSizeWithNoArrow.height();
+  const int kLeftVertTransparentArrowX =
+      kAnchor.x() + kAnchor.width() + kNoArrowShift;
+  const int kRightVertTransparentArrowX =
+      kAnchor.x() - kNoArrowShift - kTotalSizeWithNoArrow.width();
+
+  const int kTopHorizNoArrowY = kAnchor.y() + kAnchor.height() + kArrowShift;
+  const int kBottomHorizNoArrowY =
+      kAnchor.y() - kArrowShift - kTotalSizeWithNoArrow.height();
+  const int kLeftVertNoArrowX = kAnchor.x() + kAnchor.width() + kArrowShift;
+  const int kRightVertNoArrowX =
+      kAnchor.x() - kArrowShift - kTotalSizeWithNoArrow.width();
 
   struct TestCase {
     BubbleBorder::Arrow arrow;
     BubbleBorder::BubbleAlignment alignment;
     int expected_x;
     int expected_y;
+    int expected_with_transparent_arrow_x;
+    int expected_with_transparent_arrow_y;
+    int expected_without_arrow_x;
+    int expected_without_arrow_y;
   };
 
   TestCase cases[] = {
-    // Horizontal arrow tests.
-    { BubbleBorder::TOP_LEFT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
-      kAnchor.CenterPoint().x() - kArrowOffsetForNotCenter, kTopHorizArrowY },
-    { BubbleBorder::TOP_LEFT, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
-      kAnchor.x() + BubbleBorder::kStroke - kBorderThickness, kTopHorizArrowY },
-    { BubbleBorder::TOP_CENTER, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
-      kAnchor.CenterPoint().x() - kArrowOffsetForHorizCenter, kTopHorizArrowY },
-    { BubbleBorder::BOTTOM_RIGHT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
-      kAnchor.CenterPoint().x() + kArrowOffsetForNotCenter -
-          kTotalSizeWithHorizArrow.width(), kBottomHorizArrowY },
-    { BubbleBorder::BOTTOM_RIGHT, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
-      kAnchor.x() + kAnchor.width() - kTotalSizeWithHorizArrow.width() +
-          kBorderThickness - BubbleBorder::kStroke, kBottomHorizArrowY },
-
-    // Vertical arrow tests.
-    { BubbleBorder::LEFT_TOP, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
-      kLeftVertArrowX, kAnchor.CenterPoint().y() - kArrowOffsetForNotCenter },
-    { BubbleBorder::LEFT_TOP, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
-      kLeftVertArrowX, kAnchor.y() + BubbleBorder::kStroke - kBorderThickness },
-    { BubbleBorder::LEFT_CENTER, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
-      kLeftVertArrowX, kAnchor.CenterPoint().y() - kArrowOffsetForVertCenter },
-    { BubbleBorder::RIGHT_BOTTOM, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
-      kRightVertArrowX, kAnchor.CenterPoint().y() + kArrowOffsetForNotCenter -
-          kTotalSizeWithVertArrow.height() },
-    { BubbleBorder::RIGHT_BOTTOM, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
-      kRightVertArrowX, kAnchor.y() + kAnchor.height() -
-          kTotalSizeWithVertArrow.height() + kBorderThickness -
-          BubbleBorder::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 },
+      // Horizontal arrow tests.
+      {BubbleBorder::TOP_LEFT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+       kAnchor.CenterPoint().x() - kArrowOffsetForNotCenter, kTopHorizArrowY,
+       kAnchor.CenterPoint().x() - kArrowOffsetForNotCenter,
+       kTopHorizTransparentArrowY,
+       kAnchor.CenterPoint().x() - kArrowOffsetForNotCenter, kTopHorizNoArrowY},
+      {BubbleBorder::TOP_LEFT, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
+       kAnchor.x() + BubbleBorder::kStroke - kBorderThickness, kTopHorizArrowY,
+       kAnchor.x() + BubbleBorder::kStroke - kBorderThickness,
+       kTopHorizTransparentArrowY,
+       kAnchor.x() + BubbleBorder::kStroke - kBorderThickness,
+       kTopHorizNoArrowY},
+      {BubbleBorder::TOP_CENTER, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+       kAnchor.CenterPoint().x() - kArrowOffsetForHorizCenter, kTopHorizArrowY,
+       kAnchor.CenterPoint().x() - kArrowOffsetForHorizCenter,
+       kTopHorizTransparentArrowY,
+       kAnchor.CenterPoint().x() - kArrowOffsetForHorizCenter,
+       kTopHorizNoArrowY},
+      {BubbleBorder::BOTTOM_RIGHT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+       kAnchor.CenterPoint().x() + kArrowOffsetForNotCenter -
+           kTotalSizeWithHorizArrow.width(),
+       kBottomHorizArrowY,
+       kAnchor.CenterPoint().x() + kArrowOffsetForNotCenter -
+           kTotalSizeWithHorizArrow.width(),
+       kBottomHorizTransparentArrowY,
+       kAnchor.CenterPoint().x() + kArrowOffsetForNotCenter -
+           kTotalSizeWithHorizArrow.width(),
+       kBottomHorizNoArrowY},
+      {BubbleBorder::BOTTOM_RIGHT, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
+       kAnchor.x() + kAnchor.width() - kTotalSizeWithHorizArrow.width() +
+           kBorderThickness - BubbleBorder::kStroke,
+       kBottomHorizArrowY,
+       kAnchor.x() + kAnchor.width() - kTotalSizeWithHorizArrow.width() +
+           kBorderThickness - BubbleBorder::kStroke,
+       kBottomHorizTransparentArrowY,
+       kAnchor.x() + kAnchor.width() - kTotalSizeWithHorizArrow.width() +
+           kBorderThickness - BubbleBorder::kStroke,
+       kBottomHorizNoArrowY},
+
+      // Vertical arrow tests.
+      {BubbleBorder::LEFT_TOP, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+       kLeftVertArrowX, kAnchor.CenterPoint().y() - kArrowOffsetForNotCenter,
+       kLeftVertTransparentArrowX,
+       kAnchor.CenterPoint().y() - kArrowOffsetForNotCenter, kLeftVertNoArrowX,
+       kAnchor.CenterPoint().y() - kArrowOffsetForNotCenter},
+      {BubbleBorder::LEFT_TOP, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
+       kLeftVertArrowX, kAnchor.y() + BubbleBorder::kStroke - kBorderThickness,
+       kLeftVertTransparentArrowX,
+       kAnchor.y() + BubbleBorder::kStroke - kBorderThickness,
+       kLeftVertNoArrowX,
+       kAnchor.y() + BubbleBorder::kStroke - kBorderThickness},
+      {BubbleBorder::LEFT_CENTER, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+       kLeftVertArrowX, kAnchor.CenterPoint().y() - kArrowOffsetForVertCenter,
+       kLeftVertTransparentArrowX,
+       kAnchor.CenterPoint().y() - kArrowOffsetForVertCenter, kLeftVertNoArrowX,
+       kAnchor.CenterPoint().y() - kArrowOffsetForVertCenter},
+      {BubbleBorder::RIGHT_BOTTOM, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+       kRightVertArrowX, kAnchor.CenterPoint().y() + kArrowOffsetForNotCenter -
+                             kTotalSizeWithVertArrow.height(),
+       kRightVertTransparentArrowX,
+       kAnchor.CenterPoint().y() + kArrowOffsetForNotCenter -
+           kTotalSizeWithVertArrow.height(),
+       kRightVertNoArrowX,
+       kAnchor.CenterPoint().y() + kArrowOffsetForNotCenter -
+           kTotalSizeWithVertArrow.height()},
+      {BubbleBorder::RIGHT_BOTTOM, BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE,
+       kRightVertArrowX,
+       kAnchor.y() + kAnchor.height() - kTotalSizeWithVertArrow.height() +
+           kBorderThickness - BubbleBorder::kStroke,
+       kRightVertTransparentArrowX,
+       kAnchor.y() + kAnchor.height() - kTotalSizeWithVertArrow.height() +
+           kBorderThickness - BubbleBorder::kStroke,
+       kRightVertNoArrowX,
+       kAnchor.y() + kAnchor.height() - kTotalSizeWithVertArrow.height() +
+           kBorderThickness - BubbleBorder::kStroke},
+
+      // No arrow tests.
+      {BubbleBorder::NONE, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
+       kAnchor.x() + (kAnchor.width() - kTotalSizeWithNoArrow.width()) / 2,
+       kAnchor.y() + kAnchor.height(),
+       kAnchor.x() + (kAnchor.width() - kTotalSizeWithNoArrow.width()) / 2,
+       kAnchor.y() + kAnchor.height(),
+       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,
+       kAnchor.x() + (kAnchor.width() - kTotalSizeWithNoArrow.width()) / 2,
+       kAnchor.y() + (kAnchor.height() - kTotalSizeWithNoArrow.height()) / 2,
+       kAnchor.x() + (kAnchor.width() - kTotalSizeWithNoArrow.width()) / 2,
+       kAnchor.y() + (kAnchor.height() - kTotalSizeWithNoArrow.height()) / 2},
   };
 
   for (size_t i = 0; i < arraysize(cases); ++i) {
@@ -411,9 +485,20 @@ TEST_F(BubbleBorderTest, GetBoundsOriginTest) {
     border.set_arrow(cases[i].arrow);
     border.set_alignment(cases[i].alignment);
 
+    border.set_paint_arrow(BubbleBorder::PAINT_NORMAL);
     gfx::Point origin = border.GetBounds(kAnchor, kContentSize).origin();
     EXPECT_EQ(cases[i].expected_x, origin.x());
     EXPECT_EQ(cases[i].expected_y, origin.y());
+
+    border.set_paint_arrow(BubbleBorder::PAINT_TRANSPARENT);
+    origin = border.GetBounds(kAnchor, kContentSize).origin();
+    EXPECT_EQ(cases[i].expected_with_transparent_arrow_x, origin.x());
+    EXPECT_EQ(cases[i].expected_with_transparent_arrow_y, origin.y());
+
+    border.set_paint_arrow(BubbleBorder::PAINT_NONE);
+    origin = border.GetBounds(kAnchor, kContentSize).origin();
+    EXPECT_EQ(cases[i].expected_without_arrow_x, origin.x());
+    EXPECT_EQ(cases[i].expected_without_arrow_y, origin.y());
   }
 }