Update omnibox chips in MD

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 88acf5493217afa34a172b46759624ae99a4ccaf..c82088a7411e116105f257c92a14f5ce2c8d4b9c 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
@@ -38,8 +38,6 @@ IconLabelBubbleView::IconLabelBubbleView(int contained_image,
     : 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) {
@@ -63,9 +61,8 @@ IconLabelBubbleView::IconLabelBubbleView(int contained_image,
   // 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)));
 }
 
 IconLabelBubbleView::~IconLabelBubbleView() {
@@ -98,13 +95,6 @@ void IconLabelBubbleView::SetLabel(const base::string16& 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_);

[Peter Kasting, 2016/05/20 20:54:11]

If this call is removed, GetVisibleMargins() can be removed entirely; this was
the only non-test caller.

[Evan Stade, 2016/05/20 22:33:53]

yes, as noted above. The only reason I haven't done it yet is that I didn't want
to go through with the hassle until this removal was approved by you.

-    if (base::i18n::IsRTL())
-      std::swap(builtin_leading_padding_, builtin_trailing_padding_);
-  }
 }
 
 bool IconLabelBubbleView::ShouldShowBackground() const {
@@ -189,15 +179,10 @@ 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) {
@@ -236,6 +221,9 @@ gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int label_width) const {
   // is necessary to animate |total_width| even when the background is hidden
   // as long as the animation is still shrinking.
   if (ShouldShowBackground() || shrinking) {
+    // Extra padding to place at the end of the label (right side in LTR).
+    const int extra_trailing_padding =
+        ui::MaterialDesignController::IsModeMaterial() ? 7 : 0;

[Evan Stade, 2016/05/19 21:20:45]

these constants are basically trial and error. The measurements in the specs
were not possible to copy directly because of various other padding (such as the
effective padding around text, which also varies with the actual characters
being displayed).

[Peter Kasting, 2016/05/20 20:54:11]

The mock is 8 dp between text and divider line, right?  I would probably make
this value 8 if so; I would imagine some characters have a fully- or
mostly-transparent pixel to their right and some don't, and we probably want to
ensure "at least 8 px".

Although, how does this space get apportioned across (pre-divider space,
divider, post-divider space)?  We want that total to be 17 dp (depending on the
answer to the divider scaling question below), right?  I guess I'm not sure how
we get away with only 7 here.

[Evan Stade, 2016/05/20 22:33:53]

The way the mocks measure the padding abuts the readlines too closely to the
text. 7 gets us closer to the effective appearance represented by the mocks.
Thus we really only want to get to 15.

We are still padding by ICON_LABEL_VIEW_TRAILING_PADDING, i.e. 6, so we're
adding 13 in total. 2 more come from the spacing between the "bubble" and
omnibox view. That makes 15 total.

It would be a little simpler if I could move that 2 into
ICON_LABEL_VIEW_TRAILING_PADDING, but I can't because that would affect the
padding before the icon as well.

[Peter Kasting, 2016/05/20 22:44:56]

Is that because that same constant is used as the initial padding?  Can we just
split that into two separate constants, and then move the 2 as you suggest?

[Evan Stade, 2016/05/23 21:24:13]

OK, I tried that, but it messed up the blocked popup chip spacing (that
chip/bubble being followed by the bookmark star). So I left the 2 in place but
made it more mathy.

     // |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
@@ -243,7 +231,8 @@ gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int label_width) const {
     // 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);
@@ -276,8 +265,7 @@ int IconLabelBubbleView::GetOuterPadding(bool leading) const {
     // 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);
+    return GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING);
   }
 
   return GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING) -
