Added material design mouse hover feedback support.

Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.SetHoveredIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsNotHovered

BUG=537238, 518919
Committed: https://crrev.com/c657e3be3b1248d7e7908c10fe75dc2f191dccee
Cr-Commit-Position: refs/heads/master@{#371511}

[tdanderson, 2015-10-15 14:46:53]

tdanderson@chromium.org changed reviewers:
+ tdanderson@chromium.org

[tdanderson, 2015-10-15 14:46:54]

Overall LG, please see a few comments.

https://codereview.chromium.org/1390113006/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1390113006/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.h:104: // Animation controller
for the ink drop ripple effect.
nit: update documentation to indicate that the IDAC also handles hover states
too.

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_hover.cc (right):

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_hover.cc:57: ui::CallbackLayerAnimationObserver*
animation_observer =
Do you really want |animation_observer| and |animation_sequence| to be raw
pointers?

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_hover.cc:67:
ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
from in person conversation: should this be preempt?

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_hover.h (right):

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_hover.h:45: enum HoverAnimation { FADE_IN, FADE_OUT
};
nit: I would prefer a name such as HoverAnimationType

[bruthig, 2015-10-15 21:49:28]

bruthig@chromium.org changed reviewers:
+ pkasting@chromium.org, sadrul@chromium.org

[bruthig, 2015-10-15 21:49:29]

pkasting@chromium.org, Please review changes in:
 - chrome/browser/*

sadrul@chromium.org, Please review changes in:
 - ui/views/*

https://codereview.chromium.org/1390113006/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1390113006/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.h:104: // Animation controller
for the ink drop ripple effect.
On 2015/10/15 14:46:54, tdanderson wrote:
> nit: update documentation to indicate that the IDAC also handles hover states
> too.

Done.

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_hover.cc (right):

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_hover.cc:57: ui::CallbackLayerAnimationObserver*
animation_observer =
On 2015/10/15 14:46:54, tdanderson wrote:
> Do you really want |animation_observer| and |animation_sequence| to be raw
> pointers?

I've added a comment about the |animation_observer| since it's an uncommon
pattern, but omitted one for the |animation_sequence| because I think anyone
working with LayerAnimationSequences and LayerAnimators should know that the
LayerAnimator takes over ownership of the sequence in the StartAnimation()
method.

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_hover.cc:67:
ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
On 2015/10/15 14:46:54, tdanderson wrote:
> from in person conversation: should this be preempt?

I double checked and IMMEDIATELY_ANIMATE_TO_NEW_TARGET is correct here.

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_hover.h (right):

https://codereview.chromium.org/1390113006/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_hover.h:45: enum HoverAnimation { FADE_IN, FADE_OUT
};
On 2015/10/15 14:46:54, tdanderson wrote:
> nit: I would prefer a name such as HoverAnimationType

Done.

[Peter Kasting, 2015-10-15 22:21:33]

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:147:
UpdateInkDropHoverState();
Why do you need this?

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:160: void
ToolbarButton::OnMouseMoved(const ui::MouseEvent& event) {
Why do you need this override if you have the entered and exited overrides?

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:348:
UpdateInkDropHoverState();
Why do you need this?

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:367:
ink_drop_animation_controller_->SetHovered((enabled() && IsMouseHovered()));
Too many parens

[bruthig, 2015-10-16 14:57:21]

pkasting@, can you PTAL?

sadrul@, can you review the comments in toolbar_button.cc in patch set 3.  There
certainly is some odd behavior for when OnMouseExited() notifications are
dispatched and I'm wondering if it's WAI?

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:147:
UpdateInkDropHoverState();
On 2015/10/15 22:21:33, Peter Kasting wrote:
> Why do you need this?

It is needed for the following situation:

1.) Left mouse click on the Home button and drag outside the bounds of the
button.
2.) Release the mouse. An OnMouseExited() event is NOT dispatched until the
mouse is moved.

By updating the status on an OnMouseReleased() we avoid having the hover
incorrectly shown if the mouse button is released outside the bounds of the
button.

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:160: void
ToolbarButton::OnMouseMoved(const ui::MouseEvent& event) {
On 2015/10/15 22:21:33, Peter Kasting wrote:
> Why do you need this override if you have the entered and exited overrides?

This was necessary for the following situation:

1.) Mouse over the Back button causing hover to fade in
2.) Tap and hold the Back button until the menu shows. This causes the hover to
disappear AND the mouse icon to disappear, making IsHovered() return false.
3.) Hit escape to dismiss the menu. 
At this point you are in a state where the mouse is hidden but is located inside
the bounds of the Back button. Updating the hover here makes the hover fade back
in if the user moves the mouse because no OnMouseEntered() event will occur.

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:348:
UpdateInkDropHoverState();
On 2015/10/15 22:21:33, Peter Kasting wrote:
> Why do you need this?

Similar reason to why I needed it in ToolbarButton::OnMouseReleased(), the mouse
may or may not be inside the bounds of the button when the menu closes and
OnMouseEntered()/OnMouseExited() events are not dispatched while the menu is
being shown or when the menu is closed.  Oddly enough the OnMouseExited() is not
dispatched even when the mouse moves and it is over the web contents area.

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:367:
ink_drop_animation_controller_->SetHovered((enabled() && IsMouseHovered()));
On 2015/10/15 22:21:33, Peter Kasting wrote:
> Too many parens

Done.

[Peter Kasting, 2015-10-16 18:41:16]

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:147:
UpdateInkDropHoverState();
On 2015/10/16 14:57:21, bruthig wrote:
> On 2015/10/15 22:21:33, Peter Kasting wrote:
> > Why do you need this?
> 
> It is needed for the following situation:
> 
> 1.) Left mouse click on the Home button and drag outside the bounds of the
> button.
> 2.) Release the mouse. An OnMouseExited() event is NOT dispatched until the
> mouse is moved.
> 
> By updating the status on an OnMouseReleased() we avoid having the hover
> incorrectly shown if the mouse button is released outside the bounds of the
> button.

It sounds like a bug to me that OnMouseExited() isn't fired when the mouse is
down.  I think instead you should find and fix the cause of that.

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:160: void
ToolbarButton::OnMouseMoved(const ui::MouseEvent& event) {
On 2015/10/16 14:57:21, bruthig wrote:
> On 2015/10/15 22:21:33, Peter Kasting wrote:
> > Why do you need this override if you have the entered and exited overrides?
> 
> This was necessary for the following situation:
> 
> 1.) Mouse over the Back button causing hover to fade in
> 2.) Tap and hold the Back button until the menu shows. This causes the hover
to
> disappear AND the mouse icon to disappear, making IsHovered() return false.
> 3.) Hit escape to dismiss the menu. 
> At this point you are in a state where the mouse is hidden but is located
inside
> the bounds of the Back button. Updating the hover here makes the hover fade
back
> in if the user moves the mouse because no OnMouseEntered() event will occur.

I don't reproduce the mouse pointer disappearing when the button is held down. 
Is this CrOS only?  That seems like a bug; I don't think we should be hiding the
mouse pointer or changing IsHovered() to return false.

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:348:
UpdateInkDropHoverState();
On 2015/10/16 14:57:21, bruthig wrote:
> On 2015/10/15 22:21:33, Peter Kasting wrote:
> > Why do you need this?
> 
> Similar reason to why I needed it in ToolbarButton::OnMouseReleased(), the
mouse
> may or may not be inside the bounds of the button when the menu closes and
> OnMouseEntered()/OnMouseExited() events are not dispatched while the menu is
> being shown or when the menu is closed.  Oddly enough the OnMouseExited() is
not
> dispatched even when the mouse moves and it is over the web contents area.

Same reply as above.

[bruthig, 2015-10-16 19:10:51]

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:160: void
ToolbarButton::OnMouseMoved(const ui::MouseEvent& event) {
On 2015/10/16 18:41:16, Peter Kasting wrote:
> On 2015/10/16 14:57:21, bruthig wrote:
> > On 2015/10/15 22:21:33, Peter Kasting wrote:
> > > Why do you need this override if you have the entered and exited
overrides?
> > 
> > This was necessary for the following situation:
> > 
> > 1.) Mouse over the Back button causing hover to fade in
> > 2.) Tap and hold the Back button until the menu shows. This causes the hover
> to
> > disappear AND the mouse icon to disappear, making IsHovered() return false.
> > 3.) Hit escape to dismiss the menu. 
> > At this point you are in a state where the mouse is hidden but is located
> inside
> > the bounds of the Back button. Updating the hover here makes the hover fade
> back
> > in if the user moves the mouse because no OnMouseEntered() event will occur.
> 
> I don't reproduce the mouse pointer disappearing when the button is held down.

> Is this CrOS only?  That seems like a bug; I don't think we should be hiding
the
> mouse pointer or changing IsHovered() to return false.

I just want to confirm that you are indeed using a touch screen and tapping with
your finger for step 2?

[Peter Kasting, 2015-10-16 19:21:31]

On 2015/10/16 19:10:51, bruthig wrote:
>
https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):
> 
>
https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/toolbar/toolbar_button.cc:160: void
> ToolbarButton::OnMouseMoved(const ui::MouseEvent& event) {
> On 2015/10/16 18:41:16, Peter Kasting wrote:
> > On 2015/10/16 14:57:21, bruthig wrote:
> > > On 2015/10/15 22:21:33, Peter Kasting wrote:
> > > > Why do you need this override if you have the entered and exited
> overrides?
> > > 
> > > This was necessary for the following situation:
> > > 
> > > 1.) Mouse over the Back button causing hover to fade in
> > > 2.) Tap and hold the Back button until the menu shows. This causes the
hover
> > to
> > > disappear AND the mouse icon to disappear, making IsHovered() return
false.
> > > 3.) Hit escape to dismiss the menu. 
> > > At this point you are in a state where the mouse is hidden but is located
> > inside
> > > the bounds of the Back button. Updating the hover here makes the hover
fade
> > back
> > > in if the user moves the mouse because no OnMouseEntered() event will
occur.
> > 
> > I don't reproduce the mouse pointer disappearing when the button is held
down.
> 
> > Is this CrOS only?  That seems like a bug; I don't think we should be hiding
> the
> > mouse pointer or changing IsHovered() to return false.
> 
> I just want to confirm that you are indeed using a touch screen and tapping
with
> your finger for step 2?

Wait, now I'm confused.  Are you saying, mouse over the button with the mouse,
but then don't click it, and instead let go of the mouse and touch the screen?

If so, then I wonder whether moving the mouse in a way that makes the pointer
reappear should send MouseEntered to the control under the pointer.  What does
Windows do?

[bruthig, 2015-10-22 15:31:24]

sadrul@, Friendly ping. Can you review the comments in toolbar_button.cc in
patch set 3.  There
certainly is some odd behavior for when OnMouseExited() notifications are
dispatched and I'm wondering if it's WAI?

We might need to have a follow-up offline discussion, but the threads here will
provide context.

[sadrul, 2015-10-22 21:26:03]

Sorry about the delay.

On 2015/10/22 15:31:24, bruthig wrote:
> sadrul@, Friendly ping. Can you review the comments in toolbar_button.cc in
> patch set 3.  There
> certainly is some odd behavior for when OnMouseExited() notifications are
> dispatched and I'm wondering if it's WAI?
> 
> We might need to have a follow-up offline discussion, but the threads here
will
> provide context.

There seem to be two issues:

. OnMouseExited() is not fired if the mouse-cursor is dragged out of a View
before releasing the button. It does sound like a bug. It'd be good to fix this.
(but considering this is a pretty old behaviour, there may be code that relies
on this behaviour, I am not sure).

. When mouse-cursor is hidden because of some other input event (e.g. touch), we
should dispatch a MouseExited event. Similarly, when the mouse-cursor is made
visible again, we should dispatch a MouseEntered event. If we are not doing
that, then that sounds like a bug we should fix. I seem to remember we
implemented this behaviour a while back. Can you confirm with tdanderson@?

[bruthig, 2015-11-02 16:42:31]

I've updated the patch set so that the hover is not automatically shown after a
ripple animation.

pkasting@, can you please take another look at:
 - chrome/browser/*

sadrul@, can you please take another look at:
 - ui/views/*

On 2015/10/22 21:26:03, sadrul wrote:
> Sorry about the delay.
> 
> On 2015/10/22 15:31:24, bruthig wrote:
> > sadrul@, Friendly ping. Can you review the comments in toolbar_button.cc in
> > patch set 3.  There
> > certainly is some odd behavior for when OnMouseExited() notifications are
> > dispatched and I'm wondering if it's WAI?
> > 
> > We might need to have a follow-up offline discussion, but the threads here
> will
> > provide context.
> 
> There seem to be two issues:
> 
> . OnMouseExited() is not fired if the mouse-cursor is dragged out of a View
> before releasing the button. It does sound like a bug. It'd be good to fix
this.
> (but considering this is a pretty old behaviour, there may be code that relies
> on this behaviour, I am not sure).
> 
> . When mouse-cursor is hidden because of some other input event (e.g. touch),
we
> should dispatch a MouseExited event. Similarly, when the mouse-cursor is made
> visible again, we should dispatch a MouseEntered event. If we are not doing
> that, then that sounds like a bug we should fix. I seem to remember we
> implemented this behaviour a while back. Can you confirm with tdanderson@?

With the change in behaviour I don't think we need to address these "bugs".

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:147:
UpdateInkDropHoverState();
On 2015/10/16 18:41:16, Peter Kasting wrote:
> On 2015/10/16 14:57:21, bruthig wrote:
> > On 2015/10/15 22:21:33, Peter Kasting wrote:
> > > Why do you need this?
> > 
> > It is needed for the following situation:
> > 
> > 1.) Left mouse click on the Home button and drag outside the bounds of the
> > button.
> > 2.) Release the mouse. An OnMouseExited() event is NOT dispatched until the
> > mouse is moved.
> > 
> > By updating the status on an OnMouseReleased() we avoid having the hover
> > incorrectly shown if the mouse button is released outside the bounds of the
> > button.
> 
> It sounds like a bug to me that OnMouseExited() isn't fired when the mouse is
> down.  I think instead you should find and fix the cause of that.

I have changed the behavior such that the hover is not faded in after a ripple
animation.  So this is no longer needed.

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:160: void
ToolbarButton::OnMouseMoved(const ui::MouseEvent& event) {
On 2015/10/16 19:10:51, bruthig wrote:
> On 2015/10/16 18:41:16, Peter Kasting wrote:
> > On 2015/10/16 14:57:21, bruthig wrote:
> > > On 2015/10/15 22:21:33, Peter Kasting wrote:
> > > > Why do you need this override if you have the entered and exited
> overrides?
> > > 
> > > This was necessary for the following situation:
> > > 
> > > 1.) Mouse over the Back button causing hover to fade in
> > > 2.) Tap and hold the Back button until the menu shows. This causes the
hover
> > to
> > > disappear AND the mouse icon to disappear, making IsHovered() return
false.
> > > 3.) Hit escape to dismiss the menu. 
> > > At this point you are in a state where the mouse is hidden but is located
> > inside
> > > the bounds of the Back button. Updating the hover here makes the hover
fade
> > back
> > > in if the user moves the mouse because no OnMouseEntered() event will
occur.
> > 
> > I don't reproduce the mouse pointer disappearing when the button is held
down.
> 
> > Is this CrOS only?  That seems like a bug; I don't think we should be hiding
> the
> > mouse pointer or changing IsHovered() to return false.
> 
> I just want to confirm that you are indeed using a touch screen and tapping
with
> your finger for step 2?

FTR This is still needed for the following scenario:

1. Mouse enters (hover fades in)
2. Mouse Clicks (hover fades out and ripple performs the quick action animation)
At this point the mouse is within the bounds of the button but the hover is not
visible.
3. The mouse moves (the hover fades in).  IMO it doesn't make sense to rely on
getting an OnMouseEntered() here because the visibility of the cursor never
changed.

https://codereview.chromium.org/1390113006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:348:
UpdateInkDropHoverState();
On 2015/10/16 18:41:16, Peter Kasting wrote:
> On 2015/10/16 14:57:21, bruthig wrote:
> > On 2015/10/15 22:21:33, Peter Kasting wrote:
> > > Why do you need this?
> > 
> > Similar reason to why I needed it in ToolbarButton::OnMouseReleased(), the
> mouse
> > may or may not be inside the bounds of the button when the menu closes and
> > OnMouseEntered()/OnMouseExited() events are not dispatched while the menu is
> > being shown or when the menu is closed.  Oddly enough the OnMouseExited() is
> not
> > dispatched even when the mouse moves and it is over the web contents area.
> 
> Same reply as above.

This has been removed since the hover is turned off whenever a ripple animation
is triggered.

[Peter Kasting, 2015-11-02 19:32:58]

On 2015/11/02 16:42:31, bruthig wrote:
> I've updated the patch set so that the hover is not automatically shown after
a
> ripple animation.

That seems guaranteed to lead to glitchy-looking behavior with the mouse.  The
user will always move the mouse after clicking, usually immediately but
sometimes later.  At that point we fade the hover state back in.  Especially in
the case where the mouse movement is to leave the button (the common case), this
basically results in flickering the hover state.

This is why we don't do this for hover states normally; instead, pressing the
mouse changes to a pressed state (in your case, triggers some animation), but as
soon as the mouse is unpressed, the state goes back to HOVERED, not NORMAL.

We should be transitioning to hover as soon as the mouse button is released, not
on mouse move.

[sadrul, 2015-11-04 19:58:11]

On 2015/11/02 19:32:58, Peter Kasting wrote:
...
> We should be transitioning to hover as soon as the mouse button is released,
not
> on mouse move.

+1

https://codereview.chromium.org/1390113006/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:159:
UpdateInkDropHoverState();
Is it not sufficient to update the state from enter/exit only?

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_impl.cc (right):

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:30:
ink_drop_host_->AddInkDropLayer(root_layer_.get());
The layer is never removed from host?

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:74: return
hover_->IsVisible();
return hover_ && hover_->IsVisible()

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:156: }
What's the reason for the switch statement here?

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_hover.h (right):

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_hover.h:34: void FadeIn(const base::TimeDelta&
duration);
TimeDelta is a single int64, and we typically just send by value. (although now
that I look, we do use a const-ref in quite a few places ... hm)

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_hover.h:40: ui::Layer* root_layer() { return
layer_.get(); }
Why root_layer() instead of just layer()?

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_painted_layer_delegates.h (right):

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_painted_layer_delegates.h:101: }  // namespace views
You should split out a separate CL that just moves the existing delegates out
into a separate file, and introduce Rounded*Delegate in this feedback CL.

[sadrul, 2015-11-04 20:00:55]

For non-trivial CLs such as this, please make sure the CL description is
up-to-date, and contains enough information to describe the changes in the CL.
(i.e. something more than just 'add new feature' 'fix bug')

[bruthig, 2015-11-11 18:28:36]

Description was changed from

==========
Added material design mouse hover feedback support.

TEST=InkDropAnimationControllerFactoryTest.InitialState
TEST=InkDropAnimationControllerFactoryTest.SetHovered
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=MDInkDropAnimationControllerFactoryTest.InitialState
TEST=MDInkDropAnimationControllerFactoryTest.SetHovered
TEST=MDInkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

to

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.
- Added a new InkDropConsumer class that defines the interface that the
  InkDropAnimationController can interact with.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropAnimationControllerFactoryTest.InitialState
TEST=InkDropAnimationControllerFactoryTest.SetHovered
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=MDInkDropAnimationControllerFactoryTest.InitialState
TEST=MDInkDropAnimationControllerFactoryTest.SetHovered
TEST=MDInkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

[bruthig, 2015-11-11 21:48:44]

bruthig@chromium.org changed reviewers:
+ varkha@chromium.org

[bruthig, 2015-11-11 21:48:45]

I've updated the hover to delay for 1 second before fading in after an ink drop
animation completes.

varkha@, can you PTAL from a feature correctness point of view?

pkasting@, can you provide an OWNERs review for:
 - chrome/browser/ui/*

sadrul@, can you provide an OWNERs review for:
 - ui/views/*

https://codereview.chromium.org/1390113006/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:159:
UpdateInkDropHoverState();
On 2015/11/04 19:58:10, sadrul wrote:
> Is it not sufficient to update the state from enter/exit only?

Removed.

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_impl.cc (right):

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:30:
ink_drop_host_->AddInkDropLayer(root_layer_.get());
On 2015/11/04 19:58:10, sadrul wrote:
> The layer is never removed from host?

Whoops, done.

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:74: return
hover_->IsVisible();
On 2015/11/04 19:58:10, sadrul wrote:
> return hover_ && hover_->IsVisible()

Done.

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:156: }
On 2015/11/04 19:58:10, sadrul wrote:
> What's the reason for the switch statement here?

Removed.

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_hover.h (right):

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_hover.h:34: void FadeIn(const base::TimeDelta&
duration);
On 2015/11/04 19:58:11, sadrul wrote:
> TimeDelta is a single int64, and we typically just send by value. (although
now
> that I look, we do use a const-ref in quite a few places ... hm)

Seems like a reasonable abstraction to me :)

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_hover.h:40: ui::Layer* root_layer() { return
layer_.get(); }
On 2015/11/04 19:58:11, sadrul wrote:
> Why root_layer() instead of just layer()?

Done.

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_painted_layer_delegates.h (right):

https://codereview.chromium.org/1390113006/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_painted_layer_delegates.h:101: }  // namespace views
On 2015/11/04 19:58:11, sadrul wrote:
> You should split out a separate CL that just moves the existing delegates out
> into a separate file, and introduce Rounded*Delegate in this feedback CL.

Done.
https://codereview.chromium.org/1430153003/

[Peter Kasting, 2015-11-11 21:52:08]

It looks like this CL and https://codereview.chromium.org/1422593003/ contain
overlapping changes.  Please split things up so there's no overlap.

[bruthig, 2015-11-11 22:05:15]

On 2015/11/11 21:52:08, Peter Kasting wrote:
> It looks like this CL and https://codereview.chromium.org/1422593003/ contain
> overlapping changes.  Please split things up so there's no overlap.

Whoops, it looks like that other review wasn't properly diffed against this one
when uploaded.

[Peter Kasting, 2015-11-11 22:08:24]

LGTM.  I copied over the applicable comment from the other review.

https://codereview.chromium.org/1390113006/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:159:
UpdateInkDropHoverState();
Nit: It might be better to do this below the SetState() call, just in case any
future code ever wants to make the ink drop state depend on the current button
state.

[bruthig, 2015-11-12 18:21:08]

https://codereview.chromium.org/1390113006/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:159:
UpdateInkDropHoverState();
On 2015/11/11 22:08:24, Peter Kasting wrote:
> Nit: It might be better to do this below the SetState() call, just in case any
> future code ever wants to make the ink drop state depend on the current button
> state.

It certainly would make more sense, good thinking!

[Evan Stade, 2015-11-12 19:31:45]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2015-11-12 19:31:46]

https://codereview.chromium.org/1390113006/diff/260001/ui/views/animation/ink...
File ui/views/animation/ink_drop_consumer.h (right):

https://codereview.chromium.org/1390113006/diff/260001/ui/views/animation/ink...
ui/views/animation/ink_drop_consumer.h:13: // Pure virtual base class that
defines the API required of the ink drop
nit: s/Pure virtual base class/Interface/

https://codereview.chromium.org/1390113006/diff/260001/ui/views/animation/ink...
ui/views/animation/ink_drop_consumer.h:15: class VIEWS_EXPORT InkDropConsumer {
why is this not part of InkDropHost? It looks pretty funky to write
CreateInkDropController(this, this); (especially since it's this, this every
time it's called)

[bruthig, 2015-11-12 21:16:12]

https://codereview.chromium.org/1390113006/diff/260001/ui/views/animation/ink...
File ui/views/animation/ink_drop_consumer.h (right):

https://codereview.chromium.org/1390113006/diff/260001/ui/views/animation/ink...
ui/views/animation/ink_drop_consumer.h:15: class VIEWS_EXPORT InkDropConsumer {
On 2015/11/12 19:31:46, Evan Stade wrote:
> why is this not part of InkDropHost? It looks pretty funky to write
> CreateInkDropController(this, this); (especially since it's this, this every
> time it's called)

The thinking is that the InkDropHost could host multiple ink drops, with
different sizes and locations whereas there will be a one-to-one relationship of
InkDropConsumers to InkDropAnimationControllers.

Point taken tho, I will try and add a CreateInkDropController() method that
takes a single parameter, since that will be the common use case.

[bruthig, 2015-11-17 21:38:51]

sadrul@, friendly ping! Just wanted to make sure this one was on your radar :)

[bruthig, 2015-12-23 18:10:22]

Description was changed from

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.
- Added a new InkDropConsumer class that defines the interface that the
  InkDropAnimationController can interact with.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropAnimationControllerFactoryTest.InitialState
TEST=InkDropAnimationControllerFactoryTest.SetHovered
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=MDInkDropAnimationControllerFactoryTest.InitialState
TEST=MDInkDropAnimationControllerFactoryTest.SetHovered
TEST=MDInkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

to

==========
Added material design mouse hover feedback support.

DEPENDS ON:
 - https://codereview.chromium.org/1544613002/

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.
- Added a new InkDropConsumer class that defines the interface that the
  InkDropAnimationController can interact with.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropAnimationControllerFactoryTest.InitialState
TEST=InkDropAnimationControllerFactoryTest.SetHovered
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=MDInkDropAnimationControllerFactoryTest.InitialState
TEST=MDInkDropAnimationControllerFactoryTest.SetHovered
TEST=MDInkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

[bruthig, 2015-12-23 20:24:34]

Description was changed from

==========
Added material design mouse hover feedback support.

DEPENDS ON:
 - https://codereview.chromium.org/1544613002/

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.
- Added a new InkDropConsumer class that defines the interface that the
  InkDropAnimationController can interact with.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropAnimationControllerFactoryTest.InitialState
TEST=InkDropAnimationControllerFactoryTest.SetHovered
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=MDInkDropAnimationControllerFactoryTest.InitialState
TEST=MDInkDropAnimationControllerFactoryTest.SetHovered
TEST=MDInkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

to

==========
Added material design mouse hover feedback support.

DEPENDS ON:
 - https://codereview.chromium.org/1550443002/
 - https://codereview.chromium.org/1544613002/

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

[bruthig, 2015-12-23 20:31:35]

FYI I've made this CL slightly similar by extracting some
changes out in to https://codereview.chromium.org/1550443002/.

sadrul@, Can you PTAL?

https://codereview.chromium.org/1390113006/diff/260001/ui/views/animation/ink...
File ui/views/animation/ink_drop_consumer.h (right):

https://codereview.chromium.org/1390113006/diff/260001/ui/views/animation/ink...
ui/views/animation/ink_drop_consumer.h:13: // Pure virtual base class that
defines the API required of the ink drop
On 2015/11/12 19:31:46, Evan Stade wrote:
> nit: s/Pure virtual base class/Interface/

Done.

https://codereview.chromium.org/1390113006/diff/260001/ui/views/animation/ink...
ui/views/animation/ink_drop_consumer.h:15: class VIEWS_EXPORT InkDropConsumer {
On 2015/11/12 21:16:12, bruthig wrote:
> On 2015/11/12 19:31:46, Evan Stade wrote:
> > why is this not part of InkDropHost? It looks pretty funky to write
> > CreateInkDropController(this, this); (especially since it's this, this every
> > time it's called)
> 
> The thinking is that the InkDropHost could host multiple ink drops, with
> different sizes and locations whereas there will be a one-to-one relationship
of
> InkDropConsumers to InkDropAnimationControllers.
> 
> Point taken tho, I will try and add a CreateInkDropController() method that
> takes a single parameter, since that will be the common use case.

The InkDropConsumer has been removed.

[bruthig, 2016-01-08 21:48:11]

Description was changed from

==========
Added material design mouse hover feedback support.

DEPENDS ON:
 - https://codereview.chromium.org/1550443002/
 - https://codereview.chromium.org/1544613002/

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

to

==========
Added material design mouse hover feedback support.

DEPENDS ON:
 - https://codereview.chromium.org/1550443002/

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

[bruthig, 2016-01-08 21:48:44]

sadrul@, this patch is ready for review now as well.  Can you PTAL?

[bruthig, 2016-01-12 15:35:28]

Description was changed from

==========
Added material design mouse hover feedback support.

DEPENDS ON:
 - https://codereview.chromium.org/1550443002/

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

to

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

[sadrul, 2016-01-12 15:55:38]

https://codereview.chromium.org/1390113006/diff/580001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1390113006/diff/580001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:206: UpdateInkDropHoverState();
Can ButtonInkDropDelegate take care of updating the hover state itself (other
than the enable-ness change case)?

[bruthig, 2016-01-12 16:27:17]

sadrul@, PTAL

https://codereview.chromium.org/1390113006/diff/580001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1390113006/diff/580001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:206: UpdateInkDropHoverState();
On 2016/01/12 15:55:38, sadrul wrote:
> Can ButtonInkDropDelegate take care of updating the hover state itself (other
> than the enable-ness change case)?

Done.

[sadrul, 2016-01-22 15:23:18]

ui/views/ looks good. It'd be good to have varkha@ review before landing as
well. lgtm

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_impl.h (right):

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.h:51: base::Timer*
hover_after_animation_timer_for_test() {
Don't need this since FactoryTest is already a friend?

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.h:122:
scoped_ptr<base::Timer> hover_after_animation_timer_;
Why a scoped_ptr<>?

[varkha, 2016-01-22 15:45:33]

https://codereview.chromium.org/1390113006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:147:
UpdateInkDropHoverState();
Would this not be taken care by the delegate's OnMouseEvent?

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
File ui/views/animation/ink_drop_hover.cc (right):

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
ui/views/animation/ink_drop_hover.cc:98: case FADE_OUT:
Not sure if this could be an if(FADE_OUT).

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
File ui/views/animation/ink_drop_painted_layer_delegates.cc (right):

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
ui/views/animation/ink_drop_painted_layer_delegates.cc:100: return
gfx::PointF(size_.width() / 2.0f, size_.height() / 2.0f);
Could use gfx::Rect(size_)::CenterPoint().

[bruthig, 2016-01-25 22:37:43]

Description was changed from

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState

BUG=537238, 518919
==========

to

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.SetHoveredIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsNotHovered

BUG=537238, 518919
==========

[bruthig, 2016-01-25 22:39:17]

I've added some InkDropAnimationControllerImplTests.

sadrul@, can you take a look at:
 - ui/views/animation/ink_drop_hover_unittest.cc

varka@, PTAL.

https://codereview.chromium.org/1390113006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1390113006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:147:
UpdateInkDropHoverState();
On 2016/01/22 15:45:32, varkha wrote:
> Would this not be taken care by the delegate's OnMouseEvent?

Removed.

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_impl.h (right):

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.h:51: base::Timer*
hover_after_animation_timer_for_test() {
On 2016/01/22 15:23:18, sadrul wrote:
> Don't need this since FactoryTest is already a friend?

Removed.

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.h:122:
scoped_ptr<base::Timer> hover_after_animation_timer_;
On 2016/01/22 15:23:18, sadrul wrote:
> Why a scoped_ptr<>?

This was originally done so that the Timer could be destroyed when not in use. 
I've updated it to do this as well as added some tests.

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
File ui/views/animation/ink_drop_hover.cc (right):

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
ui/views/animation/ink_drop_hover.cc:98: case FADE_OUT:
On 2016/01/22 15:45:32, varkha wrote:
> Not sure if this could be an if(FADE_OUT).

Done.

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
File ui/views/animation/ink_drop_painted_layer_delegates.cc (right):

https://codereview.chromium.org/1390113006/diff/600001/ui/views/animation/ink...
ui/views/animation/ink_drop_painted_layer_delegates.cc:100: return
gfx::PointF(size_.width() / 2.0f, size_.height() / 2.0f);
On 2016/01/22 15:45:32, varkha wrote:
> Could use gfx::Rect(size_)::CenterPoint().

Done.

[bruthig, 2016-01-26 14:45:06]

We're planning to turn MD on by default today so I'm submitting this now. 
varkha@, sadrul@, PTAL at the added tests and changes since your previous
concerns.

[bruthig, 2016-01-26 14:45:16]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-01-26 14:45:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1390113006/#ps680001
(title: "Changed the InkDropAnimationControllerImpl to destroy its timer when
not running.")

[commit-bot: I haz the power, 2016-01-26 14:45:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390113006/680001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390113006/680001

[commit-bot: I haz the power, 2016-01-26 14:51:15]

Description was changed from

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.SetHoveredIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsNotHovered

BUG=537238, 518919
==========

to

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.SetHoveredIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsNotHovered

BUG=537238, 518919
==========

[commit-bot: I haz the power, 2016-01-26 14:51:16]

Committed patchset #35 (id:680001)

[commit-bot: I haz the power, 2016-01-26 14:52:17]

Description was changed from

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.SetHoveredIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsNotHovered

BUG=537238, 518919
==========

to

==========
Added material design mouse hover feedback support.

Overview of changes:
- Added InkDropHover class that manages animating a Layer to show
  visual feedback for mouse hover.
- Wired the InkDropHover class in to the InkDropAnimationControllerImpl.
- Wired some Views up to use the hover animation.

Overview of Hover behavior:
- Hover will fade in when mouse enters an enabled View
- Hover will fade out instantly if an ink drop animation is initiated.
- Hover will fade back in, after a delay, when an ink drop animation
  completes and the mouse is still hovering the View.

TEST=InkDropHoverTest.InitialStateAfterConstruction
TEST=InkDropHoverTest.IsHoveredStateTransitions
TEST=InkDropAnimationControllerFactoryTest.HoveredStateAfterAnimateToState
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.SetHoveredIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsHovered
TEST=HoveredStateAfterHoverTimerFiresWhenHostIsHovered.HoveredStateAfterHoverTimerFiresWhenHostIsNotHovered

BUG=537238, 518919

Committed: https://crrev.com/c657e3be3b1248d7e7908c10fe75dc2f191dccee
Cr-Commit-Position: refs/heads/master@{#371511}
==========

[commit-bot: I haz the power, 2016-01-26 14:52:18]

Patchset 35 (id:??) landed as
https://crrev.com/c657e3be3b1248d7e7908c10fe75dc2f191dccee
Cr-Commit-Position: refs/heads/master@{#371511}

[varkha, 2016-01-26 15:30:33]

After-the-fact lgtm.