Update omnibox chips in MD

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/gfx/scoped_canvas.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/controls/textfield/textfield.h"
 #include "ui/views/painter.h"
 
 namespace {
 
+// Extra padding to place at the end of the chip when the label is showing.
+const int kExtraTrailingPadding = 7;
+
 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|.
   image_->set_interactive(false);
   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
   // the bubble should be smaller, so use an empty border to shrink down the
   // content bounds so the background gets painted correctly.
-  const int padding = GetLayoutConstant(LOCATION_BAR_BUBBLE_VERTICAL_PADDING);
-  SetBorder(
-      views::Border::CreateEmptyBorder(gfx::Insets(padding, 0, padding, 0)));
+  SetBorder(views::Border::CreateEmptyBorder(
+      gfx::Insets(GetLayoutConstant(LOCATION_BAR_BUBBLE_VERTICAL_PADDING), 0)));
+
+  // Flip the canvas in MD RTL so the separator is drawn on the correct side.
+  EnableCanvasFlippingForRTLUI(ui::MaterialDesignController::IsModeMaterial());
 }
 
 IconLabelBubbleView::~IconLabelBubbleView() {
 }
 
 void IconLabelBubbleView::SetBackgroundImageGrid(
     const int background_images[]) {
   should_show_background_ = true;
   if (!ui::MaterialDesignController::IsModeMaterial()) {
     background_painter_.reset(
@@ skipping 12 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;
 }
 
@@ skipping 64 matching lines @@
   const int label_x = image_x + image_width + GetInternalSpacing();
   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())
-    return;
-
-  bool inverted = color_utils::IsDark(GetParentBackgroundColor());
-  SkColor border_color = inverted ? SK_ColorWHITE : GetBorderColor();
-  SkColor background_color =
-      inverted ? SK_ColorWHITE : SkColorSetA(border_color, 0x13);
-  set_background(new BackgroundWith1PxBorder(background_color, border_color));
-  SetLabelBackgroundColor(background_color);
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    label_->SetBackgroundColor(GetParentBackgroundColor());
+    SchedulePaint();
+  }
 }
 
 void IconLabelBubbleView::AddInkDropLayer(ui::Layer* ink_drop_layer) {
   image()->SetPaintToLayer(true);
   image()->layer()->SetFillsBoundsOpaquely(false);
   InkDropHostView::AddInkDropLayer(ink_drop_layer);
 }
 
 void IconLabelBubbleView::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {
   InkDropHostView::RemoveInkDropLayer(ink_drop_layer);
@@ skipping 18 matching lines @@
 }
 
 gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int label_width) const {
   gfx::Size size(image_->GetPreferredSize());
   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 (ShouldShowBackground() || shrinking) {
+    const int extra_trailing_padding =
+        ui::MaterialDesignController::IsModeMaterial() ? kExtraTrailingPadding
+                                                       : 0;
     // |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 = MinimumWidthForImageWithBackgroundShown() +
-                          GetInternalSpacing() + label_width;
+                          GetInternalSpacing() + label_width +
+                          extra_trailing_padding;
     const int current_width = WidthMultiplier() * max_width;
     size.set_width(shrinking ? std::max(current_width, size.width())
                              : current_width);
   }
   return size;
 }
 
 int IconLabelBubbleView::MinimumWidthForImageWithBackgroundShown() const {
   return GetOuterPadding(true) + image_->GetPreferredSize().width() +
          GetOuterPadding(false);
 }
 
 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::GetOuterPadding(bool leading) const {
-  if (ui::MaterialDesignController::IsModeMaterial()) {
-    // The apparent leading and trailing padding should be equal, so we need to
-    // subtract the amount of built-in padding in the image.  This will mean
-    // that the actual padding + the padding inside the image add up to the same
-    // amount of padding as on the trailing edge of the bubble.
-    return GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING) -
-           (leading ? builtin_leading_padding_ : 0);
-  }
+  if (ui::MaterialDesignController::IsModeMaterial())
+    return GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING);
 
   return GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING) -
          GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
          (leading ? 0 : GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING));
 }
 
 int IconLabelBubbleView::GetInternalSpacing() const {
   return image_->GetPreferredSize().IsEmpty()
              ? 0
-             : (GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_SPACING) -
-                builtin_trailing_padding_);
+             : GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_SPACING);
 }
 
 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);