@@ -288,8 +276,7 @@ int IconLabelBubbleView::GetOuterPadding(bool leading) const {
 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 {
@@ -303,6 +290,19 @@ void IconLabelBubbleView::OnPaint(gfx::Canvas* canvas) {
     views::Painter::PaintPainterAt(canvas, background_painter_.get(),
                                    GetContentsBounds());
   }
-  if (background())
-    background()->Paint(canvas, this);
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    gfx::Rect bounds(GetLocalBounds());
+    // Amount of space to leave after the separator (dp).
+    const int kPostSeparatorSpacing = 4;

[Peter Kasting, 2016/05/20 20:54:11]

This is calculated so that with the additional spacing between items, it's 8 dp,
right?

Maybe this value should be "8 - (whatever constant we use for spacing between
items)" then.

[Evan Stade, 2016/05/20 22:33:53]

well, not quite. The mocks seem to assume less internal padding inside the text
fields / labels than reality. So this would need to be ~7 - 1 - some constant.
That constant is 2, the magic number from LocationBarView::Layout. The reason I
didn't want to share this number and use math is because in the course of
creating this CL I found it much more confusing to read calculations and
treacherous to change values that are used in multiple places, rather than just
having a few hardcoded constants (especially given that the mathy approach
generally doesn't reduce the number of hardcoded values).

[Peter Kasting, 2016/05/20 22:44:56]

Totally agree that calculations can be confusing and hard to read.

I still kind of like them, and the reason is because they make explicit in the
code the implicit dependencies between the values of particular variable and the
places affected by them.  I ran into a lot of frame and location bar bugs during
the MD work that had happened because we changed the constants that obviously
affected one location and didn't realize this actually meant we needed to change
some constants in a different class.  Not a few of these had to do with the old
version of these chips -- e.g. over time people had put in fixes for the EV
bubble that actually broke some stuff about keyword search, and then the keyword
search bubble worked around that, but no one had ever fixed the blocked popup
bubble which had the same problem, etc.

So if this implicitly depends on that other value I'd kind of rather make it
explicitly depend on it even if it's ugly.

Another route could be to change how the parent of these objects (the
LocationBarView) lays them out to not put any external padding around them and
instead expect the code here to do everything.  That would let you just use a
single constant here.  The question mark would be (1) how feasible that is and
(2) whether there's really still an implicit dependency, where if in the future
we change the padding between most items, we really want to change all the
padding values here too.

[Evan Stade, 2016/05/23 21:24:13]

made this mathy, ptal?

+    // Height of the separator (dp).
+    const int kSeparatorHeight = 16;

[Peter Kasting, 2016/05/20 20:54:11]

I wonder if this should be calculated based on the actual font metrics.  It
looked in the mock like this was the same height up and down as the normal
ascent (i.e. no internal leading) and descent.  Perhaps we should be taking the
max distance either of those goes from the centerline and making that the
"radius" of this divider?

This would matter for e.g. Hindi or something where the substituted font may
have very different metrics than the normal UI font.

Or maybe we just want 16 px no matter what.

[Evan Stade, 2016/05/20 22:33:53]

Not sure what you want me to do at the moment. Shall I file a bug to follow up
on this?

[Peter Kasting, 2016/05/20 22:44:57]

I'd just ask Max, and assume until he answers that hardcoding 16 is fine.

[Evan Stade, 2016/05/23 21:24:13]

he seems to want exactly 16

+    bounds.Inset(kPostSeparatorSpacing,
+                 (bounds.height() - kSeparatorHeight) / 2);
+    SkColor separator_color =
+        SkColorSetA(GetNativeTheme()->GetSystemColor(
+                        ui::NativeTheme::kColorId_TextfieldDefaultColor),
+                    0x26);
+    canvas->DrawLine(bounds.top_right(), bounds.bottom_right(),
+                     separator_color);

[Peter Kasting, 2016/05/20 20:54:11]

Will this draw a 2 px-thick line on scale factor 2?  I'm wondering whether we
want that, or we want a 1 px line at all scale factors.

[Evan Stade, 2016/05/20 22:33:53]

I think we want 1dp, i.e. 2px on scale factor 2. The 200% mocks do have 2px
width. I'll ping Max.

[Evan Stade, 2016/05/23 21:24:13]

ok, 1px it is

+  }
 }