Skip to content

Commit

Permalink
[views] Moves ScopedChildrenLock into internal namespace
Browse files Browse the repository at this point in the history 
Having that class in anonymous namespace was causing problems compiling
with DCHECKs on.
Additionally fixing some bugs around reentrant iterations and actually
enabling the checks by creatig ScopedChildrenLock objects.

Thanks to dcastagna@ who posted this CL originally:
https://codereview.chromium.org/2606513002 .

BUG=NONE

Review-Url: https://codereview.chromium.org/2608303002
Cr-Commit-Position: refs/heads/master@{#441158}
  • Loading branch information
varkha authored and Commit bot committed Jan 3, 2017
1 parent 6a8aa64 commit 624f6a0
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 36 deletions.
64 changes: 32 additions & 32 deletions ui/views/view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,19 +90,19 @@ const View* GetHierarchyRoot(const View* view) {
return root;
}

} // namespace

namespace internal {

#if DCHECK_IS_ON()
class ScopedChildrenLock {
public:
explicit ScopedChildrenLock(const View* view) : view_(view) {
DCHECK(!view->iterating_);
view->iterating_ = true;
}

~ScopedChildrenLock() { view_->iterating_ = false; }

private:
const View* view_;
DISALLOW_COPY_AND_ASSIGN(ScopedChildrenLock);
explicit ScopedChildrenLock(const View* view)
: reset_(&view->iterating_, true) {}
~ScopedChildrenLock() {}
private:
base::AutoReset<bool> reset_;
DISALLOW_COPY_AND_ASSIGN(ScopedChildrenLock);
};
#else
class ScopedChildrenLock {
Expand All @@ -112,7 +112,7 @@ const View* GetHierarchyRoot(const View* view) {
};
#endif

} // namespace
} // namespace internal

