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

ui/views/view.cc

 // Copyright (c) 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.
 
 #define _USE_MATH_DEFINES // For VC++ to get M_PI. This has to be first.
 
 #include "ui/views/view.h"
 
 #include <algorithm>
 #include <cmath>
 #include <memory>
 #include <utility>
 
+#include "base/containers/adapters.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "third_party/skia/include/core/SkRect.h"
@@ skipping 58 matching lines @@
 const int kDefaultVerticalDragThreshold = 8;
 
 // Returns the top view in |view|'s hierarchy.
 const View* GetHierarchyRoot(const View* view) {
   const View* root = view;
   while (root && root->parent())
     root = root->parent();
   return root;
 }
 
+#if DCHECK_IS_ON()
+  class ScopedChildrenLock {
+   public:
+    explicit ScopedChildrenLock(const View* view) : view_(view) {
+      DCHECK(!view->iterating_);
+      view->iterating_ = true;

[Daniele Castagna, 2016/12/23 23:45:14]

|iterating_| is private and this code doesn't compile with DCHECKS on.
crrev.com/2606513002 if you are fine making |iterating_| public.

[sadrul, 2016/12/24 00:13:11]

|iterating_| should remain private. But I think ScopedChildrenLock needs to be
in non-anon namespace for the friend declared in .h to work. But I believe our
trybot/buildbots use dcheck_always_on flag, which means they do exercise the
DCHECKs. It's curious that those bots are not failing to build. Do you have a
link to a failing bot, or if you are getting the failures locally, are you using
some non-standard configs?

[Daniele Castagna, 2016/12/24 00:48:01]

I'm not sure why we're not catching that on trybots.

I had trouble building locally for cyan with $GN_ARGS plus
dcheck_always_on=true, so, nothing non-standard.

I noticed ScopedChildrenLock was already friend (or at least, that seemed the
intention), crrev.com/2606513002 has been updated moving that class to internal
namesapce.

+    }
+
+    ~ScopedChildrenLock() { view_->iterating_ = false; }
+
+    private:
+     const View* view_;
+     DISALLOW_COPY_AND_ASSIGN(ScopedChildrenLock);
+  };
+#else
+  class ScopedChildrenLock {
+    public:
+      explicit ScopedChildrenLock(const View* view) {}
+      ~ScopedChildrenLock() {}
+  };
+#endif
+
 }  // namespace
 
 // static
 const char View::kViewClassName[] = "View";
 
 ////////////////////////////////////////////////////////////////////////////////
 // View, public:
 
 // Creation and lifetime -------------------------------------------------------
 
 View::View()
     : owned_by_client_(false),
       id_(0),
       group_(-1),
       parent_(NULL),
+#if DCHECK_IS_ON()
+      iterating_(false),
+#endif
       visible_(true),
       enabled_(true),
       notify_enter_exit_on_child_(false),
       registered_for_visible_bounds_notification_(false),
       needs_layout_(true),
       snap_layer_to_pixel_boundary_(false),
       flip_canvas_on_paint_for_rtl_ui_(false),
       paint_to_layer_(false),
       accelerator_focus_manager_(NULL),
       registered_accelerator_count_(0),
       next_focusable_view_(NULL),
       previous_focusable_view_(NULL),
       focus_behavior_(FocusBehavior::NEVER),
       context_menu_controller_(NULL),
       drag_controller_(NULL),
       native_view_accessibility_(NULL) {
   SetTargetHandler(this);
 }
 
 View::~View() {
   if (parent_)
     parent_->RemoveChildView(this);
 
   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;
+  {
+    ScopedChildrenLock(this);
+    for (auto* child : children_) {
+      child->parent_ = NULL;
+      if (!child->owned_by_client_)
+        delete child;
+    }
   }
 
   // Release ownership of the native accessibility object, but it's
   // reference-counted on some platforms, so it may not be deleted right away.
   if (native_view_accessibility_)
     native_view_accessibility_->Destroy();
 }
 
 // Tree operations -------------------------------------------------------------
 
@@ skipping 26 matching lines @@
       ReorderChildView(view, index);
       return;
     }
     parent->DoRemoveChildView(view, true, true, false, this);
   }
 
   // Sets the prev/next focus views.
   InitFocusSiblings(view, index);
 
   view->parent_ = this;
