Skip to content

Commit

Permalink
Fix up cc::Layer scroll parent management
Browse files Browse the repository at this point in the history 
The previous code was trying to perform scroll parent pointer fixup during
LayerImpl destruction, which is problematic since ~LayerImpl() always runs
during tree synchronization and there is nothing to ensure that the scroll
parent's portion of the tree has be fully synchronized when a particular
scroll child is being destroyed.

Instead, this performs pointer fixup on the main thread and then just pushes
the values through to the LayerImpl tree without performing additional fixups.
So long as the main-thread (cc::Layer) tree stays structurally valid there's
no need for additional changes at layer destruction or tree manipulation time
on any LayerImpl trees.

BUG=347284

Review URL: https://codereview.chromium.org/182363002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@254244 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
jamesr@chromium.org committed Feb 28, 2014
1 parent b9473ee commit d097e24
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 78 deletions.
20 changes: 17 additions & 3 deletions cc/layers/layer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -957,34 +957,48 @@ void Layer::PushPropertiesTo(LayerImpl* layer) {
layer->set_user_scrollable_vertical(user_scrollable_vertical_);

LayerImpl* scroll_parent = NULL;
if (scroll_parent_)
if (scroll_parent_) {
scroll_parent = layer->layer_tree_impl()->LayerById(scroll_parent_->id());
DCHECK(scroll_parent);
}

layer->SetScrollParent(scroll_parent);
if (scroll_children_) {
std::set<LayerImpl*>* scroll_children = new std::set<LayerImpl*>;
for (std::set<Layer*>::iterator it = scroll_children_->begin();
it != scroll_children_->end(); ++it)
scroll_children->insert(layer->layer_tree_impl()->LayerById((*it)->id()));
it != scroll_children_->end();
++it) {
DCHECK_EQ((*it)->scroll_parent(), this);
LayerImpl* scroll_child =
layer->layer_tree_impl()->LayerById((*it)->id());
DCHECK(scroll_child);
scroll_children->insert(scroll_child);
}
layer->SetScrollChildren(scroll_children);
} else {
layer->SetScrollChildren(NULL);
}

LayerImpl* clip_parent = NULL;
if (clip_parent_) {
clip_parent =
layer->layer_tree_impl()->LayerById(clip_parent_->id());
DCHECK(clip_parent);
}

layer->SetClipParent(clip_parent);
if (clip_children_) {
std::set<LayerImpl*>* clip_children = new std::set<LayerImpl*>;
for (std::set<Layer*>::iterator it = clip_children_->begin();
it != clip_children_->end(); ++it) {
DCHECK_EQ((*it)->clip_parent(), this);
LayerImpl* clip_child = layer->layer_tree_impl()->LayerById((*it)->id());
DCHECK(clip_child);
clip_children->insert(clip_child);
}
layer->SetClipChildren(clip_children);
} else {
layer->SetClipChildren(NULL);
}