+
+  // In MD, draw a separator and not a background.
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    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);
+
+    // Amount of padding that is built into the whatever view comes after this
+    // one in the location bar (e.g. omnibox textfield).
+    int post_chip_spacing =
+        GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING);
+    // Amount of space to leave after the separator (dp).
+    const float kPostSeparatorSpacing =
+        (GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING) +
+         kExtraTrailingPadding + post_chip_spacing) /
+            2.0f -
+        post_chip_spacing;
+    // Height of the separator (dp).
+    const int kSeparatorHeight = 16;
+    gfx::RectF bounds(GetLocalBounds());

[Peter Kasting, 2016/05/25 20:23:03]

Can using float-based bounds here result in drawing the separator in between two
pixels (and thus antialiased across both) instead of snapped to the grid in some
cases?

[Evan Stade, 2016/05/25 21:03:26]

no because there's no anti aliasing, also it's hairline width.

[Peter Kasting, 2016/05/25 21:24:17]

Since there's no code here that explicitly disables AA, and I'm not sure which
side a line that's directly on the pixel divider will be snapped to, does it
make sense to explicitly compute integer drawing coords anyway?  I would find
that clearer (and it would avoid the need to add overrides to Canvas).

Basically, I think all that would be needed would be to call ToEnclosingRect()
or ToEnclosedRect() (depending on which snapping direction you wanted) on
|scaled_bounds| right before the DrawLine() call.

[Evan Stade, 2016/05/25 21:43:08]

Don't you mean explicitly compute non-integer coordinates? Because whole numbers
are between the pixels whereas X.5 is the middle of the (X+1)th pixel from the
left. There are many places where we rely on the snapping behavior already
(everywhere that uses the integer version of DrawLine, for example). I think it
would make more sense to make gfx::Canvas::DrawLine explicitly disable anti
aliasing, except since the default is not to AA, that also feels wrong.

[Peter Kasting, 2016/05/25 21:54:12]

Does that apply to Canvas::DrawLine()?  I thought that, given the integer
coordinates that currently takes, it handles conversion to the appropriate pixel
grid, whereas if you're constructing e.g. an SkPath you need to worry about this
yourself.

That said, in most places I use the Canvas functions, I draw rects (of 1 px
width) instead of lines, in part because the way the coordinates work is
clearer.

I guess what I'm really saying in all this is, I'm not worried about your code
having bugs, and trying to prevent those bugs.  I'm worried that, when I read
your code, it's doing something different than everywhere else (calling
DrawLine() with floating-point coordinates), it looks like sometimes the
coordinates might be at 0.5 and sometimes at 0.0, and it's not obvious that both
of those distinct cases are intended and how they'll be handled.  Whereas if you
use e.g. ToEnclosingRect(), I know exactly what the code intends without having
to investigate how Skia handles fractional coordinates or snaps to the pixel
grid.

[Evan Stade, 2016/05/25 22:32:43]

I don't think there's any explicit handling, DrawLine just calls
SkCanvas::drawLine and wraps the integers with SkIntToScalar. (Also, I was wrong
about hairline, it explicitly sets stroke width to 1.)
so I just tried this and ToEnclosedRect does the right thing in LTR and the
wrong thing in RTL. ToEnclosingRect does the wrong thing in LTR.

[Peter Kasting, 2016/05/25 22:44:33]

I'm kind of lost by this point as to what snapping is desirable in which modes.

Do you get the right answer if you just replace all the floats with ints in this
code (like you originally had it)?  Is there any way at all of writing this so
the reader can see precisely how we allocate extra space in which
directionalities/scale factors?

+    bounds.Inset(kPostSeparatorSpacing,
+                 (bounds.height() - kSeparatorHeight) / 2);
+
+    // 1px at all scale factors.
+    gfx::ScopedCanvas scoped_canvas(canvas);
+    const float scale = canvas->UndoDeviceScaleFactor();
+    const gfx::RectF scaled_bounds = gfx::ScaleRect(bounds, scale);
+    canvas->DrawLine(scaled_bounds.top_right(), scaled_bounds.bottom_right(),
+                     separator_color);
+  }
 }