Fix padding in location bar bubbles to match specs on bug.

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 "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/location_bar/background_with_1_px_border.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_utils.h"
+#include "ui/gfx/image/image_util.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/animation/ink_drop_hover.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/painter.h"
 
 namespace {
 
 SkColor CalculateImageColor(gfx::ImageSkia* image) {
   // We grab the color of the middle pixel of the image, which we treat as
   // the representative color of the entire image (reasonable, given the current
   // appearance of these assets).
   const SkBitmap& bitmap(image->GetRepresentation(1.0f).sk_bitmap());
   SkAutoLockPixels pixel_lock(bitmap);
   return bitmap.getColor(bitmap.width() / 2, bitmap.height() / 2);
 }
 
 }  // namespace
 
 IconLabelBubbleView::IconLabelBubbleView(int contained_image,
                                          const gfx::FontList& font_list,
                                          SkColor parent_background_color,
                                          bool elide_in_middle)
     : background_painter_(nullptr),
       image_(new views::ImageView()),
       label_(new views::Label(base::string16(), font_list)),
+      builtin_leading_padding_(0),
+      builtin_trailing_padding_(0),
       is_extension_icon_(false),
       parent_background_color_(parent_background_color) {
   if (contained_image) {
     image_->SetImage(
         ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
             contained_image));
   }
 
   // Disable separate hit testing for |image_|.  This prevents views treating
   // |image_| as a separate mouse hover region from |this|.
@@ skipping 38 matching lines @@
   should_show_background_ = false;
   SetLabelBackgroundColor(SK_ColorTRANSPARENT);
 }
 
 void IconLabelBubbleView::SetLabel(const base::string16& label) {
   label_->SetText(label);
 }
 
 void IconLabelBubbleView::SetImage(const gfx::ImageSkia& image_skia) {
   image_->SetImage(image_skia);
+
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    gfx::GetVisibleMargins(image_skia, &builtin_leading_padding_,
+                           &builtin_trailing_padding_);
+    if (base::i18n::IsRTL())
+      std::swap(builtin_leading_padding_, builtin_trailing_padding_);
+  }
 }
 
 bool IconLabelBubbleView::ShouldShowBackground() const {
   return should_show_background_;
 }
 
 double IconLabelBubbleView::WidthMultiplier() const {
   return 1.0;
 }
 
 bool IconLabelBubbleView::IsShrinking() const {
   return false;
 }
 
-int IconLabelBubbleView::GetImageAndPaddingWidth() const {
-  const int image_width = image_->GetPreferredSize().width();
-  return image_width ? (image_width + GetInternalSpacing()) : 0;
-}
-
 gfx::Size IconLabelBubbleView::GetPreferredSize() const {
   // Height will be ignored by the LocationBarView.
   return GetSizeForLabelWidth(label_->GetPreferredSize().width());
 }
 
 void IconLabelBubbleView::Layout() {
   // Compute the image bounds.  In non-MD, the leading padding depends on
   // whether this is an extension icon, since extension icons and
   // Chrome-provided icons are different sizes.  In MD, these sizes are the
   // same, so it's not necessary to handle the two types differently.
@@ skipping 24 matching lines @@
   // 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());
 
   // 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 + GetImageAndPaddingWidth();
+  const int label_x =
+      image_x + image_width + (image_width ? GetInternalSpacing() : 0);
   const int label_width =
       std::max(0, width() - label_x - bubble_trailing_padding);
   label_->SetBounds(label_x, 0, label_width, height());
 }
 
 void IconLabelBubbleView::OnNativeThemeChanged(
     const ui::NativeTheme* native_theme) {
   label_->SetEnabledColor(GetTextColor());
 
   if (!ui::MaterialDesignController::IsModeMaterial())
@@ skipping 56 matching lines @@
     if (shrinking)
       total_width = std::max(total_width, image_width);
     size.set_width(total_width);
   }
   return size;
 }
 
 int IconLabelBubbleView::GetOuterPadding(bool leading) const {
   if (ui::MaterialDesignController::IsModeMaterial()) {
     // Leading and trailing padding are equal.
-    return GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING);
+    return GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING) -
+           (leading ? builtin_leading_padding_ : 0);

