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"
 
 namespace {
 
 // Amount of space on either side of the separator that appears after the label.
 constexpr int kSpaceBesideSeparator = 8;
 
 }  // namespace
 
 IconLabelBubbleView::IconLabelBubbleView(const gfx::FontList& font_list,
                                          bool elide_in_middle)
     : image_(new views::ImageView()),
       label_(new views::Label(base::string16(), font_list)) {
   // Disable separate hit testing for |image_|.  This prevents views treating
   // |image_| as a separate mouse hover region from |this|.
   image_->set_interactive(false);
+  image_->SetBorder(views::CreateEmptyBorder(
+      gfx::Insets(LocationBarView::kIconInteriorPadding)));
   AddChildView(image_);
 
   label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
 
   if (elide_in_middle)
     label_->SetElideBehavior(gfx::ELIDE_MIDDLE);
   AddChildView(label_);
 
   // Bubbles are given the full internal height of the location bar so that all
   // child views in the location bar have the same height. The visible height of
@@ skipping 44 matching lines @@
   return false;
 }
 
 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;
-
-  // 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.
-  const int image_preferred_width = image_->GetPreferredSize().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);
+  // When the view is expanding, if we don't have horizontal room for both the
+  // image and the trailing padding, we take space away from the image. When the
+  // view is contracting, instead we whittle away at the trailing padding.
+  int bubble_trailing_padding = GetPostSeparatorPadding();
+  int image_width = image_->GetPreferredSize().width();
+  const int space_shortage = image_width + bubble_trailing_padding - width();
+  if (space_shortage > 0) {
+    if (ShouldShowLabel())

[Peter Kasting, 2017/01/25 04:25:18]

The comment above seems to imply this should check IsShrinking() instead.  I
suspect the code here is correct and it's the comment that needs to be clarified
(replace bits about the view expanding/contracting with comments about trying to
show the label).

[Evan Stade, 2017/01/26 00:52:59]

thinking of it in terms of expanding vs contracting seemed clearer to me, but
I've updated the comment to cover the cases where there's no current movement
("steady-state").

[Peter Kasting, 2017/01/26 01:25:13]

The reason I'm anxious about referring to expanding and contracting without
going further and explaining the whole animation is that ShouldShowLabel() is a
virtual method whose implementation is potentially arbitrary (and is not
equivalent to !IsShrinking()).

So for example, if we're animating the label closed but it's still partly
visible, then IsShrinking() is true, ShouldShowLabel() is true, the view is
contracting, and we're not in the hidden-label steady state.  Both the old and
the new comments are unclear at this point.

It seems like either we could provide more detail about all the different
animation phases (a la the old comments) or we could try to change the code and
function names so that things are clearer, but I"m not sure how.

This is making me wonder if ShouldShowLabel() and !IsShrinking() are actually
the same in the case where space_shortage > 0 (even though they're not always
the same.  If they are, and we replaced the former with the latter, we could use
your original comment, and we could maybe simplify ShouldShowLabel().  But I'm
not convinced they are.

+      image_width -= space_shortage;
+    else
+      bubble_trailing_padding -= space_shortage;
   }
-
-  // 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.
-  const int image_width =
-      std::min(image_preferred_width,
-               std::max(0, width() - image_x - bubble_trailing_padding));
-  image_->SetBounds(image_x, 0, image_width, height());
+  image_->SetBounds(0, 0, image_width, height());
 
   // Compute the label bounds.  The label gets whatever size is left over after
   // accounting for the preferred image width and padding amounts.  Note that if
   // the label has zero size it doesn't actually matter what we compute its X
   // value to be, since it won't be visible.
-  const int label_x = image_x + image_width + GetInternalSpacing();
+  const int label_x = image_->bounds().right() + GetInternalSpacing();
   const int label_width =
       std::max(0, width() - label_x - bubble_trailing_padding);
   label_->SetBounds(label_x, 0, label_width, height());
 }
 
 void IconLabelBubbleView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   label_->GetAccessibleNodeData(node_data);
 }
 
 void IconLabelBubbleView::OnNativeThemeChanged(
@@ skipping 46 matching lines @@
     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() +
+    const int max_width = image_->GetPreferredSize().width() +

[Peter Kasting, 2017/01/25 04:25:18]

Nit: Use |size|?

[Evan Stade, 2017/01/26 00:52:59]

Done.

                           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()
              ? 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;
 }
 
 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());
+  gfx::Rect bounds(label_->bounds());
   const int kSeparatorHeight = 16;
-  bounds.Inset(GetPostSeparatorPadding(),
-               (bounds.height() - kSeparatorHeight) / 2);
+  bounds.Inset(0, (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);
 }