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

ui/views/animation/ink_drop_animation_controller_impl.cc

Index: ui/views/animation/ink_drop_animation_controller_impl.cc
diff --git a/ui/views/animation/ink_drop_animation_controller_impl.cc b/ui/views/animation/ink_drop_animation_controller_impl.cc
index 0cca4cd8d72e6615b2b745fd5b57e8c90bd40b28..0e99dd80df0106539bbc1a16d98eb25a5bcd158c 100644
--- a/ui/views/animation/ink_drop_animation_controller_impl.cc
+++ b/ui/views/animation/ink_drop_animation_controller_impl.cc
@@ -54,17 +54,17 @@ InkDropAnimationControllerImpl::InkDropAnimationControllerImpl(
     InkDropHost* ink_drop_host)
     : ink_drop_host_(ink_drop_host),
       root_layer_(new ui::Layer(ui::LAYER_NOT_DRAWN)),
+      root_layer_added_to_host_(false),
       is_hovered_(false),
       hover_after_animation_timer_(nullptr) {
   root_layer_->set_name("InkDropAnimationControllerImpl:RootLayer");
-  ink_drop_host_->AddInkDropLayer(root_layer_.get());
 }
 
 InkDropAnimationControllerImpl::~InkDropAnimationControllerImpl() {
   // Explicitly destroy the InkDropAnimation so that this still exists if
   // views::InkDropAnimationObserver methods are called on this.
   DestroyInkDropAnimation();

[varkha, 2016/04/28 20:35:00]

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?

[bruthig, 2016/04/29 11:02:34]

Good catch!  Fixed and test added.

-  ink_drop_host_->RemoveInkDropLayer(root_layer_.get());
+  RemoveRootLayerFromHost();
 }
 
 InkDropState InkDropAnimationControllerImpl::GetTargetInkDropState() const {
@@ -125,6 +125,7 @@ void InkDropAnimationControllerImpl::CreateInkDropAnimation() {
   ink_drop_animation_ = ink_drop_host_->CreateInkDropAnimation();
   ink_drop_animation_->set_observer(this);
   root_layer_->Add(ink_drop_animation_->GetRootLayer());
+  AddRootLayerToHost();
 }
 
 void InkDropAnimationControllerImpl::DestroyInkDropAnimation() {
@@ -132,6 +133,7 @@ void InkDropAnimationControllerImpl::DestroyInkDropAnimation() {
     return;
   root_layer_->Remove(ink_drop_animation_->GetRootLayer());
   ink_drop_animation_.reset();
+  RemoveRootLayerFromHost();
 }
 
 void InkDropAnimationControllerImpl::CreateInkDropHover() {
@@ -140,7 +142,9 @@ void InkDropAnimationControllerImpl::CreateInkDropHover() {
   hover_ = ink_drop_host_->CreateInkDropHover();
   if (!hover_)
     return;
+  hover_->set_observer(this);

[varkha, 2016/04/28 20:35:00]

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.

[bruthig, 2016/04/29 11:02:34]

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

   root_layer_->Add(hover_->layer());
+  AddRootLayerToHost();
 }
 
 void InkDropAnimationControllerImpl::DestroyInkDropHover() {
@@ -148,6 +152,21 @@ void InkDropAnimationControllerImpl::DestroyInkDropHover() {
     return;
   root_layer_->Remove(hover_->layer());
   hover_.reset();
+  RemoveRootLayerFromHost();
+}
+
+void InkDropAnimationControllerImpl::AddRootLayerToHost() {
+  if (!root_layer_added_to_host_ && (hover_ || ink_drop_animation_)) {

[varkha, 2016/04/28 20:35:00]

Shouldn't it be possible to DCHECK(hover_ || ink_drop_animation_) instead of
using it in condition?

[bruthig, 2016/04/29 11:02:34]

Done.

+    root_layer_added_to_host_ = true;
+    ink_drop_host_->AddInkDropLayer(root_layer_.get());
+  }
+}
+
+void InkDropAnimationControllerImpl::RemoveRootLayerFromHost() {
+  if (root_layer_added_to_host_ && !hover_ && !ink_drop_animation_) {
+    root_layer_added_to_host_ = false;
+    ink_drop_host_->RemoveInkDropLayer(root_layer_.get());
+  }
 }
 
 bool InkDropAnimationControllerImpl::IsHoverFadingInOrVisible() const {
@@ -174,6 +193,18 @@ void InkDropAnimationControllerImpl::AnimationEnded(
   }
 }
 

[varkha, 2016/04/28 20:35:00]

Maybe a // views::InkDropHoverObserver: banner comment here (and the other
interface class comment before the 2 similar methods above) just to clarify
those handlers.

[bruthig, 2016/04/29 11:02:34]

Done.

+void InkDropAnimationControllerImpl::AnimationStarted(
+    InkDropHover::AnimationType animation_type) {}
+
+void InkDropAnimationControllerImpl::AnimationEnded(
+    InkDropHover::AnimationType animation_type,
+    InkDropAnimationEndedReason reason) {
+  if (animation_type == InkDropHover::FADE_OUT &&
+      reason == InkDropAnimationEndedReason::SUCCESS) {
+    DestroyInkDropHover();
+  }
+}
+
 void InkDropAnimationControllerImpl::SetHoveredInternal(
     bool is_hovered,
     base::TimeDelta animation_duration,