CC Animations: Make LayerAnimationController creation optional

ui/compositor/layer.cc

Index: ui/compositor/layer.cc
diff --git a/ui/compositor/layer.cc b/ui/compositor/layer.cc
index 2b6f9219c574b40c1bdd6d6e0742c0d47d0a1478..90a83c2271763d9e3bb8aff8b190434f1034810c 100644
--- a/ui/compositor/layer.cc
+++ b/ui/compositor/layer.cc
@@ -26,6 +26,7 @@
 #include "cc/output/filter_operation.h"
 #include "cc/output/filter_operations.h"
 #include "cc/resources/transferable_resource.h"
+#include "cc/trees/layer_tree_host.h"
 #include "ui/compositor/compositor_switches.h"
 #include "ui/compositor/dip_util.h"
 #include "ui/compositor/layer_animator.h"
@@ -124,7 +125,9 @@ Layer::~Layer() {
     layer_mask_back_link_->SetMaskLayer(NULL);
   for (size_t i = 0; i < children_.size(); ++i)
     children_[i]->parent_ = NULL;
-  cc_layer_->RemoveLayerAnimationEventObserver(this);
+
+  DetachAnimationObservers();
+
   cc_layer_->RemoveFromParent();
 }
 
@@ -142,21 +145,33 @@ float Layer::opacity() const {
 }
 
 void Layer::SetCompositor(Compositor* compositor) {
-  // This function must only be called to set the compositor on the root layer,
-  // or to reset it.
-  DCHECK(!compositor || !compositor_);
-  DCHECK(!compositor || compositor->root_layer() == this);
+  // This function must only be called to set the compositor on the root ui
+  // layer.
+  DCHECK(compositor);
+  DCHECK(!compositor_);
+  DCHECK(compositor->root_layer() == this);
+  DCHECK(!parent_);
+
+  compositor_ = compositor;
+  OnDeviceScaleFactorChanged(compositor->device_scale_factor());
+  AddAnimatorsInTreeToCollection(compositor_->layer_animator_collection());
+}
+
+void Layer::SetCompositorRootCCLayer(scoped_refptr<cc::Layer> root_layer) {
+  // This function must only be called on the root ui layer.

[piman, 2015/04/29 00:46:11]

Can you add a DCHECK that it's the root (it doesn't have a parent)?

Why does it need to be separated from SetCompositor?

[loyso (OOO), 2015/04/29 03:49:33]

Ack'ed.
I thought on merging them into just one call. But
Compositor::SetRootLayer code allows to call SetCompositorRootCCLayer w/o
setting the compositor (calling SetCompositor). I'm not sure, whether we use
this configuration in runtime on all our platforms. That's a change in behavior.
It would be better to have such a transform as a separate small CL to isolate
risks. 