+#if DCHECK_IS_ON()
+  DCHECK(!iterating_);
+#endif
   children_.insert(children_.begin() + index, view);
 
   // Ensure the layer tree matches the view tree before calling to any client
   // code. This way if client code further modifies the view tree we are in a
   // sane state.
   const bool did_reparent_any_layers = view->UpdateParentLayers();
   Widget* widget = GetWidget();
   if (did_reparent_any_layers && widget)
     widget->UpdateRootLayers();
 
@@ skipping 38 matching lines @@
   DCHECK_EQ(view->parent_, this);
   if (index < 0)
     index = child_count() - 1;
   else if (index >= child_count())
     return;
   if (children_[index] == view)
     return;
 
   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
   View* next_focusable = view->next_focusable_view_;
   View* prev_focusable = view->previous_focusable_view_;
   if (prev_focusable)
     prev_focusable->next_focusable_view_ = next_focusable;
   if (next_focusable)
     next_focusable->previous_focusable_view_ = prev_focusable;
 
@@ skipping 297 matching lines @@
   // If we have a layout manager, let it handle the layout for us.
   if (layout_manager_.get())
     layout_manager_->Layout(this);
 
   // Make sure to propagate the Layout() call to any children that haven't
   // received it yet through the layout manager and need to be laid out. This
   // is needed for the case when the child requires a layout but its bounds
   // 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);
+  ScopedChildrenLock(this);
+  for (auto* child : children_) {
     if (child->needs_layout_ || !layout_manager_.get()) {
       TRACE_EVENT1("views", "View::Layout", "class", child->GetClassName());
       child->needs_layout_ = false;
       child->Layout();
     }
   }
 }
 
 void View::InvalidateLayout() {
   // Always invalidate up. This is needed to handle the case of us already being
@@ skipping 45 matching lines @@
 
 View* View::GetAncestorWithClassName(const std::string& name) {
   return const_cast<View*>(const_cast<const View*>(this)->
       GetAncestorWithClassName(name));
 }
 
 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);
+  ScopedChildrenLock(this);
+  for (auto* child : children_) {
+    const View* view = child->GetViewByID(id);
     if (view)
       return view;
   }
   return NULL;
 }
 
 View* View::GetViewByID(int id) {
   return const_cast<View*>(const_cast<const View*>(this)->GetViewByID(id));
 }
 
 void View::SetGroup(int gid) {
   // Don't change the group id once it's set.
   DCHECK(group_ == -1 || group_ == gid);
   group_ = gid;
 }
 
 int View::GetGroup() const {
   return group_;
 }
 
 bool View::IsGroupFocusTraversable() const {
   return true;
 }
 
 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);
+  ScopedChildrenLock(this);
+  for (auto* child : children_)
+    child->GetViewsInGroup(group, views);
 }
 
 View* View::GetSelectedViewForGroup(int group) {
   Views views;
   GetWidget()->GetRootView()->GetViewsInGroup(group, &views);
   return views.empty() ? NULL : views[0];
 }
 
 // Coordinate conversion -------------------------------------------------------
 
@@ skipping 244 matching lines @@
   return true;
 }
 
 View* View::GetTooltipHandlerForPoint(const gfx::Point& point) {
   // TODO(tdanderson): Move this implementation into ViewTargetDelegate.
   if (!HitTestPoint(point) || !CanProcessEventsWithinSubtree())
     return NULL;
 
   // 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;
 
     gfx::Point point_in_child_coords(point);
     ConvertPointToTarget(this, child, &point_in_child_coords);
     View* handler = child->GetTooltipHandlerForPoint(point_in_child_coords);
     if (handler)
       return handler;
   }
   return this;
@@ skipping 498 matching lines @@
 
     if (focus_manager)
       RegisterPendingAccelerators();
   }
 }
 
 // Painting --------------------------------------------------------------------
 
 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);
