Adjust positioning of location bar icons.

chrome/browser/ui/views/location_bar/icon_label_bubble_view.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 "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"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/scoped_canvas.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/animation/ink_drop_highlight.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/widget/widget.h"
@@ skipping 70 matching lines @@
 }
 
 bool IconLabelBubbleView::OnKeyReleased(const ui::KeyEvent& event) {
   if (event.key_code() == ui::VKEY_SPACE)
     return OnActivate(event);
   return false;
 }
 
 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
   // these cases, we leave the leading and trailing padding alone.  If this is
   // false, however, then we're only showing the image, and either the view
   // width is the image width, or it's animating downwards and getting close to
   // 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(
         bubble_trailing_padding, width() - image_preferred_width - image_x);
   }
 
   // Now that we've computed the padding values, give the image all the
   // remaining width.  This will be less than the image's preferred width during
   // the first portion of the animation; during the very beginning there may not
   // be enough room to show the image at all.
@@ skipping 44 matching lines @@
 SkColor IconLabelBubbleView::GetInkDropBaseColor() const {
   return color_utils::DeriveDefaultIconColor(GetTextColor());
 }
 
 SkColor IconLabelBubbleView::GetParentBackgroundColor() const {
   return GetNativeTheme()->GetSystemColor(
       ui::NativeTheme::kColorId_TextfieldDefaultBackground);
 }
 
 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
   // is necessary to animate |total_width| even when the background is hidden
   // as long as the animation is still shrinking.
   if (ShouldShowLabel() || shrinking) {
     // On scale factors < 2, we reserve 1 DIP for the 1 px separator.  For
     // higher scale factors, we simply take the separator px out of the
     // kSpaceBesideSeparator region before the separator, as that results in a
     // width closer to the desired gap than if we added a whole DIP for the
     // separator px.  (For scale 2, the two methods have equal error: 1 px.)
     const int separator_width = (GetScaleFactor() >= 2) ? 0 : 1;
     const int post_label_width =
         (kSpaceBesideSeparator + separator_width + GetPostSeparatorPadding());
 
     // |multiplier| grows from zero to one, stays equal to one and then shrinks
     // to zero again. The view width should correspondingly grow from zero to
     // fully showing both label and icon, stay there, then shrink to just large
     // 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);
   }
   return size;
 }
 
 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 {
   const views::Widget* widget = GetWidget();
   // There may be no widget in tests, and in ash there may be no compositor if
   // the native view of the Widget doesn't have a parent.
   const ui::Compositor* compositor = widget ? widget->GetCompositor() : nullptr;
   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";
 }
 
 void IconLabelBubbleView::OnPaint(gfx::Canvas* canvas) {
   if (!ShouldShowLabel())
     return;
 
   const SkColor plain_text_color = GetNativeTheme()->GetSystemColor(
       ui::NativeTheme::kColorId_TextfieldDefaultColor);
   const SkColor separator_color = SkColorSetA(
       plain_text_color, color_utils::IsDark(plain_text_color) ? 0x59 : 0xCC);
 
   gfx::Rect bounds(GetLocalBounds());
   const int kSeparatorHeight = 16;
   bounds.Inset(GetPostSeparatorPadding(),
                (bounds.height() - kSeparatorHeight) / 2);
 
   // Draw the 1 px separator.
   gfx::ScopedCanvas scoped_canvas(canvas);
   const float scale = canvas->UndoDeviceScaleFactor();
   // Keep the separator aligned on a pixel center.
   const gfx::RectF pixel_aligned_bounds =
       gfx::ScaleRect(gfx::RectF(bounds), scale) - gfx::Vector2dF(0.5f, 0);
   canvas->DrawLine(pixel_aligned_bounds.top_right(),
                    pixel_aligned_bounds.bottom_right(), separator_color);
 }