Makes vertical alignment of location bar bubbles same

ui/views/bubble/bubble_border_unittest.cc

 // 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.
 
 #include "ui/views/bubble/bubble_border.h"
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 291 matching lines @@
     SCOPED_TRACE(base::StringPrintf("i=%d arrow=%d",
         static_cast<int>(i), cases[i].arrow));
 
     border.set_arrow(cases[i].arrow);
 
     border.set_paint_arrow(BubbleBorder::PAINT_NORMAL);
     EXPECT_EQ(cases[i].expected_with_arrow,
               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);
     EXPECT_EQ(cases[i].expected_without_arrow,
               border.GetSizeForContentsSize(cases[i].content));
   }
 }
 
 TEST_F(BubbleBorderTest, GetBoundsOriginTest) {
   views::BubbleBorder border(BubbleBorder::TOP_LEFT,
@@ skipping 17 matching lines @@
   const gfx::Size kTotalSizeWithNoArrow =
       border.GetSizeForContentsSize(kContentSize);
 
   const int kBorderThickness = border.GetBorderThickness();
 
   const int kArrowOffsetForHorizCenter = kTotalSizeWithHorizArrow.width() / 2;
   const int kArrowOffsetForVertCenter = kTotalSizeWithVertArrow.height() / 2;
   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();
+      kAnchor.y() - kArrowShift - kTotalSizeWithHorizArrow.height();
+  const int kLeftVertArrowX = kAnchor.x() + kAnchor.width() + kArrowShift;
+  const int kRightVertArrowX =
+      kAnchor.x() - kArrowShift - kTotalSizeWithVertArrow.width();
 
-  const int kLeftVertArrowX = kAnchor.x() + kAnchor.width() + kArrowSize;
-  const int kRightVertArrowX =
-      kAnchor.x() - kArrowSize - 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 },
+      // 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 },
-    { 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 },
+      // 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() },
-    { BubbleBorder::FLOAT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR,
-      kAnchor.x() + (kAnchor.width() - kTotalSizeWithNoArrow.width()) / 2,
-      kAnchor.y() + (kAnchor.height() - kTotalSizeWithNoArrow.height()) / 2 },
+      // 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) {
     SCOPED_TRACE(base::StringPrintf("i=%d arrow=%d alignment=%d",
         static_cast<int>(i), cases[i].arrow, cases[i].alignment));
     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());
   }
 }
 
 // Ensure all the shadow types pass some size validation and paint sanely.
 TEST_F(BubbleBorderTest, ShadowTypes) {
   const gfx::Rect rect(0, 0, 320, 200);
   View paint_view;
   paint_view.SetBoundsRect(rect);
 
   for (int i = 0; i < BubbleBorder::SHADOW_COUNT; ++i) {
@@ skipping 13 matching lines @@
     EXPECT_GE(border_images->border_thickness,
               border_images->border_interior_thickness);
 
     // For a TOP_LEFT arrow, the x-offset always matches the border thickness.
     EXPECT_EQ(border.GetArrowRect(rect).x(), border_images->border_thickness);
     border.Paint(paint_view, &canvas);
   }
 }
 
 }  // namespace views