Made material design ink drop QUICK_ACTION animation more visible.

DO NOT REVIEW.  Will be replaced by: https://codereview.chromium.org/1495753002/

Made material design ink drop QUICK_ACTION animation more visible.

Depends on: https://codereview.chromium.org/1390113006

The material design ink drop QUICK_ACTION animation was fading out to 0
opacity to quickly and thus not visible enough to the user. This CL adds the
concept of ripple sub-animations to the InkDropAnimation so that the animation
sequences can be more finely controlled. In addition to that the ripple
animation sequences have been tweaked to use different tween types and visible
opacity pauses prior to fading out have been added.

BUG=551600

[bruthig, 2015-10-21 21:16:48]

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

[bruthig, 2015-10-21 21:16:48]

varkha@, can you take a first look?

[varkha, 2015-10-22 18:33:08]

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:68: // The time taken to animate the
fade out portion of the SLOW_ACTION animation.
s/SLOW_ACTION/DEACTIVATED?

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:85: enum InkDropSubAnimations {
Should this enum be above the time constants since they refer to the enum
values?

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:95: QUICK_ACTION_VISIBLE,
nit: White space after this line for consistency.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:131: // // Returns the InkDropState
animation duration for the given |state|.
nit: extra //.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:278: case InkDropState::ACTION_PENDING:
General comment in this method. The code seems like it could be reduced by using
a helper method that does AnimateToOpacity, another optional AnimateToOpacity
and then AnimateToTransforms. The constants differ as well as whether you have
an optional step but otherwise handling is common, no?

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:406: root_layer_->SetVisible(false);
Just a question: did it matter that this is now the last step rather than the
first?

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:558: .Length() /
nit: indentation looks a bit odd here.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:564: // Calculate the ratio of how far
the transformed circle's top most point is
nit: topmost.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:566: const float
top_point_distance_estimate = fabs(
When is the argument of fabs ever negative?

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:567: circle_top_point.y() /
static_cast<float>(large_size_.height()) / 2.0f);
Are the brackets correct here? Is it not y/(h/2) similar to the L/(diag/2)
above? Maybe I will need an offline explanation of what this does.

[bruthig, 2015-11-04 21:50:15]

Description was changed from

==========
Made material design ink drop QUICK_ACTION animation more visible.

Depends on: https://codereview.chromium.org/1390113006

The material design ink drop QUICK_ACTION animation was fading out to 0
opacity to quickly and thus not visible enough to the user. This CL
breaks the animations in to sub animations so that part of the
transformations animations can be at full visibility and some can be
fading out to 0 opacity.
==========

to

==========
Made material design ink drop QUICK_ACTION animation more visible.

Depends on: https://codereview.chromium.org/1390113006

The material design ink drop QUICK_ACTION animation was fading out to 0
opacity to quickly and thus not visible enough to the user. This CL adds the
concept of ripple sub-animations to the InkDropAnimation so that the animation
sequences can be more finely controlled. In addition to that the ripple
animation sequences have been tweaked to use different tween types and visible
opacity pauses prior to fading out have been added.

BUG=551600
==========

[bruthig, 2015-11-04 21:51:02]

varkha@, I've addressed your previous comments and included our offline tweaks. 
Can you PTAL?

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:68: // The time taken to animate the
fade out portion of the SLOW_ACTION animation.
On 2015/10/22 18:33:08, varkha wrote:
> s/SLOW_ACTION/DEACTIVATED?

Done.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:85: enum InkDropSubAnimations {
On 2015/10/22 18:33:07, varkha wrote:
> Should this enum be above the time constants since they refer to the enum
> values?

Done.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:95: QUICK_ACTION_VISIBLE,
On 2015/10/22 18:33:08, varkha wrote:
> nit: White space after this line for consistency.

Done.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:131: // // Returns the InkDropState
animation duration for the given |state|.
On 2015/10/22 18:33:08, varkha wrote:
> nit: extra //.

Done.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:278: case InkDropState::ACTION_PENDING:
On 2015/10/22 18:33:08, varkha wrote:
> General comment in this method. The code seems like it could be reduced by
using
> a helper method that does AnimateToOpacity, another optional AnimateToOpacity
> and then AnimateToTransforms. The constants differ as well as whether you have
> an optional step but otherwise handling is common, no?

I'm not so sure how to reduce the amount of code here.  The bulk of the code is
parameter values which is necessary.  I'm open to more concrete suggestions
though.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:406: root_layer_->SetVisible(false);
On 2015/10/22 18:33:07, varkha wrote:
> Just a question: did it matter that this is now the last step rather than the
> first?

Nope

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:558: .Length() /
On 2015/10/22 18:33:08, varkha wrote:
> nit: indentation looks a bit odd here.

This is the result of 'git cl format'.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:564: // Calculate the ratio of how far
the transformed circle's top most point is
On 2015/10/22 18:33:08, varkha wrote:
> nit: topmost.

Done.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:566: const float
top_point_distance_estimate = fabs(
On 2015/10/22 18:33:08, varkha wrote:
> When is the argument of fabs ever negative?

circle_top_point.y() is expected to be negative since the center of the circle
is at (0, 0).  I've updated the formula slightly to hopefully be more clear.

https://codereview.chromium.org/1422593003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:567: circle_top_point.y() /
static_cast<float>(large_size_.height()) / 2.0f);
On 2015/10/22 18:33:07, varkha wrote:
> Are the brackets correct here? Is it not y/(h/2) similar to the L/(diag/2)
> above? Maybe I will need an offline explanation of what this does.

Done.

[varkha, 2015-11-06 00:00:28]

lgtm with a few questions / nits.

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:155: duration =
kHiddenFadeOutAnimationDurationMs;
nit: Maybe drop Animation from all of those for consistency.

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:306: AbortAllAnimations();
Why is this necessary given that we have called it above?

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller.h (right):

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller.h:35: // Returns true if the
ink drop animation will automatically transition to the
nit: s/will/should if you agree the name ShouldAutoAnimateToHidden better
reflects what this does.

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

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:66: bool
InkDropAnimationControllerImpl::WillAutoAnimateToHidden() const {
nit: Maybe ShouldAutoAnimateToHidden? It is not really a promise as much as a
configuration dictated by this implementation.

[bruthig, 2015-11-11 18:11:20]

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:155: duration =
kHiddenFadeOutAnimationDurationMs;
On 2015/11/06 00:00:27, varkha wrote:
> nit: Maybe drop Animation from all of those for consistency.

Done.

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:306: AbortAllAnimations();
On 2015/11/06 00:00:27, varkha wrote:
> Why is this necessary given that we have called it above?

The call to AbortAllAnimations() above is guarded by the previous value of
|ink_drop_state_| and this one is based on the new value.

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

https://codereview.chromium.org/1422593003/diff/110001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:66: bool
InkDropAnimationControllerImpl::WillAutoAnimateToHidden() const {
On 2015/11/06 00:00:27, varkha wrote:
> nit: Maybe ShouldAutoAnimateToHidden? It is not really a promise as much as a
> configuration dictated by this implementation.

From the perspective of the InkDropAnimationController clients this is a
promise. For example in ToolbarButton::OnGestureEvent() it checks
WillAutoAnimateToHidden() to know whether to pass along an
AnimateToState(HIDDEN) or not.  If this is not a promise then clients would not
be able to depend on it.

Having said that, I'm not too keen on the name and am open to changing it but
I'm hesitant to use 'Should' because that implies it isn't a promise when I
actually want it to be.

[bruthig, 2015-11-11 18:13:05]

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

[bruthig, 2015-11-11 18:13:06]

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/*

[bruthig, 2015-11-11 18:13:46]

bruthig@chromium.org changed reviewers:
+ pkasting@chromium.org
- pkasting@google.com

[varkha, 2015-11-11 19:02:24]

slgtm.

https://codereview.chromium.org/1422593003/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1422593003/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.h:105:
scoped_ptr<views::InkDropAnimationController> ink_drop_animation_controller_;
I think in the refactoring stage we should rename InkDropAnimationController to
something like ButtonAnimationController or ButtonAnimator since it is dealing
with more than just ink drop ripple. This CL is clearer without this rename
though so I suggest we do it separately.

https://codereview.chromium.org/1422593003/diff/150001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_factory_unittest.cc
(right):

https://codereview.chromium.org/1422593003/diff/150001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_factory_unittest.cc:23: #define
TEST_ALL_INK_DROPS(test_name, test_code)           \
Wouldn't this (testing both with and without MD flags set) be simpler (and
easily extensible to add MD-HYBRID) with TEST_P and 
testing::WithParamInterface?

[Peter Kasting, 2015-11-11 21:41:38]

LGTM

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

https://codereview.chromium.org/1422593003/diff/150001/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.

https://codereview.chromium.org/1422593003/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:203: // would prematurely
pre-empt these animations.
Nit: I'd move this comment above the conditional, reverse the conditional, and
put the AnimateToState() call inside it.

[bruthig, 2015-11-11 22:04:41]

On 2015/11/11 21:41:38, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/1422593003/diff/150001/chrome/browser/ui/view...
> File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):
> 
>
https://codereview.chromium.org/1422593003/diff/150001/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.
> 
>
https://codereview.chromium.org/1422593003/diff/150001/chrome/browser/ui/view...
> chrome/browser/ui/views/toolbar/toolbar_button.cc:203: // would prematurely
> pre-empt these animations.
> Nit: I'd move this comment above the conditional, reverse the conditional, and
> put the AnimateToState() call inside it.

Whoops, I guess the patch set you looked at wasn't diff'ed against the ink drop
hover work.  Patch set 10 should be properly diffed on it.  I will address your
comments in the other review here: https://codereview.chromium.org/1390113006/

[Peter Kasting, 2015-11-11 22:07:21]

LGTM.  I copied over the one still-applicable comment from before.

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

https://codereview.chromium.org/1422593003/diff/170001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:203: // would prematurely
pre-empt these animations.
Nit: I'd move this comment above the conditional, reverse the conditional, and
put the AnimateToState() call inside it.

[bruthig, 2015-11-12 18:30:30]

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

https://codereview.chromium.org/1422593003/diff/170001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:203: // would prematurely
pre-empt these animations.
On 2015/11/11 22:07:21, Peter Kasting wrote:
> Nit: I'd move this comment above the conditional, reverse the conditional, and
> put the AnimateToState() call inside it.

Done.

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

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

[bruthig, 2016-01-12 15:36:43]

On 2015/11/17 21:38:59, bruthig wrote:
> sadrul@, friendly ping! Just wanted to make sure this one was on your radar :)

DO NOT REVIEW.  Will be replaced by: https://codereview.chromium.org/1495753002/

[bruthig, 2016-01-12 15:36:59]

Description was changed from

==========
Made material design ink drop QUICK_ACTION animation more visible.

Depends on: https://codereview.chromium.org/1390113006

The material design ink drop QUICK_ACTION animation was fading out to 0
opacity to quickly and thus not visible enough to the user. This CL adds the
concept of ripple sub-animations to the InkDropAnimation so that the animation
sequences can be more finely controlled. In addition to that the ripple
animation sequences have been tweaked to use different tween types and visible
opacity pauses prior to fading out have been added.

BUG=551600
==========

to

==========
DO NOT REVIEW.  Will be replaced by: https://codereview.chromium.org/1495753002/

Made material design ink drop QUICK_ACTION animation more visible.

Depends on: https://codereview.chromium.org/1390113006

The material design ink drop QUICK_ACTION animation was fading out to 0
opacity to quickly and thus not visible enough to the user. This CL adds the
concept of ripple sub-animations to the InkDropAnimation so that the animation
sequences can be more finely controlled. In addition to that the ripple
animation sequences have been tweaked to use different tween types and visible
opacity pauses prior to fading out have been added.

BUG=551600
==========