Make the material design ripple effect more visible on a quick action (single click or single tap)

Make the material design ripple effect more visible on a quick action (single click or single tap)

Summary of changes:
 - Broke the InkDropState animations in to sub-state animations
 - Added DCHECKs to restrict InkDropState transitions in InkDropAnimation

BUG=551600
TEST=manual
TEST=InkDropAnimationTest.VerifyObserversAreNotified
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfSuccessfulAnimations
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfPreemptedAnimations
TEST=InkDropAnimationTest.AnimateToHiddenFromInvisibleState
TEST=InkDropAnimationTest.AnimateToHiddenFromVisibleState
TEST=InkDropAnimationTest.HideImmediately

Committed: https://crrev.com/6bf2a57d5965ced16eb8653aeb149bf194d5b4dc
Cr-Commit-Position: refs/heads/master@{#372406}

[bruthig, 2016-01-12 15:37:22]

Description was changed from

==========
[WIP - NOT FOR REVIEW] Material Design ink drop ripple prototype tweaks
==========

to

==========
[WIP - NOT FOR REVIEW] Material Design ink drop ripple prototype tweaks

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

[bruthig, 2016-01-12 16:10:32]

Description was changed from

==========
[WIP - NOT FOR REVIEW] Material Design ink drop ripple prototype tweaks

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

to

==========
[WIP - NOT FOR REVIEW] Material Design ink drop ripple prototype tweaks

DEPENDS ON: https://codereview.chromium.org/1390113006/

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

[bruthig, 2016-01-26 16:59:22]

Description was changed from

==========
[WIP - NOT FOR REVIEW] Material Design ink drop ripple prototype tweaks

DEPENDS ON: https://codereview.chromium.org/1390113006/

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

to

==========
[WIP - NOT FOR REVIEW] Material Design ink drop ripple prototype tweaks

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

[bruthig, 2016-01-27 16:05:33]

Description was changed from

==========
[WIP - NOT FOR REVIEW] Material Design ink drop ripple prototype tweaks

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

to

==========
Make the material design ripple effect more visible on a quick action (single
click or single tap)

Summary of changes:
 - Broke the InkDropState animations in to sub-state animations
 - Added DCHECKs to restrict InkDropState transitions in InkDropAnimation

BUG=551600
TEST=manual
==========

[bruthig, 2016-01-27 16:06:26]

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

[bruthig, 2016-01-27 16:06:26]

varkha@, can you please take a look from a feature correctness perspective?

sadrul@, can you please take a look from an OWNER's perspective?

[varkha, 2016-01-28 20:50:45]

Behaviour looks like a really good improvement. There are a few edge cases that
you know about (hover needs to disappear quicker when ripple starts, extra
animation when going from long-press to quick action on extension buttons).
Those can be addressed as a followup, no need to block.
A few nits for the comments.

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

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:189: duration =
kDeactivatedTransformDurationMs;
Maybe refactor this into a lookup table in future if |state| can be easily made
into an index into a table.

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.h (right):

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:43: // The valid InkDropState
transitions are defined below in (where * means any
nit: below in?
nit: Did you want to use '*' below?

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:79: // All in-progress animations will
be aborted.
Nit: How about "Imediately aborts all in-progress animations and hides the ink
drop. Sets the state to InkDropState::HIDDEN"?
This is a bit more forceful and I only hope it is still correct.

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:112: // animation will use the given
|duration|, |preemption_strategy|, and |tween|
Maybe break into a second sentence after |preemption_strategy|.

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:136: // and |observer| will be added to
all LayerAnimationSequences.
s/observer/animation_observer
Maybe also break out a second sentence after |preemption_strategy|.

[bruthig, 2016-01-28 22:37:14]

Description was changed from

==========
Make the material design ripple effect more visible on a quick action (single
click or single tap)

Summary of changes:
 - Broke the InkDropState animations in to sub-state animations
 - Added DCHECKs to restrict InkDropState transitions in InkDropAnimation

BUG=551600
TEST=manual
==========

to

==========
Make the material design ripple effect more visible on a quick action (single
click or single tap)

Summary of changes:
 - Broke the InkDropState animations in to sub-state animations
 - Added DCHECKs to restrict InkDropState transitions in InkDropAnimation

BUG=551600
TEST=manual
TEST=InkDropAnimationTest.VerifyObserversAreNotified
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfSuccessfulAnimations
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfPreemptedAnimations
TEST=InkDropAnimationTest.AnimateToHiddenFromInvisibleState
TEST=InkDropAnimationTest.AnimateToHiddenFromVisibleState
TEST=InkDropAnimationTest.HideImmediately
==========

[bruthig, 2016-01-28 22:38:00]

I have added some tests and some necessary test framework code as well as
addressed comments from previous patch sets.

varkha@, sadrul@, PTAL.

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

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:189: duration =
kDeactivatedTransformDurationMs;
On 2016/01/28 20:50:45, varkha wrote:
> Maybe refactor this into a lookup table in future if |state| can be easily
made
> into an index into a table.

Done.

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.h (right):

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:43: // The valid InkDropState
transitions are defined below in (where * means any
On 2016/01/28 20:50:45, varkha wrote:
> nit: below in?
> nit: Did you want to use '*' below?

Removed mention of '*'.

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:79: // All in-progress animations will
be aborted.
On 2016/01/28 20:50:45, varkha wrote:
> Nit: How about "Imediately aborts all in-progress animations and hides the ink
> drop. Sets the state to InkDropState::HIDDEN"?
> This is a bit more forceful and I only hope it is still correct.

Done.

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:112: // animation will use the given
|duration|, |preemption_strategy|, and |tween|
On 2016/01/28 20:50:45, varkha wrote:
> Maybe break into a second sentence after |preemption_strategy|.

Re-worded a bit, PTAL

https://codereview.chromium.org/1495753002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:136: // and |observer| will be added to
all LayerAnimationSequences.
On 2016/01/28 20:50:45, varkha wrote:
> s/observer/animation_observer
> Maybe also break out a second sentence after |preemption_strategy|.

Re-worded a bit, PTAL.

[varkha, 2016-01-29 00:11:59]

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

https://codereview.chromium.org/1495753002/diff/100001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:128: int kDurationTableInMs[] = {
nit: I think kAnimationDurationInMs[idx] would be more readable when used with
an index?

[bruthig, 2016-01-29 15:27:10]

varkha@, sadrul@, PTAL

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

https://codereview.chromium.org/1495753002/diff/100001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:128: int kDurationTableInMs[] = {
On 2016/01/29 00:11:59, varkha wrote:
> nit: I think kAnimationDurationInMs[idx] would be more readable when used with
> an index?

Done.

[varkha, 2016-01-29 19:14:47]

lgtm

[sadrul, 2016-01-29 20:12:27]

lgtm

[bruthig, 2016-01-29 20:22:47]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2016-01-29 20:25:01]

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

[commit-bot: I haz the power, 2016-01-29 20:37:21]

Description was changed from

==========
Make the material design ripple effect more visible on a quick action (single
click or single tap)

Summary of changes:
 - Broke the InkDropState animations in to sub-state animations
 - Added DCHECKs to restrict InkDropState transitions in InkDropAnimation

BUG=551600
TEST=manual
TEST=InkDropAnimationTest.VerifyObserversAreNotified
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfSuccessfulAnimations
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfPreemptedAnimations
TEST=InkDropAnimationTest.AnimateToHiddenFromInvisibleState
TEST=InkDropAnimationTest.AnimateToHiddenFromVisibleState
TEST=InkDropAnimationTest.HideImmediately
==========

to

==========
Make the material design ripple effect more visible on a quick action (single
click or single tap)

Summary of changes:
 - Broke the InkDropState animations in to sub-state animations
 - Added DCHECKs to restrict InkDropState transitions in InkDropAnimation

BUG=551600
TEST=manual
TEST=InkDropAnimationTest.VerifyObserversAreNotified
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfSuccessfulAnimations
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfPreemptedAnimations
TEST=InkDropAnimationTest.AnimateToHiddenFromInvisibleState
TEST=InkDropAnimationTest.AnimateToHiddenFromVisibleState
TEST=InkDropAnimationTest.HideImmediately
==========

[commit-bot: I haz the power, 2016-01-29 20:37:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-29 20:38:26]

Description was changed from

==========
Make the material design ripple effect more visible on a quick action (single
click or single tap)

Summary of changes:
 - Broke the InkDropState animations in to sub-state animations
 - Added DCHECKs to restrict InkDropState transitions in InkDropAnimation

BUG=551600
TEST=manual
TEST=InkDropAnimationTest.VerifyObserversAreNotified
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfSuccessfulAnimations
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfPreemptedAnimations
TEST=InkDropAnimationTest.AnimateToHiddenFromInvisibleState
TEST=InkDropAnimationTest.AnimateToHiddenFromVisibleState
TEST=InkDropAnimationTest.HideImmediately
==========

to

==========
Make the material design ripple effect more visible on a quick action (single
click or single tap)

Summary of changes:
 - Broke the InkDropState animations in to sub-state animations
 - Added DCHECKs to restrict InkDropState transitions in InkDropAnimation

BUG=551600
TEST=manual
TEST=InkDropAnimationTest.VerifyObserversAreNotified
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfSuccessfulAnimations
TEST=InkDropAnimationTest.VerifyObserversAreNotifiedOfPreemptedAnimations
TEST=InkDropAnimationTest.AnimateToHiddenFromInvisibleState
TEST=InkDropAnimationTest.AnimateToHiddenFromVisibleState
TEST=InkDropAnimationTest.HideImmediately

Committed: https://crrev.com/6bf2a57d5965ced16eb8653aeb149bf194d5b4dc
Cr-Commit-Position: refs/heads/master@{#372406}
==========

[commit-bot: I haz the power, 2016-01-29 20:38:27]

Patchset 7 (id:??) landed as
https://crrev.com/6bf2a57d5965ced16eb8653aeb149bf194d5b4dc
Cr-Commit-Position: refs/heads/master@{#372406}