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

ui/views/animation/ink_drop_animation_controller_impl.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/animation/ink_drop_animation_controller_impl.h"
 
 #include "base/auto_reset.h"
 #include "base/timer/timer.h"
 #include "ui/compositor/layer.h"
 #include "ui/views/animation/ink_drop_host.h"
@@ skipping 36 matching lines @@
       return false;
   }
 }
 
 }  // namespace
 
 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 {
   if (!ink_drop_animation_)
     return InkDropState::HIDDEN;
   return ink_drop_animation_->target_ink_drop_state();
 }
 
 bool InkDropAnimationControllerImpl::IsVisible() const {
   return ink_drop_animation_ && ink_drop_animation_->IsVisible();
@@ skipping 40 matching lines @@
        ShouldAnimateToHidden(ink_drop_animation_->target_ink_drop_state()))) {
     DestroyInkDropAnimation();
   }
 }
 
 void InkDropAnimationControllerImpl::CreateInkDropAnimation() {
   DestroyInkDropAnimation();
   ink_drop_animation_ = ink_drop_host_->CreateInkDropAnimation();
   ink_drop_animation_->set_observer(this);
   root_layer_->Add(ink_drop_animation_->GetRootLayer());
+  AddRootLayerToHost();
 }
 
 void InkDropAnimationControllerImpl::DestroyInkDropAnimation() {
   if (!ink_drop_animation_)
     return;
   root_layer_->Remove(ink_drop_animation_->GetRootLayer());
   ink_drop_animation_.reset();
+  RemoveRootLayerFromHost();
 }
 
 void InkDropAnimationControllerImpl::CreateInkDropHover() {
   DestroyInkDropHover();
 
   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() {
   if (!hover_)
     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 {
   return hover_ && hover_->IsFadingInOrVisible();
 }
 
 void InkDropAnimationControllerImpl::AnimationStarted(
     InkDropState ink_drop_state) {}
 
 void InkDropAnimationControllerImpl::AnimationEnded(
     InkDropState ink_drop_state,
     InkDropAnimationEndedReason reason) {
   if (reason != InkDropAnimationEndedReason::SUCCESS)
     return;
   if (ShouldAnimateToHidden(ink_drop_state)) {
     ink_drop_animation_->AnimateToState(views::InkDropState::HIDDEN);
   } else if (ink_drop_state == views::InkDropState::HIDDEN) {
     if (is_hovered_)
       StartHoverAfterAnimationTimer();
     // TODO(bruthig): Investigate whether creating and destroying
     // InkDropAnimations is expensive and consider creating an
     // InkDropAnimationPool. See www.crbug.com/522175.
     DestroyInkDropAnimation();
   }
 }
 

[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,
     bool explode) {
   StopHoverAfterAnimationTimer();
 
   if (IsHoverFadingInOrVisible() == is_hovered)
     return;
 
   if (is_hovered) {
@@ skipping 24 matching lines @@
 }
 
 void InkDropAnimationControllerImpl::HoverAfterAnimationTimerFired() {
   SetHoveredInternal(true, base::TimeDelta::FromMilliseconds(
                                kHoverFadeInAfterAnimationDurationInMs),
                      true);
   hover_after_animation_timer_.reset();
 }
 
 }  // namespace views