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 f58d84659ec85f466d051d0b56d8b8710343136d..d347d83b63da49501afbd6bbe7415ec44d35e40e 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,12 @@ double IconLabelBubbleView::WidthMultiplier() const {
   return 1.0;
 }
 
+void IconLabelBubbleView::StopAnimation() {}
+
+bool IconLabelBubbleView::IsShrinking() const {
+  return false;
+}
+
 int IconLabelBubbleView::GetImageAndPaddingWidth() const {
   const int image_width = image_->GetPreferredSize().width();
   return image_width
@@ -123,15 +129,38 @@ 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_min_width = image_->GetPreferredSize().width();

[Peter Kasting, 2016/03/07 20:24:27]

Nit: |image_min_width| seems misleading here since this is really the image's
preferred width, and we can allocate less space than this below.

I would name this |image_width| and then either inline the current |image_width|
temp below into the subsequent SetBounds() call or pick a name for it like
|available_width|.

[varkha, 2016/03/08 18:01:03]

Done. Renamed to |image_preferred_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_min_width)
+                          ? GetBubbleOuterPadding(icon_has_enough_padding)
+                          : (width() - image_min_width) / 2;
+  const int image_padding =
+      label_->visible() ? GetBubbleOuterPadding(false) : 0;
+  const int image_width =
+      std::min(width() - image_x - image_padding, image_min_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_min_width >= bounds().width())) {
+    label_->SetVisible(false);
+  }
+  // Animation is finished when the image is in its target position which is
+  // the position with no padding.
+  if (IsShrinking() && (image_x + image_min_width >= bounds().width()))
+    StopAnimation();

[Peter Kasting, 2016/03/07 20:24:27]

I don't think this is the right way to do this.

If we stop the animation in Layout(), then depending on whether we're dropping
frames, we may not get called until the animation has run past the "minimum
size" point; get sized to our preferred size, which will be too small; have
Layout() called; then try to stop the animation here.  I'm worried about this
resulting in flickery drawing.

It seems better to change WidthMultiplier() so that we calculate the correct
(clamped) "preferred size" at all times, and if we want to stop running the
animation once it's not changing anything, put that calculation in
AnimationProgressed() or something.

[varkha, 2016/03/08 18:01:03]

I have put the correction in IconLabelBubbleView::GetSizeForLabelWidth(). This
allows the multiplier to still grow from 0 to 1 and then go back to 0 but the
width grows from 0 to max and then shrinks from max to image size plus leading
padding at which point the icon is in its target state and the animation stops.

 
-  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 +208,25 @@ SkColor IconLabelBubbleView::GetParentBackgroundColor() const {
 
 gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int width) const {
   gfx::Size size(image_->GetPreferredSize());
-  if (ShouldShowBackground()) {
+  if (ShouldShowBackground() || IsShrinking()) {

[Peter Kasting, 2016/03/07 20:24:27]

Won't ShouldShowBackground() always be true if we actually have a running
animation?

[varkha, 2016/03/08 18:01:03]

There is also label()->visible() condition (in the
ContentSettingImageView::ShouldShowBackground(). I want to be able to hide
the label a bit earlier but continue animating in the last few frames.

     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());
   }
 
   return size;
 }
 
+int IconLabelBubbleView::GetBubbleOuterPadding(bool leading) const {
+  if (ui::MaterialDesignController::IsModeMaterial())
+    return GetBubbleOuterPaddingMd(leading);

[Peter Kasting, 2016/03/07 20:24:27]

Nit: We might as well just scrap GetBubbleOuterPaddingMd() and inline it here.

[varkha, 2016/03/08 18:01:03]

I think keeping it separate makes it a bit easier to reason and should also
simplify removing non-MD path in future. I'd rather keep it.

+
+  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 +241,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)