[views] Changes iteration over |children_| to use range-based for loops (reland)

ui/views/view.cc

Index: ui/views/view.cc
diff --git a/ui/views/view.cc b/ui/views/view.cc
index 92223157d4c8671aaa2c55aab4c006d1ee25b265..ea20b8779a57adf288fa6da08d3ad46cb9a22b74 100644
--- a/ui/views/view.cc
+++ b/ui/views/view.cc
@@ -11,6 +11,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/containers/adapters.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ -104,6 +105,9 @@ View::View()
       id_(0),
       group_(-1),
       parent_(NULL),
+#if DCHECK_IS_ON()
+      iterating_(false),
+#endif
       visible_(true),
       enabled_(true),
       notify_enter_exit_on_child_(false),
@@ -129,10 +133,15 @@ View::~View() {
 
   ViewStorage::GetInstance()->ViewRemoved(this);
 
-  for (Views::const_iterator i(children_.begin()); i != children_.end(); ++i) {
-    (*i)->parent_ = NULL;
-    if (!(*i)->owned_by_client_)
-      delete *i;
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_) {
+      child->parent_ = NULL;
+      if (!child->owned_by_client_)
+        delete child;
+    }
   }
 
   // Release ownership of the native accessibility object, but it's
@@ -179,6 +188,9 @@ void View::AddChildViewAt(View* view, int index) {
   InitFocusSiblings(view, index);
 
   view->parent_ = this;
+#if DCHECK_IS_ON()
+  DCHECK(!iterating_);

[sadrul, 2016/12/22 21:37:52]

Since this is in a DCHECK, do you need the DCHECK_IS_ON check for it?

[varkha, 2016/12/22 22:36:02]

Yes, I get compiler "error: use of undeclared identifier 'iterating_'" in
Release build if I omit #ifdef.

+#endif
   children_.insert(children_.begin() + index, view);
 
   // Ensure the layer tree matches the view tree before calling to any client
@@ -237,6 +249,9 @@ void View::ReorderChildView(View* view, int index) {
 
   const Views::iterator i(std::find(children_.begin(), children_.end(), view));
   DCHECK(i != children_.end());
+#if DCHECK_IS_ON()
+  DCHECK(!iterating_);
+#endif
   children_.erase(i);
 
   // Unlink the view first
@@ -249,6 +264,9 @@ void View::ReorderChildView(View* view, int index) {
 
   // Add it in the specified index now.
   InitFocusSiblings(view, index);
+#if DCHECK_IS_ON()
+  DCHECK(!iterating_);
+#endif
   children_.insert(children_.begin() + index, view);

[sadrul, 2016/12/22 21:37:52]

Don't need both this and in 252:254?

[varkha, 2016/12/22 22:36:01]

Done.

 
   for (ViewObserver& observer : observers_)
@@ -554,8 +572,10 @@ 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.
-  for (int i = 0, count = child_count(); i < count; ++i) {
-    View* child = child_at(i);
+#if DCHECK_IS_ON()
+  base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+  for (auto* child : children_) {
     if (child->needs_layout_ || !layout_manager_.get()) {
       TRACE_EVENT1("views", "View::Layout", "class", child->GetClassName());
       child->needs_layout_ = false;
@@ -621,8 +641,11 @@ const View* View::GetViewByID(int id) const {
   if (id == id_)
     return const_cast<View*>(this);
 
-  for (int i = 0, count = child_count(); i < count; ++i) {
-    const View* view = child_at(i)->GetViewByID(id);
+#if DCHECK_IS_ON()
+  base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+  for (auto* child : children_) {
+    const View* view = child->GetViewByID(id);
     if (view)
       return view;
   }
@@ -651,8 +674,11 @@ void View::GetViewsInGroup(int group, Views* views) {
   if (group_ == group)
     views->push_back(this);
 
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->GetViewsInGroup(group, views);
+#if DCHECK_IS_ON()
+  base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+  for (auto* child : children_)
+    child->GetViewsInGroup(group, views);
 }
 
 View* View::GetSelectedViewForGroup(int group) {
@@ -917,8 +943,7 @@ View* View::GetTooltipHandlerForPoint(const gfx::Point& point) {
 
   // Walk the child Views recursively looking for the View that most
   // tightly encloses the specified point.
-  for (int i = child_count() - 1; i >= 0; --i) {
-    View* child = child_at(i);
+  for (auto* child : base::Reversed(children_)) {
     if (!child->visible())
       continue;
 
@@ -1437,9 +1462,13 @@ void View::NativeViewHierarchyChanged() {
 
 void View::PaintChildren(const ui::PaintContext& context) {
   TRACE_EVENT1("views", "View::PaintChildren", "class", GetClassName());
-  for (int i = 0, count = child_count(); i < count; ++i)
-    if (!child_at(i)->layer())
-      child_at(i)->Paint(context);
+#if DCHECK_IS_ON()
+  base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+  for (auto* child : children_) {
+    if (!child->layer())
+      child->Paint(context);
+  }
 }
 
 void View::OnPaint(gfx::Canvas* canvas) {
@@ -1505,8 +1534,11 @@ void View::MoveLayerToParent(ui::Layer* parent_layer,
     SetLayerBounds(gfx::Rect(local_point.x(), local_point.y(),
                              width(), height()));
   } else {
-    for (int i = 0, count = child_count(); i < count; ++i)
-      child_at(i)->MoveLayerToParent(parent_layer, local_point);
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_)
+      child->MoveLayerToParent(parent_layer, local_point);
   }
 }
 
@@ -1522,8 +1554,11 @@ void View::UpdateChildLayerVisibility(bool ancestor_visible) {
   if (layer()) {
     layer()->SetVisible(ancestor_visible && visible_);
   } else {
-    for (int i = 0, count = child_count(); i < count; ++i)
-      child_at(i)->UpdateChildLayerVisibility(ancestor_visible && visible_);
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_)
+      child->UpdateChildLayerVisibility(ancestor_visible && visible_);
   }
 }
 
@@ -1531,8 +1566,10 @@ void View::UpdateChildLayerBounds(const gfx::Vector2d& offset) {
   if (layer()) {
     SetLayerBounds(GetLocalBounds() + offset);
   } else {
-    for (int i = 0, count = child_count(); i < count; ++i) {
-      View* child = child_at(i);
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_) {
       child->UpdateChildLayerBounds(
           offset + gfx::Vector2d(child->GetMirroredX(), child->y()));
     }
@@ -1588,9 +1625,11 @@ 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.
-    for (Views::reverse_iterator it(children_.rbegin());
-         it != children_.rend(); ++it) {
-      (*it)->ReorderChildLayers(parent_layer);
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : base::Reversed(children_)) {
+      child->ReorderChildLayers(parent_layer);
     }
   }
 }
@@ -1765,8 +1804,8 @@ std::string View::DoPrintViewGraph(bool first, View* view_with_children) {
   }
 
   // Children.
-  for (int i = 0, count = view_with_children->child_count(); i < count; ++i)
-    result.append(view_with_children->child_at(i)->PrintViewGraph(false));
+  for (auto* child : view_with_children->children_)
+    result.append(child->PrintViewGraph(false));
 
   if (first)
     result.append("}\n");
@@ -1862,6 +1901,9 @@ void View::DoRemoveChildView(View* view,
   if (delete_removed_view && !view->owned_by_client_)
     view_to_be_deleted.reset(view);
 
+#if DCHECK_IS_ON()
+  DCHECK(!iterating_);
+#endif
   children_.erase(i);
 
   if (update_tool_tip)
@@ -1875,8 +1917,13 @@ void View::DoRemoveChildView(View* view,
 }
 
 void View::PropagateRemoveNotifications(View* old_parent, View* new_parent) {
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->PropagateRemoveNotifications(old_parent, new_parent);
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_)
+      child->PropagateRemoveNotifications(old_parent, new_parent);
+  }
 
   ViewHierarchyChangedDetails details(false, old_parent, this, new_parent);
   for (View* v = this; v; v = v->parent_)
@@ -1885,14 +1932,24 @@ void View::PropagateRemoveNotifications(View* old_parent, View* new_parent) {
 
 void View::PropagateAddNotifications(
     const ViewHierarchyChangedDetails& details) {
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->PropagateAddNotifications(details);
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_)
+      child->PropagateAddNotifications(details);
+  }
   ViewHierarchyChangedImpl(true, details);
 }
 
 void View::PropagateNativeViewHierarchyChanged() {
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->PropagateNativeViewHierarchyChanged();
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_)
+      child->PropagateNativeViewHierarchyChanged();
+  }
   NativeViewHierarchyChanged();
 }
 
@@ -1916,16 +1973,26 @@ void View::ViewHierarchyChangedImpl(
 }
 
 void View::PropagateNativeThemeChanged(const ui::NativeTheme* theme) {
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->PropagateNativeThemeChanged(theme);
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_)
+      child->PropagateNativeThemeChanged(theme);
+  }
   OnNativeThemeChanged(theme);
 }
 
 // Size and disposition --------------------------------------------------------
 
 void View::PropagateVisibilityNotifications(View* start, bool is_visible) {
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->PropagateVisibilityNotifications(start, is_visible);
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_)
+      child->PropagateVisibilityNotifications(start, is_visible);
+  }
   VisibilityChangedImpl(start, is_visible);
 }
 
@@ -2106,8 +2173,13 @@ bool View::ConvertRectFromAncestor(const View* ancestor,
 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.
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->UpdateChildLayerVisibility(true);
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : children_)
+      child->UpdateChildLayerVisibility(true);
+  }
 
   SetLayer(base::MakeUnique<ui::Layer>());
   layer()->set_delegate(this);
@@ -2145,8 +2217,11 @@ bool View::UpdateParentLayers() {
     return false;
   }
   bool result = false;
-  for (int i = 0, count = child_count(); i < count; ++i) {
-    if (child_at(i)->UpdateParentLayers())
+#if DCHECK_IS_ON()
+  base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+  for (auto* child : children_) {
+    if (child->UpdateParentLayers())
       result = true;
   }
   return result;
@@ -2161,8 +2236,11 @@ void View::OrphanLayers() {
     // necessary to orphan the child layers.
     return;
   }
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->OrphanLayers();
+#if DCHECK_IS_ON()
+  base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+  for (auto* child : children_)
+    child->OrphanLayers();
 }
 
 void View::ReparentLayer(const gfx::Vector2d& offset, ui::Layer* parent_layer) {
@@ -2345,11 +2423,16 @@ void View::InitFocusSiblings(View* v, int index) {
       // the last focusable element. Let's try to find an element with no next
       // focusable element to link to.
       View* last_focusable_view = NULL;
-      for (Views::iterator i(children_.begin()); i != children_.end(); ++i) {
-          if (!(*i)->next_focusable_view_) {
-            last_focusable_view = *i;
+      {
+#if DCHECK_IS_ON()
+        base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+        for (auto* child : children_) {
+          if (!child->next_focusable_view_) {
+            last_focusable_view = child;
             break;
           }
+        }
       }
       if (last_focusable_view == NULL) {
         // Hum... there is a cycle in the focus list. Let's just insert ourself
@@ -2391,20 +2474,35 @@ void View::AdvanceFocusIfNecessary() {
 // System events ---------------------------------------------------------------
 
 void View::PropagateThemeChanged() {
-  for (int i = child_count() - 1; i >= 0; --i)
-    child_at(i)->PropagateThemeChanged();
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : base::Reversed(children_))
+      child->PropagateThemeChanged();
+  }
   OnThemeChanged();
 }
 
 void View::PropagateLocaleChanged() {
-  for (int i = child_count() - 1; i >= 0; --i)
-    child_at(i)->PropagateLocaleChanged();
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : base::Reversed(children_))
+      child->PropagateLocaleChanged();
+  }
   OnLocaleChanged();
 }
 
 void View::PropagateDeviceScaleFactorChanged(float device_scale_factor) {
-  for (int i = child_count() - 1; i >= 0; --i)
-    child_at(i)->PropagateDeviceScaleFactorChanged(device_scale_factor);
+  {
+#if DCHECK_IS_ON()
+    base::AutoReset<bool> iterating(&iterating_, true);
+#endif
+    for (auto* child : base::Reversed(children_))
+      child->PropagateDeviceScaleFactorChanged(device_scale_factor);
+  }

[sadrul, 2016/12/22 21:37:52]

An alternative to do this is something like this:

  ScopedChildLock lock(this);
  for (auto* child : base::Reversed(children_))
    child->....
  lock.reset();

Where ScopedChildLock looks like:

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

    ~ScopedChildLock() { reset(); }

    void reset() {
      if (!view_)
        return;
      view_->iterating_ = false;
      view_ = nullptr;
    }

    private:
     View* view_;
     DISALLOW_COPY_AND_ASSIGN(ScopedChildLock);
  };
#else
  class ScopedChildLock {
    public:
      explicit ScopedChildLock(View* view) {}
      ~ScopedChildLock() {}
      void reset() {}
  };
#endif

This way, you don't need the rest of the DCHECK_IS_ON(). We use this pattern
elsewhere (e.g. base::ThreadChecker), so that we don't need to use
DCHECK_IS_ON() in so many places inside the code.

[varkha, 2016/12/22 22:36:01]

Done.

 
   // If the view is drawing to the layer, OnDeviceScaleFactorChanged() is called
   // through LayerDelegate callback.