Allow InkDropRipple to co-exist with highlight

ui/views/animation/ink_drop_ripple.h

Index: ui/views/animation/ink_drop_ripple.h
diff --git a/ui/views/animation/ink_drop_ripple.h b/ui/views/animation/ink_drop_ripple.h
index efc4972e9040992c7def572186e7d42ca244ac17..5a60758e18c0c224736682606a5268a71ec90f28 100644
--- a/ui/views/animation/ink_drop_ripple.h
+++ b/ui/views/animation/ink_drop_ripple.h
@@ -57,7 +57,7 @@ class VIEWS_EXPORT InkDropRipple {
   //
   // NOTE: GetTargetInkDropState() should return the new |ink_drop_state| value
   // to any observers being notified as a result of the call.
-  void AnimateToState(InkDropState ink_drop_state);
+  virtual void AnimateToState(InkDropState ink_drop_state);

[bruthig, 2016/08/25 17:45:29]

This was intended to be a non-virtual function that would use the template
pattern to allow descendants to add behaviour.  The intention was that this base
class would manage all the InkDropState transitions, notifying observers, etc
and the descendants would add the visuals.  So there may be some visual based
logic baked into this class that doesn't make sense for the non-visual
EmptyInkDropRipple.  We should probably extract that visual logic from this
class instead so that you don't need to make this function virtual.

This would also apply to the SnapToActivate() function which already appears to
be virtual but it shouldn't be.  It's only working correctly since all
descending classes call it when the overridden SnapToActivated() is called.

WDYT?

[Evan Stade, 2016/08/26 23:36:15]

Generally speaking, extracting logic like this reduces encapsulation, adds to
complexity, reduces readability (because what you have to read is spread across
more files). This solution seems more elegant to me. I'm not sure what practical
harm it's causing?

[bruthig, 2016/08/31 22:22:29]

The InkDropRipple class defines a contract that guarantees that the
InkDropRippleObserver is notified for all state changes.  IMO your change makes
it much more difficult to understand and reason about the InkDropImpl class
because sometimes it will be notified and sometimes it won't.  Also, by making
it virtual you are offering future developers more choice as to which function
to override, i.e. AnimateToState() or AnimateStateChange().  Contrary to your
point in another comment where you are in favor of limiting choice.

I've seen this same pattern in the EventHandler/View/Button hierarchy where the
general OnEvent() handler as well as the more specific event handlers like
OnGestureEvent() are virtual.  The root EventHandler::OnEvent() dispatches to
these more specific event handlers and this makes it very difficult to reason
about when a more specific event handler will be called in a concrete View
because you have to look up and down the hierarchy to make sure all OnEvent()'s
call the parent's OnEvent().

I don't understand what you mean when you say this reduces encapsulation.  Can
you elaborate?

The InkDropRipple class encapsulates state transitions and notifying observers. 
Descendant classes encapsulate the visual (or non-visual) animations.  I feel
like these two levels of abstraction are a quite good and by extracting some
common visual logic doesn't expose it to clients of the InkDropRipple in anyway.

The practical harm is that developers will need to understand all of the
InkDropRipple flavors when trying to understand the InkDropImpl class.  By
keeping the uncertainty/complexity closer together it makes the larger system
easier to reason about.

[Evan Stade, 2016/09/01 01:19:15]

It's not really a choice, because AnimateStateChange is pure virtual so you must
override it.
I'm invoking encapsulation as the principle of all related logic being close
together as good.
I mean, EmptyInkDropRipple is pretty easy to understand. It barely has any
lines.

Would you feel better about adding a new virtual function called bool
HasAnimations? AnimateToState would early return if !HasAnimations. That would
accomplish the goal here without giving future developers undue amounts of
flexibility.

[bruthig, 2016/09/01 15:53:57]

I would have to see this more concretely.  The major issue here is that
AnimationEnded is not going to be called and thus the |ink_drop_ripple_| is not
going to be destroyed.

[Evan Stade, 2016/09/07 23:46:19]

updated.

I don't think it's really a problem if an empty ink drop ripple is never
destroyed, but it actually will be destroyed in DestroyHiddenTargetedAnimations
because the target state is always HIDDEN.

[bruthig, 2016/09/08 15:56:04]

Right, the layer won't leak memory but it will exist until the
InkDropImpl::AnimateToState() or InkDropImpl::SnapToAcitvated() is called the
next time.  In which case the existing EmptyInkDrop will be destroyed but a new
one will be created right away.  Thus any InkDrop enabled surface, that has had
an animation will have an instance of the EmptyInkDrop.  At this point it would
be a simple change.  Instead of returning early in
InkDropRipple::AnimateToState() and SnapToActivated() just allow the creation of
the |animation_observer| and the SetActive() called on it and prevent all the
other code from being called.

[Evan Stade, 2016/09/08 19:17:31]

I don't understand the motivation to do that. We're releasing some memory a
little earlier by allocating more memory?

[bruthig, 2016/09/09 16:05:47]

That is the current scenario.  I am requesting that the memory is released much
sooner, i.e. when the button is no longer being interacted with.

[Evan Stade, 2016/09/09 18:55:48]

The memory footprint is negligible and it's not a leak in the sense that it's
limited to one per button. When I said "by allocating more memory" I was
referring to the observer you'd need to create.

[bruthig, 2016/09/09 19:30:05]

How about this, in the quick return you could invoke AnimationStartedCallback()
and AnimationEndedCallback() directly?

 
   InkDropState target_ink_drop_state() const { return target_ink_drop_state_; }
 
@@ -72,13 +72,16 @@ class VIEWS_EXPORT InkDropRipple {
   // as well as the transition to the ACTIVATED state.
   virtual void SnapToActivated();
 
-  // The root Layer that can be added in to a Layer tree.
+  // The root Layer that can be added in to a Layer tree. This may return null.
   virtual ui::Layer* GetRootLayer() = 0;
 
   // Returns true when the ripple is visible. This is different from checking if
   // the ink_drop_state() == HIDDEN because the ripple may be visible while it
   // animates to the target HIDDEN state.
-  virtual bool IsVisible() const = 0;
+  bool IsVisible();
+
+  // Returns true if this ripple is mutually exclusive with InkDropHighlight.
+  virtual bool OverridesHighlight() const = 0;
 
   // Returns a test api to access internals of this. Default implmentations
   // should return nullptr and test specific subclasses can override to return