Adjusts content bubble animation with respect to icon size

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 f51e30a49c0ff20605da55c8b3c2967800534558..0bb93c6daf179de18f0c258b4bfc02ee8e418183 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
@@ -105,6 +105,10 @@ 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
@@ -123,15 +127,34 @@ void IconLabelBubbleView::Layout() {
   // this up when MD is on by default.
   bool icon_has_enough_padding =
       !is_extension_icon_ || ui::MaterialDesignController::IsModeMaterial();
-  const int image_width = image_->GetPreferredSize().width();
-  image_->SetBounds(std::min((width() - image_width) / 2,
-                             GetBubbleOuterPadding(icon_has_enough_padding)),
-                    0, image_->GetPreferredSize().width(), height());
+  const int image_preferred_width = image_->GetPreferredSize().width();
+  // image is positioned at a constant offset when a label is visible or when
+  // there is enough room to fit the whole image. This is done so that the image
+  // slides in as a part of the sliding surface and doesn't shift relative to
+  // the background.
+  const int image_x = (label_->visible() || width() > image_preferred_width)
+                          ? GetBubbleOuterPadding(icon_has_enough_padding)

[Peter Kasting, 2016/03/08 22:07:06]

Can we just always do this?  Is it ever actually necessary to center the image
instead?  Maybe for extension icons pre-MD or something?

[varkha, 2016/03/10 22:25:56]

Done. Still need to make sure the image is right aligned at the end of the
animation. We don't want it to overshoot and then snap back.

[Peter Kasting, 2016/03/11 01:14:42]

How can we overshoot given the current definition of GetSizeForLabelWidth()? 
Won't that always reserve sufficient space?

[varkha, 2016/03/11 03:08:40]

I meant this as an explanation of what I have tried to do in a previous patch
set, not as a problem. A bigger trouble of course is that I have allowed this to
happen during the growth stage as well.

+                          : (width() - image_preferred_width) / 2;
+  const int image_padding =
+      label_->visible() ? GetBubbleOuterPadding(false) : 0;
+  const int image_width =
+      std::min(width() - image_x - image_padding, image_preferred_width);
+  image_->SetBounds(image_x, 0, image_width, height());
+
+  int label_x = GetBubbleOuterPadding(true) + GetImageAndPaddingWidth();
+  const int label_width =
+      std::max(0, width() - label_x - GetBubbleOuterPadding(false));
+
+  // Hiding the label when it no longer fits and when the leading edge of the
+  // image plus padding crosses the right edge of the bubble allows the final
+  // frames of the animation to smoothly transition from showing a bubble to
+  // showing just the image.
+  if (IsShrinking() && label_->visible() && label_width == 0 &&
+      (image_x + image_padding + image_preferred_width >= bounds().width())) {

[Peter Kasting, 2016/03/08 22:07:06]

Why is the last conditional clause here necessary?

[varkha, 2016/03/10 22:25:56]

I wanted to hide the boundary only when the image is roughly centered in the
bubble - having the correct padding on the left and right. If I omit this last
condition the boundary will disappear earlier - when the text no longer has room
left which is the point where there is icon-to-text-padding +
text-to-right-boundary-padding to the right of the icon.

[Peter Kasting, 2016/03/11 01:14:42]

That sounds like the (label_width == 0) check is unnecessary, then.  Frankly,
the label_->visible() check is too, but I can see why it might seem confusing to
remove it.  But I'd at least remove the first, along with the "it no longer fits
and" bit of the comment above.

[varkha, 2016/03/11 03:08:40]

Done.

+    label_->SetVisible(false);
+  }
 
-  int pre_label_width = GetBubbleOuterPadding(true) + GetImageAndPaddingWidth();
-  label_->SetBounds(pre_label_width, 0,
-                    width() - pre_label_width - GetBubbleOuterPadding(false),
-                    height());
+  label_->SetBounds(label_x, 0, label_width, height());
 }
 
 void IconLabelBubbleView::OnNativeThemeChanged(
@@ -179,18 +202,35 @@ SkColor IconLabelBubbleView::GetParentBackgroundColor() const {
 
 gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int width) const {
   gfx::Size size(image_->GetPreferredSize());
-  if (ShouldShowBackground()) {
-    const int non_label_width = GetBubbleOuterPadding(true) +
-                                GetImageAndPaddingWidth() +
-                                GetBubbleOuterPadding(false);
-    size = gfx::Size(WidthMultiplier() * (width + non_label_width), 0);
-    if (!ui::MaterialDesignController::IsModeMaterial())
-      size.SetToMax(background_painter_->GetMinimumSize());
+  bool shrinking = IsShrinking();
+  if (ShouldShowBackground() || shrinking) {

[Peter Kasting, 2016/03/08 22:07:06]

Add comment on why it's necessary to check for |shrinking| here (basically what
you said to me when I asked before).

[varkha, 2016/03/10 22:25:57]

Done.

+    const int leading_padding = GetBubbleOuterPadding(true);
+    const int padding_rest =

[Peter Kasting, 2016/03/08 22:07:06]

Nit: |remaining_padding| seems better ("rest" sounds like "padding at rest")

[varkha, 2016/03/10 22:25:57]

Done.

+        GetBubbleOuterPadding(false) +
+        GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_PADDING);
+    const int image_width = size.width();
+    const double multiplier = WidthMultiplier();
+    // |multiplier| grows from zero to one, stays equal to one and then shrinks
+    // to zero again. This view width should grow from zero to its maximum
+    // width, then stay at its maximum width and then shrink to such width that
+    // has enough room for the image and its leading padding.
+    const int total_width = shrinking
+        ? multiplier * (width + padding_rest) + leading_padding + image_width
+        : multiplier * (width + padding_rest + leading_padding + image_width);

[Peter Kasting, 2016/03/08 22:07:06]

This means the view will shrink more slowly (in terms of linear movement speed)
than it grows.  I don't think that's as good as the apparent behavior we had
before, where the velocity was constant and the time was different.

I think you can achieve this by computing the total width like the second arm
here in all cases, then clamping if |shrinking| is true.

[varkha, 2016/03/10 22:25:57]

Sure. That makes the animation continue near the end for another few
milliseconds without making any apparent change on screen which is why I didn't
do it this way but this doesn't seem to cause trouble.

+    size.set_width(total_width);
   }
-
   return size;
 }
 
+int IconLabelBubbleView::GetBubbleOuterPadding(bool leading) const {
+  if (ui::MaterialDesignController::IsModeMaterial())
+    return GetBubbleOuterPaddingMd(leading);
+
+  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_|.
@@ -205,15 +245,6 @@ void IconLabelBubbleView::SetLabelBackgroundColor(
       SkColorGetA(chip_background_color)));
 }
 
-int IconLabelBubbleView::GetBubbleOuterPadding(bool leading) const {
-  if (ui::MaterialDesignController::IsModeMaterial())
-    return GetBubbleOuterPaddingMd(leading);
-
-  return GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING) -
-         GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
-         (leading ? 0 : GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING));
-}
-
 int IconLabelBubbleView::GetBubbleOuterPaddingMd(bool leading) const {
   // When the image is empty, leading and trailing padding are equal.
   if (image_->GetPreferredSize().IsEmpty() || !leading)