// Adjust the scroll delta to be just the scrolls that have happened since
Expand Down
60 changes: 18 additions & 42 deletions cc/layers/layer_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,24 +93,6 @@ LayerImpl::~LayerImpl() {
layer_tree_impl()->RemoveLayerWithCopyOutputRequest(this);
layer_tree_impl_->UnregisterLayer(this);

if (scroll_children_) {
for (std::set<LayerImpl*>::iterator it = scroll_children_->begin();
it != scroll_children_->end(); ++it)
(*it)->scroll_parent_ = NULL;
}

if (scroll_parent_)
scroll_parent_->RemoveScrollChild(this);

if (clip_children_) {
for (std::set<LayerImpl*>::iterator it = clip_children_->begin();
it != clip_children_->end(); ++it)
(*it)->clip_parent_ = NULL;
}

if (clip_parent_)
clip_parent_->RemoveClipChild(this);

TRACE_EVENT_OBJECT_DELETED_WITH_ID(
TRACE_DISABLED_BY_DEFAULT("cc.debug"), "cc::LayerImpl", this);
}
Expand Down Expand Up @@ -173,8 +155,8 @@ void LayerImpl::SetScrollParent(LayerImpl* parent) {
// Having both a scroll parent and a scroll offset delegate is unsupported.
DCHECK(!scroll_offset_delegate_);

if (scroll_parent_)
scroll_parent_->RemoveScrollChild(this);
if (parent)
DCHECK_EQ(layer_tree_impl()->LayerById(parent->id()), parent);

scroll_parent_ = parent;
SetNeedsPushProperties();
Expand All @@ -193,21 +175,10 @@ void LayerImpl::SetScrollChildren(std::set<LayerImpl*>* children) {
SetNeedsPushProperties();
}

void LayerImpl::RemoveScrollChild(LayerImpl* child) {
DCHECK(scroll_children_);
scroll_children_->erase(child);
if (scroll_children_->empty())
scroll_children_.reset();
SetNeedsPushProperties();
}

void LayerImpl::SetClipParent(LayerImpl* ancestor) {
if (clip_parent_ == ancestor)
return;

if (clip_parent_)
clip_parent_->RemoveClipChild(this);

clip_parent_ = ancestor;
SetNeedsPushProperties();
}
Expand All @@ -219,14 +190,6 @@ void LayerImpl::SetClipChildren(std::set<LayerImpl*>* children) {
SetNeedsPushProperties();
}

void LayerImpl::RemoveClipChild(LayerImpl* child) {
DCHECK(clip_children_);
clip_children_->erase(child);
if (clip_children_->empty())
clip_children_.reset();
SetNeedsPushProperties();
}

void LayerImpl::PassCopyRequests(ScopedPtrVector<CopyOutputRequest>* requests) {
if (requests->empty())
return;
Expand Down Expand Up @@ -568,22 +531,33 @@ void LayerImpl::PushPropertiesTo(LayerImpl* layer) {
layer->SetSentScrollDelta(gfx::Vector2d());

LayerImpl* scroll_parent = NULL;
if (scroll_parent_)
if (scroll_parent_) {
scroll_parent = layer->layer_tree_impl()->LayerById(scroll_parent_->id());
DCHECK(scroll_parent);
}

layer->SetScrollParent(scroll_parent);
if (scroll_children_) {
std::set<LayerImpl*>* scroll_children = new std::set<LayerImpl*>;
for (std::set<LayerImpl*>::iterator it = scroll_children_->begin();
it != scroll_children_->end(); ++it)
scroll_children->insert(layer->layer_tree_impl()->LayerById((*it)->id()));
it != scroll_children_->end();
++it) {
DCHECK_EQ((*it)->scroll_parent(), this);
LayerImpl* scroll_child =
layer->layer_tree_impl()->LayerById((*it)->id());
DCHECK(scroll_child);
scroll_children->insert(scroll_child);
}
layer->SetScrollChildren(scroll_children);
} else {
layer->SetScrollChildren(NULL);
}

LayerImpl* clip_parent = NULL;
if (clip_parent_) {
clip_parent = layer->layer_tree_impl()->LayerById(
clip_parent_->id());
DCHECK(clip_parent);
}

layer->SetClipParent(clip_parent);
Expand All @@ -593,6 +567,8 @@ void LayerImpl::PushPropertiesTo(LayerImpl* layer) {
it != clip_children_->end(); ++it)
clip_children->insert(layer->layer_tree_impl()->LayerById((*it)->id()));
layer->SetClipChildren(clip_children);
} else {
layer->SetClipChildren(NULL);
}

layer->PassCopyRequests(&copy_requests_);
Expand Down
2 changes: 0 additions & 2 deletions cc/layers/layer_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
const LayerImpl* scroll_parent() const { return scroll_parent_; }

void SetScrollChildren(std::set<LayerImpl*>* children);
void RemoveScrollChild(LayerImpl* child);

std::set<LayerImpl*>* scroll_children() { return scroll_children_.get(); }
const std::set<LayerImpl*>* scroll_children() const {
Expand All @@ -128,7 +127,6 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
}

void SetClipChildren(std::set<LayerImpl*>* children);
void RemoveClipChild(LayerImpl* child);

std::set<LayerImpl*>* clip_children() { return clip_children_.get(); }
const std::set<LayerImpl*>* clip_children() const {
Expand Down
4 changes: 0 additions & 4 deletions cc/layers/layer_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,10 @@ TEST(LayerImplTest, VerifyLayerChangesAreTrackedProperly) {
root->SetScrollParent(scroll_parent.get()));
EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
root->SetScrollChildren(scroll_children));
EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
root->RemoveScrollChild(scroll_child));
EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
root->SetClipParent(clip_parent.get()));
EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
root->SetClipChildren(clip_children));
EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
root->RemoveClipChild(clip_child));

