Add MD Ink Drop Layers to InkDropHost only when a ripple/hover is active.

Add MD Ink Drop to host only when a ripple/hover is active.

In order to paint the material design ink drop behind button icons the icon View
and the button View have to paint to their own Layers. There is no need to incur
the additional expense of these Layers unless the ink drop ripple or hover is
actually active on the View.

BUG=604415
TEST=InkDropHoverTest.*
TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationControllerFactoryTest.VerifyInkDropLayersRemovedAfterDestructionWhenHoverIsActive

Committed: https://crrev.com/741be72a4d24789a876d87d0d94e9e2e1cdcd674
Cr-Commit-Position: refs/heads/master@{#390696}

[bruthig, 2016-04-18 20:56:38]

Description was changed from

==========
Add MD Ink Drop to host only when a ripple/hover is active.

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

BUG=604415
==========

to

==========
Add MD Ink Drop to host only when a ripple/hover is active.

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

BUG=604415
==========

[bruthig, 2016-04-25 15:44:08]

Description was changed from

==========
Add MD Ink Drop to host only when a ripple/hover is active.

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

BUG=604415
==========

to

==========
Add MD Ink Drop to host only when a ripple/hover is active.

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

In order to paint the material design ink drop behind button icons the icon View
and the button View have to paint to their own Layers. There is no need to incur
the additional expense of these Layers unless the ink drop ripple or hover is
actually active on the View.

BUG=604415

TEST=InkDropHoverTest.*
TEST=InkDropAnimationControllerImplTest.*
==========

[bruthig, 2016-04-25 15:44:55]

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

[bruthig, 2016-04-25 15:44:56]

varkha@, can you PTAL?

[varkha, 2016-04-28 20:35:01]

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

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:66:
DestroyInkDropAnimation();
Don't you need to call DestroyInkDropHover() here as well to guarantee that
RemoveRootLayerFromHost() is not a NOP? If not then RemoveRootLayerFromHost() is
already called in DestroyInkDropAnimation() so it is redundant, no?

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:145:
hover_->set_observer(this);
Do you ever need to set it back to nullptr? I think you don't because hover or
ink drop animation lifetime is controlled from |this| but just checking.

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:159: if
(!root_layer_added_to_host_ && (hover_ || ink_drop_animation_)) {
Shouldn't it be possible to DCHECK(hover_ || ink_drop_animation_) instead of
using it in condition?

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:195: 
Maybe a // views::InkDropHoverObserver: banner comment here (and the other
interface class comment before the 2 similar methods above) just to clarify
those handlers.

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

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.h:72: void
AddRootLayerToHost();
MaybeAddRootLayerToHost? Also I think it could be possible to document right
here when that "maybe" is true.

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.h:121: // The current
InkDropHover. Lazily created using CreateInkDropHover();
Not sure about it myself and definitely not in this CL but maybe InkDropHover
should be InkDropHoverAnimation and InkDropAnimation should be
InkDropHoverRippleAnimation. alternatively consider simpler InkDropHover and
InkDropRipple. Either will bring the names closer to point the similarities.

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl_unittest.cc:141:
TEST_F(InkDropAnimationControllerImplTest, LayersRemovedFromHostAfterHover) {
Those are really nice!

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
File ui/views/animation/ink_drop_hover_observer.h (right):

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_hover_observer.h:32: InkDropHoverObserver() {}
= default here and for dtor.

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/tes...
File ui/views/animation/test/test_ink_drop_hover_observer.h (right):

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_hover_observer.h:23:
TestInkDropHoverObserver();
= default for ctor / dtor?

[bruthig, 2016-04-29 11:02:35]

Updated, varkha@, can you PTAL?

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

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:66:
DestroyInkDropAnimation();
On 2016/04/28 20:35:00, varkha wrote:
> Don't you need to call DestroyInkDropHover() here as well to guarantee that
> RemoveRootLayerFromHost() is not a NOP? If not then RemoveRootLayerFromHost()
is
> already called in DestroyInkDropAnimation() so it is redundant, no?

Good catch!  Fixed and test added.

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:145:
hover_->set_observer(this);
On 2016/04/28 20:35:00, varkha wrote:
> Do you ever need to set it back to nullptr? I think you don't because hover or
> ink drop animation lifetime is controlled from |this| but just checking.

Updated to explicitly set_observer(nullptr) in DestroyInkDropHover() to ensure
notifications aren't happening during ~InkDropAnimationControllerImpl();

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:159: if
(!root_layer_added_to_host_ && (hover_ || ink_drop_animation_)) {
On 2016/04/28 20:35:00, varkha wrote:
> Shouldn't it be possible to DCHECK(hover_ || ink_drop_animation_) instead of
> using it in condition?

Done.

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:195: 
On 2016/04/28 20:35:00, varkha wrote:
> Maybe a // views::InkDropHoverObserver: banner comment here (and the other
> interface class comment before the 2 similar methods above) just to clarify
> those handlers.

Done.

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

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.h:72: void
AddRootLayerToHost();
On 2016/04/28 20:35:00, varkha wrote:
> MaybeAddRootLayerToHost? Also I think it could be possible to document right
> here when that "maybe" is true.

I've opted to use the 'IfNeeded' Suffix.  WDYT?

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.h:121: // The current
InkDropHover. Lazily created using CreateInkDropHover();
On 2016/04/28 20:35:00, varkha wrote:
> Not sure about it myself and definitely not in this CL but maybe InkDropHover
> should be InkDropHoverAnimation and InkDropAnimation should be
> InkDropHoverRippleAnimation. alternatively consider simpler InkDropHover and
> InkDropRipple. Either will bring the names closer to point the similarities.

100% agreed.  I was thinking InkDropRipple and InkDropHover.  I just didn't want
to do this rename while I was still merging stuff back to M51.

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
File ui/views/animation/ink_drop_hover_observer.h (right):

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_hover_observer.h:32: InkDropHoverObserver() {}
On 2016/04/28 20:35:00, varkha wrote:
> = default here and for dtor.

Done.

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/tes...
File ui/views/animation/test/test_ink_drop_hover_observer.h (right):

https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_hover_observer.h:23:
TestInkDropHoverObserver();
On 2016/04/28 20:35:00, varkha wrote:
> = default for ctor / dtor?

I was able to do this for the dtor but the ctor complained "Complex constructor
has an inlined body."  This is due to having the templated parent class.

[varkha, 2016-04-29 14:45:58]

On 2016/04/29 11:02:35, bruthig wrote:
> Updated, varkha@, can you PTAL?
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> File ui/views/animation/ink_drop_animation_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> ui/views/animation/ink_drop_animation_controller_impl.cc:66:
> DestroyInkDropAnimation();
> On 2016/04/28 20:35:00, varkha wrote:
> > Don't you need to call DestroyInkDropHover() here as well to guarantee that
> > RemoveRootLayerFromHost() is not a NOP? If not then
RemoveRootLayerFromHost()
> is
> > already called in DestroyInkDropAnimation() so it is redundant, no?
> 
> Good catch!  Fixed and test added.
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> ui/views/animation/ink_drop_animation_controller_impl.cc:145:
> hover_->set_observer(this);
> On 2016/04/28 20:35:00, varkha wrote:
> > Do you ever need to set it back to nullptr? I think you don't because hover
or
> > ink drop animation lifetime is controlled from |this| but just checking.
> 
> Updated to explicitly set_observer(nullptr) in DestroyInkDropHover() to ensure
> notifications aren't happening during ~InkDropAnimationControllerImpl();
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> ui/views/animation/ink_drop_animation_controller_impl.cc:159: if
> (!root_layer_added_to_host_ && (hover_ || ink_drop_animation_)) {
> On 2016/04/28 20:35:00, varkha wrote:
> > Shouldn't it be possible to DCHECK(hover_ || ink_drop_animation_) instead of
> > using it in condition?
> 
> Done.
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> ui/views/animation/ink_drop_animation_controller_impl.cc:195: 
> On 2016/04/28 20:35:00, varkha wrote:
> > Maybe a // views::InkDropHoverObserver: banner comment here (and the other
> > interface class comment before the 2 similar methods above) just to clarify
> > those handlers.
> 
> Done.
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> File ui/views/animation/ink_drop_animation_controller_impl.h (right):
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> ui/views/animation/ink_drop_animation_controller_impl.h:72: void
> AddRootLayerToHost();
> On 2016/04/28 20:35:00, varkha wrote:
> > MaybeAddRootLayerToHost? Also I think it could be possible to document right
> > here when that "maybe" is true.
> 
> I've opted to use the 'IfNeeded' Suffix.  WDYT?
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> ui/views/animation/ink_drop_animation_controller_impl.h:121: // The current
> InkDropHover. Lazily created using CreateInkDropHover();
> On 2016/04/28 20:35:00, varkha wrote:
> > Not sure about it myself and definitely not in this CL but maybe
InkDropHover
> > should be InkDropHoverAnimation and InkDropAnimation should be
> > InkDropHoverRippleAnimation. alternatively consider simpler InkDropHover and
> > InkDropRipple. Either will bring the names closer to point the similarities.
> 
> 100% agreed.  I was thinking InkDropRipple and InkDropHover.  I just didn't
want
> to do this rename while I was still merging stuff back to M51.
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> File ui/views/animation/ink_drop_hover_observer.h (right):
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/ink...
> ui/views/animation/ink_drop_hover_observer.h:32: InkDropHoverObserver() {}
> On 2016/04/28 20:35:00, varkha wrote:
> > = default here and for dtor.
> 
> Done.
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/tes...
> File ui/views/animation/test/test_ink_drop_hover_observer.h (right):
> 
>
https://codereview.chromium.org/1897073002/diff/140001/ui/views/animation/tes...
> ui/views/animation/test/test_ink_drop_hover_observer.h:23:
> TestInkDropHoverObserver();
> On 2016/04/28 20:35:00, varkha wrote:
> > = default for ctor / dtor?
> 
> I was able to do this for the dtor but the ctor complained "Complex
constructor
> has an inlined body."  This is due to having the templated parent class.

lgtm. (Still something with the templates on Win).

[bruthig, 2016-04-29 16:16:02]

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

[bruthig, 2016-04-29 16:16:03]

sky@, can you take a look at:
- ui/views/views.gyp

[bruthig, 2016-04-29 16:17:09]

Description was changed from

==========
Add MD Ink Drop to host only when a ripple/hover is active.

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

In order to paint the material design ink drop behind button icons the icon View
and the button View have to paint to their own Layers. There is no need to incur
the additional expense of these Layers unless the ink drop ripple or hover is
actually active on the View.

BUG=604415

TEST=InkDropHoverTest.*
TEST=InkDropAnimationControllerImplTest.*
==========

to

==========
Add MD Ink Drop to host only when a ripple/hover is active.

In order to paint the material design ink drop behind button icons the icon View
and the button View have to paint to their own Layers. There is no need to incur
the additional expense of these Layers unless the ink drop ripple or hover is
actually active on the View.

BUG=604415

TEST=InkDropHoverTest.*
TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationControllerFactoryTest.VerifyInkDropLayersRemovedAfterDestructionWhenHoverIsActive
==========

[sky, 2016-04-29 17:19:19]

LGTM

[bruthig, 2016-04-29 17:56:00]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-04-29 17:56:00]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org
Link to the patchset: https://codereview.chromium.org/1897073002/#ps220001
(title: "Fixed InkDropHoverTests to use proper expectation comparisons.")

[commit-bot: I haz the power, 2016-04-29 17:56:24]

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

[commit-bot: I haz the power, 2016-04-29 18:03:05]

Description was changed from

==========
Add MD Ink Drop to host only when a ripple/hover is active.

In order to paint the material design ink drop behind button icons the icon View
and the button View have to paint to their own Layers. There is no need to incur
the additional expense of these Layers unless the ink drop ripple or hover is
actually active on the View.

BUG=604415

TEST=InkDropHoverTest.*
TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationControllerFactoryTest.VerifyInkDropLayersRemovedAfterDestructionWhenHoverIsActive
==========

to

==========
Add MD Ink Drop to host only when a ripple/hover is active.

In order to paint the material design ink drop behind button icons the icon View
and the button View have to paint to their own Layers. There is no need to incur
the additional expense of these Layers unless the ink drop ripple or hover is
actually active on the View.

BUG=604415

TEST=InkDropHoverTest.*
TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationControllerFactoryTest.VerifyInkDropLayersRemovedAfterDestructionWhenHoverIsActive
==========

[commit-bot: I haz the power, 2016-04-29 18:03:07]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-04-30 17:27:20]

