Adjusts content bubble animation with respect to icon size

chrome/browser/ui/views/location_bar/content_setting_image_view.cc

Index: chrome/browser/ui/views/location_bar/content_setting_image_view.cc
diff --git a/chrome/browser/ui/views/location_bar/content_setting_image_view.cc b/chrome/browser/ui/views/location_bar/content_setting_image_view.cc
index b60d76758b535c8088c568eb3df845fd69c3168b..faf7d5a38e0d3e4a91c41fd1b82dee9bf10a7239 100644
--- a/chrome/browser/ui/views/location_bar/content_setting_image_view.cc
+++ b/chrome/browser/ui/views/location_bar/content_setting_image_view.cc
@@ -56,7 +56,9 @@ ContentSettingImageView::ContentSettingImageView(
     SetBackgroundImageGrid(kBackgroundImages);
   }
 
-  image()->SetHorizontalAlignment(views::ImageView::LEADING);
+  image()->SetHorizontalAlignment(base::i18n::IsRTL()
+                                      ? views::ImageView::TRAILING
+                                      : views::ImageView::LEADING);
   image()->set_interactive(true);
   image()->EnableCanvasFlippingForRTLUI(true);
   image()->SetAccessibilityFocusable(true);
@@ -113,7 +115,8 @@ SkColor ContentSettingImageView::GetBorderColor() const {
 }
 
 bool ContentSettingImageView::ShouldShowBackground() const {
-  return slide_animator_.is_animating() || pause_animation_;
+  return label()->visible() &&
+         (slide_animator_.is_animating() || pause_animation_);
 }
 
 double ContentSettingImageView::WidthMultiplier() const {
@@ -132,12 +135,25 @@ double ContentSettingImageView::WidthMultiplier() const {
   return size_fraction;
 }
 
+void ContentSettingImageView::StopAnimation() {
+  slide_animator_.Stop();

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

Stop() is redundant with Reset().  You should only need the latter.

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

Done. also this method is no longer necessary.

+  slide_animator_.Reset();
+}
+
+bool ContentSettingImageView::IsShrinking() const {
+  const double kOpenFraction =
+      static_cast<double>(kOpenTimeMS) / kAnimationDurationMS;
+  return (!pause_animation_ && slide_animator_.is_animating() &&
+          slide_animator_.GetCurrentValue() > (1.0 - kOpenFraction));
+}
+
 void ContentSettingImageView::AnimationEnded(const gfx::Animation* animation) {
   slide_animator_.Reset();
   if (!pause_animation_) {
     label()->SetVisible(false);
     parent_->Layout();
     parent_->SchedulePaint();
+    SchedulePaint();

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

Given the definition of SchedulePaint(), it seems like calling it on parent_
should schedule a paint for the entire parent rect, which should eliminate the
need to call SchedulePaint() here on the child.  Either this should not be
added, or I'd like to understand technically why what I just said isn't true.

I wonder if the issue is that parent_->Layout() sees that the parent's size
hasn't changed and just quickly no-ops or something, without sizing us to our
new preferred size.

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

Done.

[varkha, 2016/03/08 21:56:03]

Will remove the commented line obviously before commit.

   }
 }
 
@@ -245,6 +261,17 @@ void ContentSettingImageView::OnClick() {
     if (!pause_animation_) {
       pause_animation_ = true;
       pause_animation_state_ = slide_animator_.GetCurrentValue();
+
+      // Snap animation to target if a user clicks before the image is exposed.

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

Nit: a -> the

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

Done.

+      const double kOpenFraction =
+          static_cast<double>(kOpenTimeMS) / kAnimationDurationMS;
+      if (pause_animation_state_ < kOpenFraction &&

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

Is this first portion of the conditional needed?  Seems like if at the end of
the animation we animate just down to our target size, this check is redundant.

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

Done.

+          width() < image()->GetPreferredSize().width() +
+                        GetBubbleOuterPadding(true) +
+                        GetBubbleOuterPadding(false)) {
+        pause_animation_ = false;
+        StopAnimation();
+      }
     }
     slide_animator_.Reset();
   }