// After setting all these properties already, setting to the exact same
// values again should not cause any change.
Expand Down
55 changes: 55 additions & 0 deletions cc/trees/tree_synchronizer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "cc/trees/tree_synchronizer.h"

#include <set>

#include "base/containers/hash_tables.h"
#include "base/containers/scoped_ptr_hash_map.h"
#include "base/debug/trace_event.h"
Expand Down Expand Up @@ -234,11 +236,64 @@ void TreeSynchronizer::PushPropertiesInternal(
*num_dependents_need_push_properties_for_parent += add_self_to_parent ? 1 : 0;
}

static void CheckScrollAndClipPointersRecursive(Layer* layer,
LayerImpl* layer_impl) {
DCHECK_EQ(!!layer, !!layer_impl);
if (!layer)
return;

DCHECK_EQ(!!layer->scroll_parent(), !!layer_impl->scroll_parent());
if (layer->scroll_parent())
DCHECK_EQ(layer->scroll_parent()->id(), layer_impl->scroll_parent()->id());

DCHECK_EQ(!!layer->clip_parent(), !!layer_impl->clip_parent());
if (layer->clip_parent())
DCHECK_EQ(layer->clip_parent()->id(), layer_impl->clip_parent()->id());

DCHECK_EQ(!!layer->scroll_children(), !!layer_impl->scroll_children());
if (layer->scroll_children()) {
for (std::set<Layer*>::iterator it = layer->scroll_children()->begin();
it != layer->scroll_children()->end();
++it) {
DCHECK_EQ((*it)->scroll_parent(), layer);
}
for (std::set<LayerImpl*>::iterator it =
layer_impl->scroll_children()->begin();
it != layer_impl->scroll_children()->end();
++it) {
DCHECK_EQ((*it)->scroll_parent(), layer_impl);
}
}

DCHECK_EQ(!!layer->clip_children(), !!layer_impl->clip_children());
if (layer->clip_children()) {
for (std::set<Layer*>::iterator it = layer->clip_children()->begin();
it != layer->clip_children()->end();
++it) {
DCHECK_EQ((*it)->clip_parent(), layer);
}
for (std::set<LayerImpl*>::iterator it =
layer_impl->clip_children()->begin();
it != layer_impl->clip_children()->end();
++it) {
DCHECK_EQ((*it)->clip_parent(), layer_impl);
}
}

for (size_t i = 0u; i < layer->children().size(); ++i) {
CheckScrollAndClipPointersRecursive(layer->child_at(i),
layer_impl->child_at(i));
}
}

void TreeSynchronizer::PushProperties(Layer* layer,
LayerImpl* layer_impl) {
size_t num_dependents_need_push_properties = 0;
PushPropertiesInternal(
layer, layer_impl, &num_dependents_need_push_properties);
if (DCHECK_IS_ON()) {
CheckScrollAndClipPointersRecursive(layer, layer_impl);
}
}