Patchset 12 (id:??) landed as
https://crrev.com/741be72a4d24789a876d87d0d94e9e2e1cdcd674
Cr-Commit-Position: refs/heads/master@{#390696}

[commit-bot: I haz the power, 2016-04-30 18:01:26]

Description was changed from

==========
Add MD Ink Drop to host only when a ripple/hover is active.

In order to paint the material design ink drop behind button icons the icon View
and the button View have to paint to their own Layers. There is no need to incur
the additional expense of these Layers unless the ink drop ripple or hover is
actually active on the View.

BUG=604415

TEST=InkDropHoverTest.*
TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationControllerFactoryTest.VerifyInkDropLayersRemovedAfterDestructionWhenHoverIsActive
==========

to

==========
Add MD Ink Drop to host only when a ripple/hover is active.

In order to paint the material design ink drop behind button icons the icon View
and the button View have to paint to their own Layers. There is no need to incur
the additional expense of these Layers unless the ink drop ripple or hover is
actually active on the View.

BUG=604415

TEST=InkDropHoverTest.*
TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationControllerFactoryTest.VerifyInkDropLayersRemovedAfterDestructionWhenHoverIsActive

Committed: https://crrev.com/741be72a4d24789a876d87d0d94e9e2e1cdcd674
Cr-Commit-Position: refs/heads/master@{#390696}
==========