Adding ripple effect for clicks on MD slider

ui/views/controls/slider.cc

Index: ui/views/controls/slider.cc
diff --git a/ui/views/controls/slider.cc b/ui/views/controls/slider.cc
index dd1148ff1a59123e6f2c960404aee5ffe732267c..169e2c74619217b913bcc5557ba24a43be21feea 100644
--- a/ui/views/controls/slider.cc
+++ b/ui/views/controls/slider.cc
@@ -27,7 +27,7 @@
 #include "ui/views/widget/widget.h"
 
 namespace {
-const int kSlideValueChangeDurationMS = 150;
+const int kSlideValueChangeDurationMs = 150;
 
 // The image chunks.
 enum BorderElements {
@@ -55,7 +55,7 @@ Slider::Slider(SliderListener* listener)
     : listener_(listener),
       value_(0.f),
       keyboard_increment_(0.1f),
-      animating_value_(0.f),
+      initial_animating_value_(0.f),
       value_is_valid_(false),
       accessibility_events_enabled_(true),
       focus_border_color_(0),
@@ -71,6 +71,11 @@ Slider::Slider(SliderListener* listener)
 Slider::~Slider() {
 }
 
+void Slider::SetHighlighted(bool is_highlighted) {
+

[bruthig, 2016/09/23 22:24:39]

nit: no newline required. the braces can be on the same line.  Running 'git cl
format' might do this for you ;)

[yiyix, 2016/09/24 06:24:36]

Done. format did not fix this.... unfortunately. 

+}
+
+
 void Slider::SetValue(float value) {
   SetValueInternal(value, VALUE_CHANGED_BY_API);
 }
@@ -93,26 +98,24 @@ void Slider::SetValueInternal(float value, SliderChangeReason reason) {
   if (old_value_valid && base::MessageLoop::current()) {
     // Do not animate when setting the value of the slider for the first time.
     // There is no message-loop when running tests. So we cannot animate then.
-    animating_value_ = old_value;
-    move_animation_.reset(new gfx::SlideAnimation(this));
-    move_animation_->SetSlideDuration(kSlideValueChangeDurationMS);
-    move_animation_->Show();
-    AnimationProgressed(move_animation_.get());
+    if (!move_animation_.get() || !move_animation_->is_animating()) {

[bruthig, 2016/09/23 22:24:39]

This new condition confuses me a little, what is its purpose?

[yiyix, 2016/09/24 06:24:36]

This is because I had !move_animation_->is_animating() then reset the animation
in my animationEnded functions. I was imagining that the move_animation_ is
reset when this condition holds. As we can safely remove this reset in
animationEnded, I can safely remove this second condition. 

+      initial_animating_value_ = old_value;
+      move_animation_.reset(new gfx::SlideAnimation(this));
+      move_animation_->SetSlideDuration(kSlideValueChangeDurationMs);
+      move_animation_->Show();
+    }
   } else {
     SchedulePaint();
   }
-  if (accessibility_events_enabled_ && GetWidget()) {
-    NotifyAccessibilityEvent(
-        ui::AX_EVENT_VALUE_CHANGED, true);
-  }
+  if (accessibility_events_enabled_ && GetWidget())
+    NotifyAccessibilityEvent(ui::AX_EVENT_VALUE_CHANGED, true);
 }
 
 void Slider::PrepareForMove(const int new_x) {
   // Try to remember the position of the mouse cursor on the button.
   gfx::Insets inset = GetInsets();
   gfx::Rect content = GetContentsBounds();
-  float value = move_animation_.get() && move_animation_->is_animating() ?
-        animating_value_ : value_;
+  float value = GetAnimatingValue();
 
   const int thumb_width = GetThumbWidth();
   const int thumb_x = value * (content.width() - thumb_width);
@@ -172,7 +175,8 @@ void Slider::OnPaint(gfx::Canvas* canvas) {
 
 float Slider::GetAnimatingValue() const{
   return move_animation_.get() && move_animation_->is_animating()
-             ? animating_value_
+             ? move_animation_->CurrentValueBetween(initial_animating_value_,
+                                                    value_)
              : value_;
 }
 
@@ -241,10 +245,16 @@ void Slider::OnGestureEvent(ui::GestureEvent* event) {
 }
 
 void Slider::AnimationProgressed(const gfx::Animation* animation) {
-  animating_value_ = animation->CurrentValueBetween(animating_value_, value_);
+  if (animation && animation != move_animation_.get())

[bruthig, 2016/09/23 22:24:39]

nit: I think it's safe to assume |animation| is not null here. i.e. drop the
first clause.  If it were possible for |animation| to be null I don't think you
want to be calling SchedulePaint() anyway right?

[yiyix, 2016/09/24 06:24:36]

I think you mean "if it were possible for |animation| to be null, I think you
want to be calling SchedulePaint() anyways" :P

I agree with you, i want to paint the new state.

+    return;
   SchedulePaint();
 }
 
+void Slider::AnimationEnded(const gfx::Animation* animation) {
+  if (animation == move_animation_.get() && !move_animation_->IsShowing())

[bruthig, 2016/09/23 22:24:39]

From what I can see Hide() will never be used on the |move_animation_|.  Thus
!move_animation_->IsShowing() will always be false and you will never be
destroying the |move_animation_|.  You should be able to safely drop the second
clause here.

[yiyix, 2016/09/24 06:24:36]

You right, I never hide anything in this class

+    move_animation_.reset();
+}
+
 void Slider::GetAccessibleState(ui::AXViewState* state) {
   state->role = ui::AX_ROLE_SLIDER;
   state->name = accessible_name_;
@@ -253,11 +263,13 @@ void Slider::GetAccessibleState(ui::AXViewState* state) {
 }
 
 void Slider::OnSliderDragStarted() {
+  SetHighlighted(true);
   if (listener_)
     listener_->SliderDragStarted(this);
 }
 
 void Slider::OnSliderDragEnded() {
+  SetHighlighted(false);
   if (listener_)
     listener_->SliderDragEnded(this);
 }