Makes vertical alignment of location bar bubbles same

ui/views/bubble/bubble_border.cc

Index: ui/views/bubble/bubble_border.cc
diff --git a/ui/views/bubble/bubble_border.cc b/ui/views/bubble/bubble_border.cc
index ec7fc07d6b88580ca1d918c6b0d74d9e7ddfff92..dff39de19ae18c3673434a541047cea427037db0 100644
--- a/ui/views/bubble/bubble_border.cc
+++ b/ui/views/bubble/bubble_border.cc
@@ -164,8 +164,12 @@ gfx::Rect BubbleBorder::GetBounds(const gfx::Rect& anchor_rect,
   int h = anchor_rect.height();
   const gfx::Size size(GetSizeForContentsSize(contents_size));
   const int arrow_offset = GetArrowOffset(size);
-  const int arrow_size =
+  int arrow_shift =
       images_->arrow_interior_thickness + kStroke - images_->arrow_thickness;

[Evan Stade, 2016/03/18 01:49:21]

took me a good while to figure out what this is. It's actually the size of the
shadow I guess? Any way we can use variable names to make that more clear?

Is the += just below equivalent to

arrow_shift = images_->arrow_thickness;

?

[varkha, 2016/03/18 04:14:41]

The image we use for the arrow (I am taking the arrow used with the content
bubbles that is used with TOP_RIGHT arrow as an example here) contains 3
elements:
 - Top blank space (shadow)
 - upward pointing arrow tip (|arrow_interior_thickness| tall, think of it as 6
pixels)
 - horizontal line ("stroke" - |kStroke| thick line, 1 pixel)
The whole image is |arrow_thickness| tall (think of it as 10 pixels).
For the TOP_RIGHT arrow type we want to align the tip of the arrow with the
anchor bottom which means we need to shift the BubbleBorder bounds up by the
shadow thickness. Shadow thickness is |arrow_thickness - kStroke -
arrow_internal_thickness|. The shift is negative that which is the calculation
in line 168. The previous name |arrow_size| was totally confusing, I have
changed it to arrow_shift, I have added a comment but I prefer "shift" to
referring to image elements because the shift can be positive or negative
depending on arrow paint type. I hope the comment makes it a bit less confusing.
I don't think so. With the numbers as in my example above (which happen to be
the numbers I see in the debug traces) I get arrow_shift = (6 + 1 - 10) + 6 = -3
+ 6 = 3. With your suggestion I would get arrow_shift = 10 which would place it
in a wrong place.

+  // When arrow is painted transparently the visible border of the bubble needs
+  // to be positioned at the same bounds as when the arrow is shown.
+  if (arrow_paint_type_ == PAINT_TRANSPARENT)
+    arrow_shift += images_->arrow_interior_thickness;

[msw, 2016/03/18 02:02:52]

Hmm, I don't understand why adding the interior thickness again works as
intended, shouldn't this instead only subtract images_->arrow_thickness above if
arrow_paint_type_ != PAINT_TRANSPARENT? Perhaps manually test this change (in
views_examples?) with each of the BubbleBorder::Shadow types. I wonder if the
bubble placements with bottom/right arrows aren't correct.

[varkha, 2016/03/18 04:14:41]

No. I am not logically adding it again (although the math works this way), but
rather shifting down the bounds by the visible height of the visible arrow tip
which is |arrow_internal_thickness|. Please see the comment above for what I
think those mean.

Consider that the height of the bubble with TRANSPARENT arrow (with this CL) is
same as the height with NONE arrow - instead of reserving space for the arrow
and not drawing it I now simply don't reserve the space. This meant that to
maintain the same visible bounds for the TRANSPARENT case I had to shift the
bounds down by |arrow_internal_thickness|. Effectively the change in this file
is pixel-neutral but allows clicks just above the bubble to reach the elements
under the click.

For simplicity all my comments refer to bubble placement below the anchor but of
course I have checked the bottom/right placements as well as top and left and
they seem right to me (and I've double checked the math here). I have also added
the unit test and it checks that the bubble is where I expect it to be with all
permutations of arrow type and alignment. That test should be easy to reason
about.

   const bool mid_anchor = alignment_ == ALIGN_ARROW_TO_MID_ANCHOR;
 
   // Calculate the bubble coordinates based on the border and arrow settings.
@@ -178,9 +182,11 @@ gfx::Rect BubbleBorder::GetBounds(const gfx::Rect& anchor_rect,
       x += mid_anchor ? w / 2 + arrow_offset - size.width() :
                         w - size.width() + GetBorderThickness() - kStroke;
     }
-    y += is_arrow_on_top(arrow_) ? h + arrow_size : -arrow_size - size.height();
+    y += is_arrow_on_top(arrow_) ? h + arrow_shift
+                                 : -arrow_shift - size.height();
   } else if (has_arrow(arrow_)) {
-    x += is_arrow_on_left(arrow_) ? w + arrow_size : -arrow_size - size.width();
+    x += is_arrow_on_left(arrow_) ? w + arrow_shift
+                                  : -arrow_shift - size.width();
     if (is_arrow_on_top(arrow_)) {
       y += mid_anchor ? h / 2 - arrow_offset : kStroke - GetBorderThickness();
     } else if (is_arrow_at_center(arrow_)) {
@@ -253,7 +259,7 @@ void BubbleBorder::Paint(const views::View& view, gfx::Canvas* canvas) {
 gfx::Insets BubbleBorder::GetInsets() const {
   // The insets contain the stroke and shadow pixels outside the bubble fill.
   const int inset = GetBorderThickness();
-  if ((arrow_paint_type_ == PAINT_NONE) || !has_arrow(arrow_))
+  if (arrow_paint_type_ != PAINT_NORMAL || !has_arrow(arrow_))
     return gfx::Insets(inset);
 
   int first_inset = inset;
@@ -279,14 +285,16 @@ gfx::Size BubbleBorder::GetSizeForContentsSize(
 
   // Ensure the bubble is large enough to not overlap border and arrow images.
   const int min = 2 * images_->border_thickness;
+  // Only take arrow image sizes into account when the bubble tip is shown.
+  if (arrow_paint_type_ != PAINT_NORMAL || !has_arrow(arrow_)) {
+    size.SetToMax(gfx::Size(min, min));
+    return size;
+  }
   const int min_with_arrow_width = min + images_->arrow_width;
   const int min_with_arrow_thickness = images_->border_thickness +
       std::max(images_->arrow_thickness + images_->border_interior_thickness,
                images_->border_thickness);
-  // Only take arrow image sizes into account when the bubble tip is shown.
-  if (arrow_paint_type_ == PAINT_NONE || !has_arrow(arrow_))
-    size.SetToMax(gfx::Size(min, min));
-  else if (is_arrow_on_horizontal(arrow_))
+  if (is_arrow_on_horizontal(arrow_))
     size.SetToMax(gfx::Size(min_with_arrow_width, min_with_arrow_thickness));
   else
     size.SetToMax(gfx::Size(min_with_arrow_thickness, min_with_arrow_width));