+  ScopedChildrenLock(this);
+  for (auto* child : children_) {
+    if (!child->layer())
+      child->Paint(context);
+  }
 }
 
 void View::OnPaint(gfx::Canvas* canvas) {
   TRACE_EVENT1("views", "View::OnPaint", "class", GetClassName());
   OnPaintBackground(canvas);
   OnPaintBorder(canvas);
 }
 
 void View::OnPaintBackground(gfx::Canvas* canvas) {
   if (background_.get()) {
@@ skipping 45 matching lines @@
 void View::MoveLayerToParent(ui::Layer* parent_layer,
                              const gfx::Point& point) {
   gfx::Point local_point(point);
   if (parent_layer != layer())
     local_point.Offset(GetMirroredX(), y());
   if (layer() && parent_layer != layer()) {
     parent_layer->Add(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);
+    ScopedChildrenLock(this);
+    for (auto* child : children_)
+      child->MoveLayerToParent(parent_layer, local_point);
   }
 }
 
 void View::UpdateLayerVisibility() {
   bool visible = visible_;
   for (const View* v = parent_; visible && v && !v->layer(); v = v->parent_)
     visible = v->visible();
 
   UpdateChildLayerVisibility(visible);
 }
 
 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_);
+    ScopedChildrenLock(this);
+    for (auto* child : children_)
+      child->UpdateChildLayerVisibility(ancestor_visible && visible_);
   }
 }
 
 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);
+    ScopedChildrenLock(this);
+    for (auto* child : children_) {
       child->UpdateChildLayerBounds(
           offset + gfx::Vector2d(child->GetMirroredX(), child->y()));
     }
   }
 }
 
 void View::OnPaintLayer(const ui::PaintContext& context) {
   Paint(context);
 }
 
@@ skipping 35 matching lines @@
 }
 
 void View::ReorderChildLayers(ui::Layer* parent_layer) {
   if (layer() && layer() != parent_layer) {
     DCHECK_EQ(parent_layer, layer()->parent());
     parent_layer->StackAtBottom(layer());
   } else {
     // 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);
+    ScopedChildrenLock(this);
+    for (auto* child : base::Reversed(children_)) {
+      child->ReorderChildLayers(parent_layer);
     }
   }
 }
 
 // Input -----------------------------------------------------------------------
 
 View::DragInfo* View::GetDragInfo() {
   return parent_ ? parent_->GetDragInfo() : NULL;
 }
 
@@ skipping 154 matching lines @@
 
     base::snprintf(pp, kMaxPointerStringLength, "%p", parent_);
     result.append("  N");
     result.append(pp + 2);
     result.append(" -> N");
     result.append(p + 2);
     result.append("\n");
   }
 
   // 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");
 
   return result;
 }
 #endif
 
 ////////////////////////////////////////////////////////////////////////////////
 // View, private:
@@ skipping 75 matching lines @@
   view->OrphanLayers();
   if (widget)
     widget->UpdateRootLayers();
 
   view->PropagateRemoveNotifications(this, new_parent);
   view->parent_ = nullptr;
 
   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)
     UpdateTooltip();
 
   if (layout_manager_)
     layout_manager_->ViewRemoved(this, view);
 
   for (ViewObserver& observer : observers_)
     observer.OnChildViewRemoved(view, this);
 }
 
 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);