void TreeSynchronizer::PushProperties(LayerImpl* layer, LayerImpl* layer_impl) {
Expand Down
50 changes: 23 additions & 27 deletions cc/trees/tree_synchronizer_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
#include <set>
#include <vector>

#include "base/format_macros.h"
#include "base/strings/stringprintf.h"
#include "cc/animation/layer_animation_controller.h"
#include "cc/layers/layer.h"
#include "cc/layers/layer_impl.h"
Expand Down Expand Up @@ -110,12 +112,14 @@ void ExpectTreesAreIdentical(Layer* layer,

ASSERT_EQ(!!layer->mask_layer(), !!layer_impl->mask_layer());
if (layer->mask_layer()) {
SCOPED_TRACE("mask_layer");
ExpectTreesAreIdentical(
layer->mask_layer(), layer_impl->mask_layer(), tree_impl);
}

ASSERT_EQ(!!layer->replica_layer(), !!layer_impl->replica_layer());
if (layer->replica_layer()) {
SCOPED_TRACE("replica_layer");
ExpectTreesAreIdentical(
layer->replica_layer(), layer_impl->replica_layer(), tree_impl);
}
Expand Down Expand Up @@ -176,6 +180,7 @@ void ExpectTreesAreIdentical(Layer* layer,
}

for (size_t i = 0; i < layer_children.size(); ++i) {
SCOPED_TRACE(base::StringPrintf("child layer %" PRIuS, i).c_str());
ExpectTreesAreIdentical(
layer_children[i].get(), layer_impl_children[i], tree_impl);
}
Expand Down Expand Up @@ -605,9 +610,12 @@ TEST_F(TreeSynchronizerTest, SynchronizeScrollParent) {
host_impl->active_tree());
TreeSynchronizer::PushProperties(layer_tree_root.get(),
layer_impl_tree_root.get());
ExpectTreesAreIdentical(layer_tree_root.get(),
layer_impl_tree_root.get(),
host_impl->active_tree());
{
SCOPED_TRACE("case one");
ExpectTreesAreIdentical(layer_tree_root.get(),
layer_impl_tree_root.get(),
host_impl->active_tree());
}

// Remove the first scroll child.
layer_tree_root->children()[1]->RemoveFromParent();
Expand All @@ -617,9 +625,12 @@ TEST_F(TreeSynchronizerTest, SynchronizeScrollParent) {
host_impl->active_tree());
TreeSynchronizer::PushProperties(layer_tree_root.get(),
layer_impl_tree_root.get());
ExpectTreesAreIdentical(layer_tree_root.get(),
layer_impl_tree_root.get(),
host_impl->active_tree());
{
SCOPED_TRACE("case two");
ExpectTreesAreIdentical(layer_tree_root.get(),
layer_impl_tree_root.get(),
host_impl->active_tree());
}

// Add an additional scroll layer.
scoped_refptr<Layer> additional_scroll_child = Layer::Create();
Expand All @@ -631,27 +642,12 @@ TEST_F(TreeSynchronizerTest, SynchronizeScrollParent) {
host_impl->active_tree());
TreeSynchronizer::PushProperties(layer_tree_root.get(),
layer_impl_tree_root.get());
ExpectTreesAreIdentical(layer_tree_root.get(),
layer_impl_tree_root.get(),
host_impl->active_tree());

// Remove the scroll parent.
scroll_parent->RemoveFromParent();
scroll_parent = NULL;
layer_impl_tree_root =
TreeSynchronizer::SynchronizeTrees(layer_tree_root.get(),
layer_impl_tree_root.Pass(),
host_impl->active_tree());
TreeSynchronizer::PushProperties(layer_tree_root.get(),
layer_impl_tree_root.get());
ExpectTreesAreIdentical(layer_tree_root.get(),
layer_impl_tree_root.get(),
host_impl->active_tree());

// The scroll children should have been unhooked.
EXPECT_EQ(2u, layer_tree_root->children().size());
EXPECT_FALSE(layer_tree_root->children()[0]->scroll_parent());
EXPECT_FALSE(layer_tree_root->children()[1]->scroll_parent());
{
SCOPED_TRACE("case three");
ExpectTreesAreIdentical(layer_tree_root.get(),
layer_impl_tree_root.get(),
host_impl->active_tree());
}
}

TEST_F(TreeSynchronizerTest, SynchronizeClipParent) {
Expand Down

0 comments on commit d097e24

Please sign in to comment.