[varkha, 2016/03/30 04:04:07]

Wouldn't that increase the visible trailing padding compared to what it was
before this CL because the constants have grown but you are not subtracting a
built-in padding in the trailing case?

[Peter Kasting, 2016/03/30 04:10:55]

In some cases, yes; this looks more consistent (the content settings padding now
matches the other bubbles) and IMO better.  Sebastien agreed in principle, I
promised him pictures of the changes.

[varkha, 2016/03/30 04:35:10]

So in the popup animated bubble the visible left padding before the icon will be
smaller than the trailing padding after the label?   

[Peter Kasting, 2016/03/30 04:44:57]

No.  The visible padding will be equal.  The point of subtracting out this
value, in fact, is to make these values appear equal.

In the blocked popup animation, they already would have, since that uses a
full-width icon.  That's not true of the EV/keyword search bubbles.

[varkha, 2016/03/30 05:21:58]

OK, but in general case if builtin_*_padding_ values are not zero would you not
end in a state when visible margins around the icon are same (leading padding
and spacing) but the padding right of the label is larger?

[Peter Kasting, 2016/03/30 05:33:37]

No.

ICON_LABEL_VIEW_TRAILING_PADDING is the number of pixels to the left of the
apparent edge of the icon, and to the right of the label.  Since there's only
one constant in MD, these two values are always the same.

If the icon actually has n px of space on its left edge, then if we were to use
the constant's value for both layout values, the apparent leading padding would
be n px larger than the label's trailing padding.  By subtracting n, we ensure
the apparent padding is always exactly the same as the post-label padding.

Note that the space between icon and label is completely distinct from the above
-- it's based on a different constant (ICON_LABEL_VIEW_INTERNAL_SPACING) and
subtracts a different built-in padding value (builtin_trailing_padding_).  So if
you're mentally assuming this value is the same as the padding to the left of
the icon, you're not thinking about the situation correctly.

[varkha, 2016/03/30 06:17:29]

I see now, thanks for the explanation. I guess I was confused because this CL
will actually change the trailing padding to be a bit larger and the leading
visible padding to be exactly same as the trailing padding.

   }
 
   return GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING) -
          GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
          (leading ? 0 : GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING));
 }
 
 void IconLabelBubbleView::SetLabelBackgroundColor(
     SkColor chip_background_color) {
   // The background images are painted atop |parent_background_color_|.
   // Alpha-blend |chip_background_color| with |parent_background_color_| to
   // determine the actual color the label text will sit atop.
   // Tricky bit: We alpha blend an opaque version of |chip_background_color|
   // against |parent_background_color_| using the original image grid color's
   // alpha. This is because AlphaBlend(a, b, 255) always returns |a| unchanged
   // even if |a| is a color with non-255 alpha.
   label_->SetBackgroundColor(color_utils::AlphaBlend(
       SkColorSetA(chip_background_color, 255), GetParentBackgroundColor(),
       SkColorGetA(chip_background_color)));
 }
 
 int IconLabelBubbleView::GetInternalSpacing() const {
-  return GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_SPACING);
+  return GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_SPACING) -
+         builtin_trailing_padding_;
 }
 
 const char* IconLabelBubbleView::GetClassName() const {
   return "IconLabelBubbleView";
 }
 
 void IconLabelBubbleView::OnPaint(gfx::Canvas* canvas) {
   if (!ShouldShowBackground())
     return;
   if (background_painter_) {
     views::Painter::PaintPainterAt(canvas, background_painter_.get(),
                                    GetContentsBounds());
   }
   if (background())
     background()->Paint(canvas, this);
 }