+  root_layer->AddChild(cc_layer_);
+  AttachAnimationObservers();
+  SendPendingThreadedAnimations();
+}
+
+void Layer::ResetCompositor() {
   DCHECK(!parent_);
   if (compositor_) {
+    DetachAnimationObservers();
     RemoveAnimatorsInTreeFromCollection(
         compositor_->layer_animator_collection());

[piman, 2015/04/29 00:46:11]

We're doing 2 recursions (one in each of these functions). Is it really needed?
It seems inefficient.

Maybe a better way is to provide a single template RecurseTree function that
takes a functor, to which you can pass a lambda:

template<typename F> void RecurseTree(const F& f) {
  f(this);
  for (child : children_)
    child->RecurseTree(f);
}

Then here you can do something like:
LayerAnimatorCollection* collection = compositor_->layer_animator_collection();
RecurseTree([collection](Layer* layer) {
  layer->DetachAnimationObserver();
  layer->RemoveAnimators(collection);
});

And same in other places below where we're doing multiple recursions.

I think most recursive functions should be removed or changed to use
RecurseTree.

   }
-  compositor_ = compositor;
-  if (compositor) {
-    OnDeviceScaleFactorChanged(compositor->device_scale_factor());
-    SendPendingThreadedAnimations();
-    AddAnimatorsInTreeToCollection(compositor_->layer_animator_collection());
-  }
+  compositor_ = nullptr;
 }
 
 void Layer::Add(Layer* child) {
@@ -167,8 +182,11 @@ void Layer::Add(Layer* child) {
   children_.push_back(child);
   cc_layer_->AddChild(child->cc_layer_);
   child->OnDeviceScaleFactorChanged(device_scale_factor_);
-  if (GetCompositor())
+  Compositor* compositor = GetCompositor();
+  if (compositor) {
+    child->AttachAnimationObservers();
     child->SendPendingThreadedAnimations();

[piman, 2015/04/29 00:46:11]

2 recursions here too. Is that needed?

+  }
   LayerAnimatorCollection* collection = GetLayerAnimatorCollection();
   if (collection)
     child->AddAnimatorsInTreeToCollection(collection);

[piman, 2015/04/29 00:46:11]

And a third one here.

@@ -184,6 +202,8 @@ void Layer::Remove(Layer* child) {
   if (collection)
     child->RemoveAnimatorsInTreeFromCollection(collection);
 
+  child->DetachAnimationObservers();

[piman, 2015/04/29 00:46:11]

2 recursions

+
   std::vector<Layer*>::iterator i =
       std::find(children_.begin(), children_.end(), child);
   DCHECK(i != children_.end());
@@ -337,8 +357,7 @@ void Layer::SetMaskLayer(Layer* layer_mask) {
   if (layer_mask_)
     layer_mask_->layer_mask_back_link_ = NULL;
   layer_mask_ = layer_mask;
-  cc_layer_->SetMaskLayer(
-      layer_mask ? layer_mask->cc_layer() : NULL);
+  cc_layer_->SetMaskLayer(layer_mask ? layer_mask->cc_layer_ : NULL);
   // We need to reference the linked object so that it can properly break the
   // link to us when it gets deleted.
   if (layer_mask) {
@@ -490,7 +509,9 @@ void Layer::SwitchToLayer(scoped_refptr<cc::Layer> new_layer) {
     cc_layer_->parent()->ReplaceChild(cc_layer_, new_layer);
   }
   cc_layer_->SetLayerClient(NULL);
-  cc_layer_->RemoveLayerAnimationEventObserver(this);
+
+  DetachAnimationObservers();

[piman, 2015/04/29 00:46:11]

Is this right? Is it done at the right time?
We removed all children in the cc tree on l.507, and we'll reattach them on
l.531.
Is it right to recurse over the children when the tree is in a transient state?
Seems scary.
Do you need to recurse at all? The children will be the same before/after, only
this one layer is changing.

[loyso (OOO), 2015/04/29 08:15:02]

Acknowledged.

+
   new_layer->SetOpacity(cc_layer_->opacity());
   new_layer->SetTransform(cc_layer_->transform());
   new_layer->SetPosition(cc_layer_->position());
@@ -503,7 +524,8 @@ void Layer::SwitchToLayer(scoped_refptr<cc::Layer> new_layer) {
   delegated_renderer_layer_ = NULL;
   surface_layer_ = NULL;
 
-  cc_layer_->AddLayerAnimationEventObserver(this);
+  AttachAnimationObservers();
+
   for (size_t i = 0; i < children_.size(); ++i) {
     DCHECK(children_[i]->cc_layer_);
     cc_layer_->AddChild(children_[i]->cc_layer_);
@@ -1034,11 +1056,26 @@ void Layer::CreateCcLayer() {
   cc_layer_->SetTransformOrigin(gfx::Point3F());
   cc_layer_->SetContentsOpaque(true);
   cc_layer_->SetIsDrawable(type_ != LAYER_NOT_DRAWN);
-  cc_layer_->AddLayerAnimationEventObserver(this);
   cc_layer_->SetLayerClient(this);
   RecomputePosition();
 }
 
+void Layer::AttachAnimationObservers() {
+  if (cc_layer_->layer_animation_controller())
+    cc_layer_->AddLayerAnimationEventObserver(this);
+
+  for (size_t i = 0; i < children_.size(); ++i)
+    children_[i]->AttachAnimationObservers();
+}
+
+void Layer::DetachAnimationObservers() {
+  if (cc_layer_->layer_animation_controller())
+    cc_layer_->RemoveLayerAnimationEventObserver(this);
+
+  for (size_t i = 0; i < children_.size(); ++i)
+    children_[i]->DetachAnimationObservers();
+}
+
 gfx::Transform Layer::transform() const {
   return cc_layer_->transform();
 }