New location security strings and animation.

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

Index: chrome/browser/ui/views/location_bar/location_icon_view.cc
diff --git a/chrome/browser/ui/views/location_bar/location_icon_view.cc b/chrome/browser/ui/views/location_bar/location_icon_view.cc
index 03382cc2a54d3a2991f4245b0b071d5787b8ac90..dcc8829515787244513d1eab8f42ba0a18a1c8e8 100644
--- a/chrome/browser/ui/views/location_bar/location_icon_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_icon_view.cc
@@ -22,7 +22,6 @@
 #include "ui/views/controls/label.h"
 #include "ui/views/painter.h"
 
-using content::NavigationController;
 using content::NavigationEntry;
 using content::WebContents;
 
@@ -34,7 +33,8 @@ LocationIconView::LocationIconView(const gfx::FontList& font_list,
                           parent_background_color,
                           true),
       suppress_mouse_released_action_(false),
-      location_bar_(location_bar) {
+      location_bar_(location_bar),
+      animation_(this) {
   set_id(VIEW_ID_LOCATION_ICON);
 
 #if defined(OS_MACOSX)
@@ -166,3 +166,26 @@ void LocationIconView::SetBackground(bool should_show_ev) {
   else
     UnsetBackgroundImageGrid();
 }
+
+void LocationIconView::StartAnimation() {
+  if (!(animation_.is_animating() || animation_.GetCurrentValue() == 1)) {
+    animation_.SetSlideDuration(label()->GetPreferredSize().width() * 10);

[Peter Kasting, 2016/08/15 23:29:50]

This doesn't seem right.  We should use the same time period to animate any of
our messages, rather than basing it on the message length.  This way we can pick
a specific short animation duration and ensure all animations comply with it,
regardless of translation etc.  This will also allow setting this duration once
at construction.

Plus this value is way too high.  Most animations in the UI, especially ones
shown frequently as these will be, should take 100 ms or less.  We're not Apple,
we shouldn't "tune for the demo, annoy users long-term" :)

[Kevin Bailey, 2016/08/16 16:39:36]

Ok, was just copying other code (I think omnibox_result_view.) To make sure I'm
on the correct page, this seems to be the opposite of what you requested in
icon_label_bubble_view.cc.
Ah, sorry, forgot about it. I had it high so I could 1) make sure it was
animating at all and 2) see clearly what the animation was doing.

[Peter Kasting, 2016/08/16 18:15:06]

Yes, I thought about noting why I'm apparently requesting the opposite.  Setting
the same timing here (and the same as the content setting code; see below) will
make sure that the different states the user gets into all have animations that
take the same time, even if the string lengths are different.  Because these
will happen at disparate points in time, the user will notice dissimilarities in
total animation time more than dissimilarities in speed.

The comment in the other spot will ensure that, especially for very short
strings, the open and close animations don't appear to run at different speeds. 
Since these animations happen within a few seconds of each other, a noticeably
slower travel rate when closing would feel "different" more than a different
total time would.

I'm having a tough time articulating this precisely, but basically both issues
are about making the perception of the different types of animations we do more
consistent.  Why this is well-served by shooting for equal times in one place
and equal speeds in another is basically about one feeling like "different
animations" and the other "two halves of the same animation".
I suggest using the same animation timing constants (ideally, from a shared
location; maybe just hoist to IconLabelBubbleView) as the
ContentSettingImageView: 150 ms each to open/close, and 3200 ms open in between.
 Consistency is good.

+    animation_.Reset(0);

[Peter Kasting, 2016/08/15 23:29:50]

Shouldn't reset to 0 here; we should animate from wherever we're at, and the
caller should ensure we get animated (or snapped) to 0 when necessary.

This is related to my comments about not running animations from LBV::Layout().

[Kevin Bailey, 2016/08/16 16:39:37]

I'll have to work on this with the rest of the approach.

[Kevin Bailey, 2016/08/17 16:33:30]

Done, and I don't think we need the checks any more either.

+    animation_.Show();
+  }
+}
+
+void LocationIconView::StopAnimation() {
+  if (animation_.is_animating() || animation_.GetCurrentValue() == 1)

[Peter Kasting, 2016/08/15 23:29:50]

Why is this conditional needed?

[Kevin Bailey, 2016/08/16 16:39:36]

This replaces 'animation_already_started_'. It is needed because this approach
repeatedly calls 'StopAnimation()'.

[Peter Kasting, 2016/08/16 18:15:06]

But if it's removed, the only effect will be that we call Reset(0) again after
calling Reset(0) once, which should be a no-op.

[Kevin Bailey, 2016/08/17 16:33:30]

Done.

+    animation_.Reset(0);
+}
+
+void LocationIconView::AnimationProgressed(const gfx::Animation*) {
+  // Since things are moving (sideways), we need a re-layout and paint.

[Peter Kasting, 2016/08/15 23:29:50]

Nit: This comment is obvious, so I'd remove it.

[Kevin Bailey, 2016/08/16 16:39:37]

Done.

+  location_bar_->Layout();
+  location_bar_->SchedulePaint();
+}
+
+double LocationIconView::WidthMultiplier() const {
+  return animation_.GetCurrentValue();
+}