cc : Make LayerImpl destruction independent of tree hierarchy

cc/layers/layer_impl.cc

 // Copyright 2012 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 "cc/layers/layer_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 89 matching lines @@
   layer_tree_impl_->RemoveLayerShouldPushProperties(this);
 
   layer_tree_impl_->RemoveFromElementMap(this);
 
   TRACE_EVENT_OBJECT_DELETED_WITH_ID(
       TRACE_DISABLED_BY_DEFAULT("cc.debug"), "cc::LayerImpl", this);
 
   if (mask_layer_)
     layer_tree_impl_->RemoveLayer(mask_layer_id_);
   if (replica_layer_)
     layer_tree_impl_->RemoveLayer(replica_layer_id_);

[ajuma, 2016/06/07 23:25:36]

Since masks and replicas are part of layers_ in LayerTreeImpl, will they already
have been destroyed at the same time that children got destroyed? That is, if
you were to DCHECK that the return value of the calls to RemoveLayer above is
always null, would that succeed?

[jaydasika, 2016/06/08 00:17:37]

I din't understand that fully. masks and replicas are in layers_ but not in
children_. But as you said, I think we need not explicitly delete them as they
are part of layers_. But I don't think they are sure to be deleted before the
layer itself. If might be true now because of the order in which we add layers
to layers_ but may not be true when we push layer list.

[jaydasika, 2016/06/08 00:49:32]

Scratch my previous comment, I understand what you mean.

[ajuma, 2016/06/08 13:48:02]

That's a good point about destruction order -- even if masks and replicas are in
layers_, there's no guarantee about the order in which things inside layers_
will be destroyed when layers_ gets cleared or destroyed. That is, we don't need
to delete the masks and replicas explicitly here, but we can't be sure they'll
be deleted before the layer itself. So the DCHECKs should be removed then.

[jaydasika, 2016/06/08 16:29:30]

I agree that there is no guarantee about destruction order, but I think it is
fine to have these DCHECKs because the destruction of layers can start only
after all the layers that are to be destroyed are removed from layers_. This is
because of the way LTI::DetachLayers is written. The function itself doesn't
lead to destructor being called. It removes all layers from layers_ and
transfers their ownership to a new vector that it returns. Layer destruction can
start after this new vector goes out of scope (so we can be sure all layers are
removed from layers_ at that point).

[ajuma, 2016/06/08 16:40:45]

Ah, got it, that makes sense.

-  ClearChildList();
+  children_.clear();
 }
 
 void LayerImpl::AddChild(std::unique_ptr<LayerImpl> child) {
   child->SetParent(this);
   DCHECK_EQ(layer_tree_impl(), child->layer_tree_impl());
   children_.push_back(child.get());
   layer_tree_impl_->AddLayer(std::move(child));
 }
 
 std::unique_ptr<LayerImpl> LayerImpl::RemoveChildForTesting(LayerImpl* child) {
   auto it = std::find(children_.begin(), children_.end(), child);
   if (it != children_.end())
     children_.erase(it);
   layer_tree_impl()->property_trees()->RemoveIdFromIdToIndexMaps(child->id());
   return layer_tree_impl_->RemoveLayer(child->id());
 }
 
 void LayerImpl::SetParent(LayerImpl* parent) {
   parent_ = parent;
 }
 
-void LayerImpl::ClearChildList() {
-  if (children_.empty())
-    return;
-  for (auto* child : children_)
-    layer_tree_impl_->RemoveLayer(child->id());
-  children_.clear();
-}
-
 void LayerImpl::ClearLinksToOtherLayers() {
   children_.clear();
   mask_layer_ = nullptr;
   replica_layer_ = nullptr;
 }
 
 void LayerImpl::SetHasWillChangeTransformHint(bool has_will_change) {
   if (has_will_change_transform_hint_ == has_will_change)
     return;
   has_will_change_transform_hint_ = has_will_change;
@@ skipping 1210 matching lines @@
            .layer_transforms_should_scale_layer_contents) {
     return default_scale;
   }
 
   gfx::Vector2dF transform_scales = MathUtil::ComputeTransform2dScaleComponents(
       ScreenSpaceTransform(), default_scale);
   return std::max(transform_scales.x(), transform_scales.y());
 }
 
 }  // namespace cc