Adjust positioning of location bar icons.

chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc

Index: chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
diff --git a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
index 9d1ff7d07306d10813cb6f2165dd99d895bf867c..3a8c06d2be22f147ddf50c015e47b3e083379694 100644
--- a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
+++ b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/views/location_bar/icon_label_bubble_view.h"
 
+#include "chrome/browser/ui/layout_constants.h"
+#include "chrome/browser/ui/views/location_bar/background_with_1_px_border.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/gfx/canvas.h"
@@ -94,8 +96,8 @@ bool IconLabelBubbleView::OnKeyReleased(const ui::KeyEvent& event) {
 
 void IconLabelBubbleView::Layout() {
   // Compute the image bounds. Leading and trailing padding are the same.
-  int image_x = LocationBarView::kHorizontalPadding;
-  int bubble_trailing_padding = image_x;
+  int image_x = 0;

[Peter Kasting, 2017/01/24 21:26:39]

Doesn't this effectively make |image_x| 0 everywhere below, and thus no longer
necessary?

[Evan Stade, 2017/01/25 00:37:49]

I guess it does. I was worried that width() - image_preferred_width could be <
0, but it's not the case when !ShouldShowLabel.

+  int bubble_trailing_padding = GetPostSeparatorPadding();
 
   // If ShouldShowLabel() is true, then either we show a label in the
   // steady state, or we're not yet in the last portion of the animation.  In
@@ -105,7 +107,7 @@ void IconLabelBubbleView::Layout() {
   // it.  In these cases, we want to shrink the trailing padding first, so the
   // image slides all the way to the trailing edge before slowing or stopping;
   // then we want to shrink the leading padding down to zero.

[Peter Kasting, 2017/01/24 21:26:40]

This code, and the code in GetSizeForLabelWidth(), were very tricky to write
correctly, and assumed various things about where there would be padding in all
cases.  I don't have this paged in well enough to catch subtle errors here; the
biggest concern I have is with code that is now effectively dead and could be
simplified (a la the |image_x| comment above), but other issues might be
possible, e.g. incorrectly adding padding between the image and label compared
to today.

Please be very careful that all these things are correct, not just in terms of
their output, but that code structure/comments are still right.

[Evan Stade, 2017/01/25 00:37:49]

I too was afraid of touching this more than I had to, but I've tried to simplify
it and verify visually (with slowed down animation) as well as with the existing
unit tests.

-  const int image_preferred_width = image_->GetPreferredSize().width();
+  const int image_preferred_width = GetImageViewSize().width();
   if (!ShouldShowLabel()) {
     image_x = std::min(image_x, width() - image_preferred_width);
     bubble_trailing_padding = std::min(
@@ -170,7 +172,7 @@ SkColor IconLabelBubbleView::GetParentBackgroundColor() const {
 }
 
 gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int label_width) const {
-  gfx::Size size(image_->GetPreferredSize());
+  gfx::Size size(GetImageViewSize());
   const bool shrinking = IsShrinking();
   // Animation continues for the last few pixels even after the label is not
   // visible in order to slide the icon into its final position. Therefore it
@@ -192,9 +194,8 @@ gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int label_width) const {
     // enough to show the icon. We don't want to shrink all the way back to
     // zero, since this would mean the view would completely disappear and then
     // pop back to an icon after the animation finishes.
-    const int max_width = LocationBarView::kHorizontalPadding +
-                          image_->GetPreferredSize().width() +
-                          GetInternalSpacing() + label_width + post_label_width;
+    const int max_width = GetImageViewSize().width() + GetInternalSpacing() +
+                          label_width + post_label_width;
     const int current_width = WidthMultiplier() * max_width;
     size.set_width(shrinking ? std::max(current_width, size.width())
                              : current_width);
@@ -203,14 +204,16 @@ gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int label_width) const {
 }
 
 int IconLabelBubbleView::GetInternalSpacing() const {
-  return image_->GetPreferredSize().IsEmpty()
+  return GetImageViewSize().IsEmpty()
              ? 0
-             : LocationBarView::kHorizontalPadding;
+             : GetLayoutConstant(LOCATION_BAR_ELEMENT_PADDING);
 }
 
 int IconLabelBubbleView::GetPostSeparatorPadding() const {
-  // The location bar will add LocationBarView::kHorizontalPadding after us.
-  return kSpaceBesideSeparator - LocationBarView::kHorizontalPadding;
+  // The location bar will add LOCATION_BAR_ELEMENT_PADDING after us.
+  return kSpaceBesideSeparator -
+         GetLayoutConstant(LOCATION_BAR_ELEMENT_PADDING) -
+         next_element_interior_padding_;
 }
 
 float IconLabelBubbleView::GetScaleFactor() const {
@@ -221,6 +224,14 @@ float IconLabelBubbleView::GetScaleFactor() const {
   return compositor ? compositor->device_scale_factor() : 1.0f;
 }
 
+gfx::Size IconLabelBubbleView::GetImageViewSize() const {
+  gfx::Rect image_rect(image_->GetPreferredSize());
+  if (image_rect.IsEmpty())
+    return gfx::Size();
+  image_rect.Inset(-gfx::Insets(LocationBarView::kIconInteriorPadding));

[Peter Kasting, 2017/01/24 21:26:40]

Nit: Simpler:

  if (!image_rect.IsEmpty())
    image_rect.Inset(-gfx::Insets(LocationBarView::kIconInteriorPadding));

(No need for early return)

[Evan Stade, 2017/01/25 00:37:49]

Done.

+  return image_rect.size();
+}
+
 const char* IconLabelBubbleView::GetClassName() const {
   return "IconLabelBubbleView";
 }