MD - add ripples to DL shelf items

ui/views/animation/ink_drop_animation_controller_impl.cc

Index: ui/views/animation/ink_drop_animation_controller_impl.cc
diff --git a/ui/views/animation/ink_drop_animation_controller_impl.cc b/ui/views/animation/ink_drop_animation_controller_impl.cc
index def6df3bb6f0b75a9938766ca38dabad570b65b3..811cd82451939d23dffd22a01acb8990b72d229d 100644
--- a/ui/views/animation/ink_drop_animation_controller_impl.cc
+++ b/ui/views/animation/ink_drop_animation_controller_impl.cc
@@ -113,7 +113,7 @@ void InkDropAnimationControllerImpl::SetHovered(bool is_hovered) {
 void InkDropAnimationControllerImpl::CompleteHiddenTargetedAnimations() {
   if (ink_drop_animation_->target_ink_drop_state() == InkDropState::HIDDEN ||
       ShouldAnimateToHidden(ink_drop_animation_->target_ink_drop_state())) {
-    ink_drop_animation_->HideImmediately();
+    CreateInkDropAnimation();

[Evan Stade, 2016/03/31 03:41:27]

this change probably warrants a rename of the fn. What do you think of the
change itself? It is needed because otherwise a second click before the first
ink drop is totally faded (and destroyed on L171) will reuse the old ink drop,
which has the original center point. We need to create a new ink drop for every
hidden->not hidden animation.

[bruthig, 2016/03/31 19:39:03]

I was really, really hoping not to tie the lifetime of an ink drop to a change
in the center point.  In this particular case we will be incurring an extra cost
of Layer destruction/creation when ideally the existing ripple could just be
re-used.

Also I was really, really hoping that not every ripple surface would have to
track the |last_ink_drop_location_| for a couple of reasons.

1. I think it will be difficult to maintain and track the ripple center
correctly when taking in to account ripples should be activated on keyboard
input and ripples are triggered up and down the Button inheritance hierarchy as
well as from the (Button)InkDropDelegate.

2. The plan is to suppress ripples for touch events on Windows.  It would be
great if this decision was captured in one spot, e.g. the
InkDropAnimationControllerImpl.  I haven't flushed out all the details but was
thinking the InkDropDelegate::OnAction() function could also accept a ui::Event
parameter and then it could also use this to set the ripples center point.

Long story short, I'm hesitant to moving forward with this pattern for centering
ripples without at least discussing alternatives.

[Evan Stade, 2016/03/31 21:07:38]

I don't think that's a perf optimization worth worrying about. Perf is important
when many things are happening at once or happening automatically. Since this
happens at most once per user gesture, I doubt this could ever be a problem.
Plus, we currently create a new inkdrop every time the old inkdrop is finished
(this change only affects what we do when the existing effect hasn't completed
yet).

In any case, the way I would go about this if I were worried about it is to make
InkDropHost::CreateInkDropAnimation pass in the old animation, and let it either
update that ink drop or return a new one, as needed.

Also note that the center point can change regardless of whether the ripple is
centered on the click event. A bookmark button can grow or shrink when you
change its title. We have to refresh or replace the ripple even in the
centerpoin-centered case.
Clearly not every ripple surface does need to. So far, only one out of many.
I don't see the difficulty. Keyboard input can reset the location to the center.

[bruthig, 2016/03/31 22:46:19]

Acknowledged.  At one point I was flirting with the idea of having the
InkDropHost be able to call a Layout like function on the InkDrop for situations
like this, but I haven't pursued this much further than that.
Well we know of at least two others in the Bookmarks bar...
How do you foresee the ButtonInkDropDelegate::OnGestureEvent() communicating the
last touch point?

[Evan Stade, 2016/04/01 19:03:13]

I guess ink drop delegate can store the last touch point and expose a getter.

   }
 }