// static
const char View::kViewClassName[] = "View";
Expand Down Expand Up @@ -156,7 +156,7 @@ View::~View() {
ViewStorage::GetInstance()->ViewRemoved(this);

{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_) {
child->parent_ = NULL;
if (!child->owned_by_client_)
Expand Down Expand Up @@ -589,7 +589,7 @@ void View::Layout() {
// weren't changed by the layout manager. If there is no layout manager, we
// just propagate the Layout() call down the hierarchy, so whoever receives
// the call can take appropriate action.
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_) {
if (child->needs_layout_ || !layout_manager_.get()) {
TRACE_EVENT1("views", "View::Layout", "class", child->GetClassName());
Expand Down Expand Up @@ -656,7 +656,7 @@ const View* View::GetViewByID(int id) const {
if (id == id_)
return const_cast<View*>(this);

ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_) {
const View* view = child->GetViewByID(id);
if (view)
Expand Down Expand Up @@ -687,7 +687,7 @@ void View::GetViewsInGroup(int group, Views* views) {
if (group_ == group)
views->push_back(this);

ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->GetViewsInGroup(group, views);
}
Expand Down Expand Up @@ -1473,7 +1473,7 @@ void View::NativeViewHierarchyChanged() {

void View::PaintChildren(const ui::PaintContext& context) {
TRACE_EVENT1("views", "View::PaintChildren", "class", GetClassName());
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_) {
if (!child->layer())
child->Paint(context);
Expand Down Expand Up @@ -1543,7 +1543,7 @@ void View::MoveLayerToParent(ui::Layer* parent_layer,
SetLayerBounds(gfx::Rect(local_point.x(), local_point.y(),
width(), height()));
} else {
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->MoveLayerToParent(parent_layer, local_point);
}
Expand All @@ -1561,7 +1561,7 @@ void View::UpdateChildLayerVisibility(bool ancestor_visible) {
if (layer()) {
layer()->SetVisible(ancestor_visible && visible_);
} else {
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->UpdateChildLayerVisibility(ancestor_visible && visible_);
}
Expand All @@ -1571,7 +1571,7 @@ void View::UpdateChildLayerBounds(const gfx::Vector2d& offset) {
if (layer()) {
SetLayerBounds(GetLocalBounds() + offset);
} else {
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_) {
child->UpdateChildLayerBounds(
offset + gfx::Vector2d(child->GetMirroredX(), child->y()));
Expand Down Expand Up @@ -1628,7 +1628,7 @@ void View::ReorderChildLayers(ui::Layer* parent_layer) {
// Iterate backwards through the children so that a child with a layer
// which is further to the back is stacked above one which is further to
// the front.
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : base::Reversed(children_)) {
child->ReorderChildLayers(parent_layer);
}
Expand Down Expand Up @@ -1919,7 +1919,7 @@ void View::DoRemoveChildView(View* view,

void View::PropagateRemoveNotifications(View* old_parent, View* new_parent) {
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->PropagateRemoveNotifications(old_parent, new_parent);
}
Expand All @@ -1932,7 +1932,7 @@ void View::PropagateRemoveNotifications(View* old_parent, View* new_parent) {
void View::PropagateAddNotifications(
const ViewHierarchyChangedDetails& details) {
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->PropagateAddNotifications(details);
}
Expand All @@ -1941,7 +1941,7 @@ void View::PropagateAddNotifications(

void View::PropagateNativeViewHierarchyChanged() {
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->PropagateNativeViewHierarchyChanged();
}
Expand Down Expand Up @@ -1969,7 +1969,7 @@ void View::ViewHierarchyChangedImpl(

void View::PropagateNativeThemeChanged(const ui::NativeTheme* theme) {
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->PropagateNativeThemeChanged(theme);
}
Expand All @@ -1980,7 +1980,7 @@ void View::PropagateNativeThemeChanged(const ui::NativeTheme* theme) {

void View::PropagateVisibilityNotifications(View* start, bool is_visible) {
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->PropagateVisibilityNotifications(start, is_visible);
}
Expand Down Expand Up @@ -2165,7 +2165,7 @@ void View::CreateLayer() {
// A new layer is being created for the view. So all the layers of the
// sub-tree can inherit the visibility of the corresponding view.
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->UpdateChildLayerVisibility(true);
}
Expand Down Expand Up @@ -2206,7 +2206,7 @@ bool View::UpdateParentLayers() {
return false;
}
bool result = false;
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_) {
if (child->UpdateParentLayers())
result = true;
Expand All @@ -2223,7 +2223,7 @@ void View::OrphanLayers() {
// necessary to orphan the child layers.
return;
}
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_)
child->OrphanLayers();
}
Expand Down Expand Up @@ -2409,7 +2409,7 @@ void View::InitFocusSiblings(View* v, int index) {
// focusable element to link to.
View* last_focusable_view = NULL;
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : children_) {
if (!child->next_focusable_view_) {
last_focusable_view = child;
Expand Down Expand Up @@ -2458,7 +2458,7 @@ void View::AdvanceFocusIfNecessary() {

void View::PropagateThemeChanged() {
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : base::Reversed(children_))
child->PropagateThemeChanged();
}
Expand All @@ -2467,7 +2467,7 @@ void View::PropagateThemeChanged() {

void View::PropagateLocaleChanged() {
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : base::Reversed(children_))
child->PropagateLocaleChanged();
}
Expand All @@ -2476,7 +2476,7 @@ void View::PropagateLocaleChanged() {

void View::PropagateDeviceScaleFactorChanged(float device_scale_factor) {
{
ScopedChildrenLock(this);
internal::ScopedChildrenLock lock(this);
for (auto* child : base::Reversed(children_))
child->PropagateDeviceScaleFactorChanged(device_scale_factor);
}
Expand Down
5 changes: 1 addition & 4 deletions ui/views/view.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,6 @@ namespace internal {
class PreEventDispatchHandler;
class PostEventDispatchHandler;
class RootView;
}

namespace {
class ScopedChildrenLock;
}

Expand Down Expand Up @@ -1233,8 +1230,8 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
friend class internal::PreEventDispatchHandler;
friend class internal::PostEventDispatchHandler;
friend class internal::RootView;
friend class internal::ScopedChildrenLock;
friend class FocusManager;
friend class ScopedChildrenLock;
friend class ViewLayerTest;
friend class Widget;

Expand Down

0 comments on commit 624f6a0

Please sign in to comment.