+  {
+    ScopedChildrenLock(this);
+    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_)
     v->ViewHierarchyChangedImpl(true, details);
 }
 
 void View::PropagateAddNotifications(
     const ViewHierarchyChangedDetails& details) {
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->PropagateAddNotifications(details);
+  {
+    ScopedChildrenLock(this);
+    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();
+  {
+    ScopedChildrenLock(this);
+    for (auto* child : children_)
+      child->PropagateNativeViewHierarchyChanged();
+  }
   NativeViewHierarchyChanged();
 }
 
 void View::ViewHierarchyChangedImpl(
     bool register_accelerators,
     const ViewHierarchyChangedDetails& details) {
   if (register_accelerators) {
     if (details.is_add) {
       // If you get this registration, you are part of a subtree that has been
       // added to the view hierarchy.
       if (GetFocusManager())
         RegisterPendingAccelerators();
     } else {
       if (details.child == this)
         UnregisterAccelerators(true);
     }
   }
 
   ViewHierarchyChanged(details);
   details.parent->needs_layout_ = true;
 }
 
 void View::PropagateNativeThemeChanged(const ui::NativeTheme* theme) {
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->PropagateNativeThemeChanged(theme);
+  {
+    ScopedChildrenLock(this);
+    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);
+  {
+    ScopedChildrenLock(this);
+    for (auto* child : children_)
+      child->PropagateVisibilityNotifications(start, is_visible);
+  }
   VisibilityChangedImpl(start, is_visible);
 }
 
 void View::VisibilityChangedImpl(View* starting_from, bool is_visible) {
   VisibilityChanged(starting_from, is_visible);
 }
 
 void View::BoundsChanged(const gfx::Rect& previous_bounds) {
   if (visible_) {
     // Paint the new bounds.
@@ skipping 160 matching lines @@
   bool result = GetTransformRelativeTo(ancestor, &trans);
   trans.TransformRectReverse(rect);
   return result;
 }
 
 // Accelerated painting --------------------------------------------------------
 
 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);
+  {
+    ScopedChildrenLock(this);
+    for (auto* child : children_)
+      child->UpdateChildLayerVisibility(true);
+  }
 
   SetLayer(base::MakeUnique<ui::Layer>());
   layer()->set_delegate(this);
   layer()->set_name(GetClassName());
 
   UpdateParentLayers();
   UpdateLayerVisibility();
 
   // The new layer needs to be ordered in the layer tree according
   // to the view tree. Children of this layer were added in order
@@ skipping 17 matching lines @@
   if (layer()) {
     if (!layer()->parent()) {
       UpdateParentLayer();
       return true;
     }
     // The layers of any child views are already in place, so we can stop
     // iterating here.
     return false;
   }
   bool result = false;
-  for (int i = 0, count = child_count(); i < count; ++i) {
-    if (child_at(i)->UpdateParentLayers())
+  ScopedChildrenLock(this);
+  for (auto* child : children_) {
+    if (child->UpdateParentLayers())
       result = true;
   }
   return result;
 }
 
 void View::OrphanLayers() {
   if (layer()) {
     if (layer()->parent())
       layer()->parent()->Remove(layer());
 
     // The layer belonging to this View has already been orphaned. It is not
     // necessary to orphan the child layers.
     return;
   }
-  for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->OrphanLayers();
+  ScopedChildrenLock(this);
+  for (auto* child : children_)
+    child->OrphanLayers();
 }
 
 void View::ReparentLayer(const gfx::Vector2d& offset, ui::Layer* parent_layer) {
   layer()->SetBounds(GetLocalBounds() + offset);
   DCHECK_NE(layer(), parent_layer);
   if (parent_layer)
     parent_layer->Add(layer());
   layer()->SchedulePaint(GetLocalBounds());
   MoveLayerToParent(layer(), gfx::Point());
 }
@@ skipping 162 matching lines @@
 
   if (count == 0) {
     v->next_focusable_view_ = NULL;
     v->previous_focusable_view_ = NULL;
   } else {
     if (index == count) {
       // We are inserting at the end, but the end of the child list may not be
       // 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;
+      {
+        ScopedChildrenLock(this);
+        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
         // after the last child.
         View* prev = children_[index - 1];
         v->previous_focusable_view_ = prev;
         v->next_focusable_view_ = prev->next_focusable_view_;
         prev->next_focusable_view_->previous_focusable_view_ = v;
         prev->next_focusable_view_ = v;
       } else {
@@ skipping 21 matching lines @@
     return;
 
   FocusManager* focus_manager = GetFocusManager();
   if (focus_manager)
     focus_manager->AdvanceFocusIfNecessary();
 }
 
 // System events ---------------------------------------------------------------
 
 void View::PropagateThemeChanged() {
-  for (int i = child_count() - 1; i >= 0; --i)
-    child_at(i)->PropagateThemeChanged();
+  {
+    ScopedChildrenLock(this);
+    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();
+  {
+    ScopedChildrenLock(this);
+    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);
+  {
+    ScopedChildrenLock(this);
+    for (auto* child : base::Reversed(children_))
+      child->PropagateDeviceScaleFactorChanged(device_scale_factor);
+  }
 
   // If the view is drawing to the layer, OnDeviceScaleFactorChanged() is called
   // through LayerDelegate callback.
   if (!layer())
     OnDeviceScaleFactorChanged(device_scale_factor);
 }
 
 // Tooltips --------------------------------------------------------------------
 
 void View::UpdateTooltip() {
@@ skipping 30 matching lines @@
   // Message the RootView to do the drag and drop. That way if we're removed
   // the RootView can detect it and avoid calling us back.
   gfx::Point widget_location(event.location());
   ConvertPointToWidget(this, &widget_location);
   widget->RunShellDrag(this, data, widget_location, drag_operations, source);
   // WARNING: we may have been deleted.
   return true;
 }
 
 }  // namespace views