Enhanced the InkDropRippleImpl to create/destroy InkDropAnimations as needed.

Enhanced the InkDropRippleImpl to create/destroy InkDropAnimations as needed.

NOTE: This is based on the CL here: https://codereview.chromium.org/1369393002/

Before this CL InkDropAnimations were created lazily and never destroyed or
released until the ripple owner was destroyed. This change adds the
necessary events required to destroy an InkDropAnimation when it becomes
hidden.

BUG=522175, 537614
Committed: https://crrev.com/09219a0b1d34102258dfc0d43d3f0bfdaedb1d3b
Cr-Commit-Position: refs/heads/master@{#353906}

[bruthig, 2015-09-28 20:32:53]

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

[bruthig, 2015-09-28 20:32:53]

varkha@, just an FYI at this point but, this CL lays the groundwork to
add/destroy ink drop ripples on an is-necessary basis.  From here we should be
able to swap in an InkDropAnimationPool quite easily if profiling shows it to be
beneficial.

[bruthig, 2015-10-05 20:56:23]

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

[bruthig, 2015-10-05 20:56:24]

pkasting@chromium.org: Please review changes in
 - chrome/browser/ui/views/toolbar/toolbar_button.cc

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

[Peter Kasting, 2015-10-05 21:01:54]

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

https://codereview.chromium.org/1373983002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:40: const int
kInkDropSmallCornerRadius = 2;
Is there some way to calculate these sizes based on the button size?  How did we
arrive at these sizes to start?  I really don't like the way all the ink drop
changes everywhere have just hardcoded lots of sizes without reference to
anything else.

https://codereview.chromium.org/1373983002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:148: return;
Why this return statement?

[bruthig, 2015-10-06 15:31:38]

pkasting@, Can you PTAL?

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

https://codereview.chromium.org/1373983002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:40: const int
kInkDropSmallCornerRadius = 2;
On 2015/10/05 21:01:54, Peter Kasting wrote:
> Is there some way to calculate these sizes based on the button size?  How did
we
> arrive at these sizes to start?  I really don't like the way all the ink drop
> changes everywhere have just hardcoded lots of sizes without reference to
> anything else.

The sizes were given by the designers.  I don't think there's a good way to
calculate them from the button sizes because the ripple size is the same for
both Material and Material-Hybrid, which have different button sizes.  FTR the
plan is to move these constants to a common place.

https://codereview.chromium.org/1373983002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:148: return;
On 2015/10/05 21:01:54, Peter Kasting wrote:
> Why this return statement?

Removed.

[varkha, 2015-10-06 20:58:38]

A few nits.

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:267: if
(!observers_.HasObserver(observer))
When is this the case? Do we need this?

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:478: void
InkDropAnimation::SetCenterPoint(const gfx::Point& center_point) {
nit: Should probably be moved above to reflect the order in .h file.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:517: for (int i = 0; i <
PAINTED_SHAPE_COUNT; ++i)
Will range syntax work here (and in SetTransforms and other similar places)?

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:135: // |ink_drop_state| animation have
started.
Can you document |observer|?

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller.h:32: // Animates from the
current InkDropState to |state|.
nit: s/state/ink_drop_state?

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_observer.h:24: // The animation was
stopped pre-maturely before reaching it's final state.
nit: s/pre-maturely/prematurely ; s/it's/its

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_observer.h:33: // then the animation has
progressed to it's final frame however if |reason|
nit: here and below - s/it's/its.

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_state.h:21: // The quick action for the view has
been triggered. e.g. A tap gesture to
nit: here and below s/. e.g. A tap/, e.g. a tap.

[Peter Kasting, 2015-10-06 21:50:24]

LGTM

[sadrul, 2015-10-08 01:06:01]

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:267: if
(!observers_.HasObserver(observer))
On 2015/10/06 20:58:38, varkha wrote:
> When is this the case? Do we need this?

+1 Let's remove this.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:534: observer.aborted_count()
Is this the only reason to send the observer back with the callback? Would it
make sense to just send that information explicitly (e.g. a boolean/enum to say
everything went well, or some didn't make it) instead?

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:57: void
RemoveObserver(InkDropAnimationObserver* observer);
- the newline

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:88: if (reason ==
SUCCESS) {
early return instead

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.h:9: #include
"base/observer_list.h"
This isn't needed?

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_observer.h:34: // is |PRE_EMPTED| then the
animation was stopped before it's final frame.
Is it possible to start the animation for one state while another animation was
in progress? If that is the case, then I assume Ended() is triggered with
PRE_EMPTED before Started() is called again for the new state? Can you clarify
that in this comment?

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_observer.h:46: VIEWS_EXPORT std::ostream&
operator<<(
Is this actually useful/necessary?

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_state.h:39: 
Same here: is this necessary/useful? We have generally not used these, and
instead went with ToString when doing string output for things (see various
gfx::Point etc. things). Maybe go with that?

[bruthig, 2015-10-08 21:42:55]

sadrul@, varkha@, can you PTAL?

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:267: if
(!observers_.HasObserver(observer))
On 2015/10/08 01:06:01, sadrul wrote:
> On 2015/10/06 20:58:38, varkha wrote:
> > When is this the case? Do we need this?
> 
> +1 Let's remove this.

It was originally intended as a safe-guard to ensure observers aren't added
multiple times.  It appeared to be a common pattern elsewhere.
I have removed it though.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:478: void
InkDropAnimation::SetCenterPoint(const gfx::Point& center_point) {
On 2015/10/06 20:58:37, varkha wrote:
> nit: Should probably be moved above to reflect the order in .h file.

Done.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:517: for (int i = 0; i <
PAINTED_SHAPE_COUNT; ++i)
On 2015/10/06 20:58:38, varkha wrote:
> Will range syntax work here (and in SetTransforms and other similar places)?

It appears there isn't built in support for range based for loops on enums (See
http://stackoverflow.com/questions/14965174/misunderstanding-of-range-based-f...).
There are some work arounds to make it work but you have to overload the * and
++ operator on the enum type and I don't think we have a precedent set for that
in our coding guidelines.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:534: observer.aborted_count()
On 2015/10/08 01:06:01, sadrul wrote:
> Is this the only reason to send the observer back with the callback? Would it
> make sense to just send that information explicitly (e.g. a boolean/enum to
say
> everything went well, or some didn't make it) instead?

In the current use case yes, this is the only data required by the callback,
however I figured it was more future proof to pass the observer in case the
observer is enhanced to provide more detailed information.

What would the benefits be of passing an enum or bool?

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:57: void
RemoveObserver(InkDropAnimationObserver* observer);
On 2015/10/08 01:06:01, sadrul wrote:
> - the newline

Done.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:135: // |ink_drop_state| animation have
started.
On 2015/10/06 20:58:38, varkha wrote:
> Can you document |observer|?

Done.

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller.h:32: // Animates from the
current InkDropState to |state|.
On 2015/10/06 20:58:38, varkha wrote:
> nit: s/state/ink_drop_state?

Done.

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:88: if (reason ==
SUCCESS) {
On 2015/10/08 01:06:01, sadrul wrote:
> early return instead

Done.

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.h:9: #include
"base/observer_list.h"
On 2015/10/08 01:06:01, sadrul wrote:
> This isn't needed?

Whoops, not yet at least. Removed.

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_observer.h:24: // The animation was
stopped pre-maturely before reaching it's final state.
On 2015/10/06 20:58:38, varkha wrote:
> nit: s/pre-maturely/prematurely ; s/it's/its

Done.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_observer.h:33: // then the animation has
progressed to it's final frame however if |reason|
On 2015/10/06 20:58:38, varkha wrote:
> nit: here and below - s/it's/its.

Done.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_observer.h:34: // is |PRE_EMPTED| then the
animation was stopped before it's final frame.
On 2015/10/08 01:06:01, sadrul wrote:
> Is it possible to start the animation for one state while another animation
was
> in progress? If that is the case, then I assume Ended() is triggered with
> PRE_EMPTED before Started() is called again for the new state? Can you clarify
> that in this comment?

Done.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_observer.h:46: VIEWS_EXPORT std::ostream&
operator<<(
On 2015/10/08 01:06:01, sadrul wrote:
> Is this actually useful/necessary?

Replaced with a ToString().

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_state.h:21: // The quick action for the view has
been triggered. e.g. A tap gesture to
On 2015/10/06 20:58:38, varkha wrote:
> nit: here and below s/. e.g. A tap/, e.g. a tap.

Done.

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_state.h:39: 
On 2015/10/08 01:06:01, sadrul wrote:
> Same here: is this necessary/useful? We have generally not used these, and
> instead went with ToString when doing string output for things (see various
> gfx::Point etc. things). Maybe go with that?

Replaced with ToString().

[sadrul, 2015-10-09 20:36:27]

lgtm

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

https://codereview.chromium.org/1373983002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:534: observer.aborted_count()
On 2015/10/08 21:42:55, bruthig wrote:
> On 2015/10/08 01:06:01, sadrul wrote:
> > Is this the only reason to send the observer back with the callback? Would
it
> > make sense to just send that information explicitly (e.g. a boolean/enum to
> say
> > everything went well, or some didn't make it) instead?
> 
> In the current use case yes, this is the only data required by the callback,
> however I figured it was more future proof to pass the observer in case the
> observer is enhanced to provide more detailed information.
> 
> What would the benefits be of passing an enum or bool?

Simplicity. It's not complex as it is now, so that's OK too.

[bruthig, 2015-10-13 21:27:02]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-10-13 21:27:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1373983002/#ps120001
(title: "Based diff delta on dependant branch.")

[commit-bot: I haz the power, 2015-10-13 21:31:09]

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

[commit-bot: I haz the power, 2015-10-13 22:12:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-13 22:12:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bruthig, 2015-10-13 22:16:31]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-10-13 22:16:31]

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/1373983002/#ps140001
(title: "Updated InkDropAnimationTests.")

[commit-bot: I haz the power, 2015-10-13 22:18:36]

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

[commit-bot: I haz the power, 2015-10-13 22:28:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-13 22:28:43]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bruthig, 2015-10-13 22:32:58]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-10-13 22:32:58]

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/1373983002/#ps160001
(title: "Fixed merge with master.")

[commit-bot: I haz the power, 2015-10-13 22:34:59]

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

[commit-bot: I haz the power, 2015-10-13 23:40:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-13 23:42:13]

Patchset 9 (id:??) landed as
https://crrev.com/09219a0b1d34102258dfc0d43d3f0bfdaedb1d3b
Cr-Commit-Position: refs/heads/master@{#353906}