Add children() and FindChild() methods to View.

Make use of these in a few places to reduce use of index-based APIs or improve
efficiency.

Bug: 940135, 942969
Change-Id: I879bec72ed25bfb2b2a1ee508fb798f76124f72f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1526628
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#641995}
diff --git a/ash/app_list/views/apps_grid_view_unittest.cc b/ash/app_list/views/apps_grid_view_unittest.cc
index be2b030..ef03caa 100644
--- a/ash/app_list/views/apps_grid_view_unittest.cc
+++ b/ash/app_list/views/apps_grid_view_unittest.cc
@@ -269,12 +269,12 @@
     const views::ViewModelT<AppListItemView>* view_model =
         apps_grid_view_->view_model();
     DCHECK_GT(view_model->view_size(), 0);
-    const int initial_index =
-        apps_grid_view_->GetIndexOf(view_model->view_at(0));
-    DCHECK_NE(-1, initial_index);
-    for (int i = 0; i < view_model->view_size(); ++i) {
-      EXPECT_EQ(view_model->view_at(i),
-                apps_grid_view_->child_at(i + initial_index));
+    auto app_iter = apps_grid_view_->FindChild(view_model->view_at(0));
+    DCHECK(app_iter != apps_grid_view_->children().cend());
+    for (int i = 1; i < view_model->view_size(); ++i) {
+      ++app_iter;
+      ASSERT_NE(apps_grid_view_->children().cend(), app_iter);
+      EXPECT_EQ(view_model->view_at(i), *app_iter);
     }
   }
 
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
index b5a6b47..676ebfb 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc
@@ -210,12 +210,12 @@
   EXPECT_EQ(6, test_helper_->GetBookmarkButtonCount());
 
   // Ensure buttons were added in the correct place.
-  int managed_button_index =
-      bookmark_bar_view_->GetIndexOf(test_helper_->managed_bookmarks_button());
+  auto button_iter =
+      bookmark_bar_view_->FindChild(test_helper_->managed_bookmarks_button());
   for (int i = 0; i < test_helper_->GetBookmarkButtonCount(); ++i) {
-    views::View* button = test_helper_->GetBookmarkButton(i);
-    EXPECT_EQ(bookmark_bar_view_->GetIndexOf(button),
-              managed_button_index + 1 + i);
+    ++button_iter;
+    ASSERT_NE(bookmark_bar_view_->children().cend(), button_iter);
+    EXPECT_EQ(test_helper_->GetBookmarkButton(i), *button_iter);
   }
 }
 
@@ -234,12 +234,12 @@
   bookmark_bar_view_->Layout();
   EXPECT_EQ(6, test_helper_->GetBookmarkButtonCount());
   // Ensure buttons were added in the correct place.
-  int managed_button_index =
-      bookmark_bar_view_->GetIndexOf(test_helper_->managed_bookmarks_button());
+  auto button_iter =
+      bookmark_bar_view_->FindChild(test_helper_->managed_bookmarks_button());
   for (int i = 0; i < test_helper_->GetBookmarkButtonCount(); ++i) {
-    views::View* button = test_helper_->GetBookmarkButton(i);
-    EXPECT_EQ(bookmark_bar_view_->GetIndexOf(button),
-              managed_button_index + 1 + i);
+    ++button_iter;
+    ASSERT_NE(bookmark_bar_view_->children().cend(), button_iter);
+    EXPECT_EQ(test_helper_->GetBookmarkButton(i), *button_iter);
   }
 }
 
diff --git a/chrome/browser/ui/views/frame/browser_view_unittest.cc b/chrome/browser/ui/views/frame/browser_view_unittest.cc
index 063fb57..3639907 100644
--- a/chrome/browser/ui/views/frame/browser_view_unittest.cc
+++ b/chrome/browser/ui/views/frame/browser_view_unittest.cc
@@ -98,10 +98,10 @@
 
   // Find bar host is at the front of the view hierarchy, followed by the
   // infobar container and then top container.
-  EXPECT_EQ(browser_view()->child_count() - 1,
-            browser_view()->GetIndexOf(browser_view()->find_bar_host_view()));
-  EXPECT_EQ(browser_view()->child_count() - 2,
-            browser_view()->GetIndexOf(browser_view()->infobar_container()));
+  ASSERT_GE(browser_view()->children().size(), 2U);
+  auto child = browser_view()->children().crbegin();
+  EXPECT_EQ(browser_view()->find_bar_host_view(), *child++);
+  EXPECT_EQ(browser_view()->infobar_container(), *child);
 
   // Verify basic layout.
   EXPECT_EQ(0, top_container->x());
@@ -143,10 +143,10 @@
 
   // Find bar host is still at the front of the view hierarchy, followed by the
   // infobar container and then top container.
-  EXPECT_EQ(browser_view()->child_count() - 1,
-            browser_view()->GetIndexOf(browser_view()->find_bar_host_view()));
-  EXPECT_EQ(browser_view()->child_count() - 2,
-            browser_view()->GetIndexOf(browser_view()->infobar_container()));
+  ASSERT_GE(browser_view()->children().size(), 2U);
+  child = browser_view()->children().crbegin();
+  EXPECT_EQ(browser_view()->find_bar_host_view(), *child++);
+  EXPECT_EQ(browser_view()->infobar_container(), *child);
 
   // Bookmark bar layout on NTP.
   EXPECT_EQ(0, bookmark_bar->x());
diff --git a/chrome/browser/ui/views/infobars/infobar_view.cc b/chrome/browser/ui/views/infobars/infobar_view.cc
index ac8a9e3..680278b 100644
--- a/chrome/browser/ui/views/infobars/infobar_view.cc
+++ b/chrome/browser/ui/views/infobars/infobar_view.cc
@@ -163,8 +163,7 @@
       OffsetY(close_button_)));
 
   // For accessibility reasons, the close button should come last.
-  DCHECK_EQ(close_button_->parent()->child_count() - 1,
-            close_button_->parent()->GetIndexOf(close_button_));
+  DCHECK_EQ(close_button_, close_button_->parent()->children().back());
 }
 
 void InfoBarView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
diff --git a/chrome/browser/ui/views/tabs/tab_strip_unittest.cc b/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
index a106daa7..e85f7ed 100644
--- a/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
+++ b/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
@@ -361,7 +361,7 @@
     Tab* left = tab_strip->tab_at(i - 1);
     Tab* right = tab_strip->tab_at(i);
 
-    if (tab_strip->GetIndexOf(right) < tab_strip->GetIndexOf(left)) {
+    if (tab_strip->FindChild(right) < tab_strip->FindChild(left)) {
       return false;
     }
   }
diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc
index df410ba..36d728b 100644
--- a/ui/views/controls/menu/menu_controller.cc
+++ b/ui/views/controls/menu/menu_controller.cc
@@ -129,12 +129,13 @@
 }
 
 // Recurses through the child views of |view| returning the first view starting
-// at |start| that is focusable. Children are considered first to last.
+// at |pos| that is focusable. Children are considered first to last.
 // TODO(https://crbug.com/942358): This can also return |view|, which seems
 // incorrect.
-View* GetFirstFocusableViewForward(View* view, int start) {
-  for (int i = start; i < view->child_count(); ++i) {
-    View* deepest = GetFirstFocusableViewForward(view->child_at(i), 0);
+View* GetFirstFocusableViewForward(View* view,
+                                   View::Views::const_iterator pos) {
+  for (auto i = pos; i != view->children().cend(); ++i) {
+    View* deepest = GetFirstFocusableViewForward(*i, (*i)->children().cbegin());
     if (deepest)
       return deepest;
   }
@@ -142,10 +143,11 @@
 }
 
 // As GetFirstFocusableViewForward(), but children are considered last to first.
-View* GetFirstFocusableViewBackward(View* view, int start) {
-  for (int i = start; i >= 0; --i) {
-    View* deepest = GetFirstFocusableViewBackward(view->child_at(i),
-                                                  view->child_count() - 1);
+View* GetFirstFocusableViewBackward(View* view,
+                                    View::Views::const_reverse_iterator pos) {
+  for (auto i = pos; i != view->children().crend(); ++i) {
+    View* deepest =
+        GetFirstFocusableViewBackward(*i, (*i)->children().crbegin());
     if (deepest)
       return deepest;
   }
@@ -154,9 +156,9 @@
 
 // Returns the first child of |start| that is focusable.
 View* GetInitialFocusableView(View* start, bool forward) {
-  return forward
-             ? GetFirstFocusableViewForward(start, 0)
-             : GetFirstFocusableViewBackward(start, start->child_count() - 1);
+  const auto& children = start->children();
+  return forward ? GetFirstFocusableViewForward(start, children.cbegin())
+                 : GetFirstFocusableViewBackward(start, children.crbegin());
 }
 
 // Returns the next view after |start_at| that is focusable. Returns null if
@@ -166,9 +168,13 @@
   View* parent = start_at;
   do {
     View* new_parent = parent->parent();
-    int index = new_parent->GetIndexOf(parent);
-    View* next = forward ? GetFirstFocusableViewForward(new_parent, index + 1)
-                         : GetFirstFocusableViewBackward(new_parent, index - 1);
+    const auto pos = new_parent->FindChild(parent);
+    // Subtle: make_reverse_iterator() will result in an iterator that refers to
+    // the element before its argument, which is what we want.
+    View* next = forward
+                     ? GetFirstFocusableViewForward(new_parent, std::next(pos))
+                     : GetFirstFocusableViewBackward(
+                           new_parent, std::make_reverse_iterator(pos));
     if (next)
       return next;
     parent = new_parent;
diff --git a/ui/views/controls/menu/menu_item_view.cc b/ui/views/controls/menu/menu_item_view.cc
index 0eab270..157ad2ba 100644
--- a/ui/views/controls/menu/menu_item_view.cc
+++ b/ui/views/controls/menu/menu_item_view.cc
@@ -320,8 +320,8 @@
 void MenuItemView::RemoveAllMenuItems() {
   DCHECK(submenu_);
 
-  for (int i = 0; i < submenu_->child_count(); ++i)
-    removed_items_.push_back(submenu_->child_at(i));
+  removed_items_.insert(removed_items_.end(), submenu_->children().begin(),
+                        submenu_->children().end());
 
   submenu_->RemoveAllChildViews(false);
 }
diff --git a/ui/views/view.cc b/ui/views/view.cc
index 359728e..09d8cbf 100644
--- a/ui/views/view.cc
+++ b/ui/views/view.cc
@@ -207,8 +207,12 @@
   return false;
 }
 
+View::Views::const_iterator View::FindChild(const View* view) const {
+  return std::find(children_.cbegin(), children_.cend(), view);
+}
+
 int View::GetIndexOf(const View* view) const {
-  const auto i = std::find(children_.cbegin(), children_.cend(), view);
+  const auto i = FindChild(view);
   return i == children_.cend()
              ? -1
              : static_cast<int>(std::distance(children_.cbegin(), i));
diff --git a/ui/views/view.h b/ui/views/view.h
index fc3dcde..7e2110aa 100644
--- a/ui/views/view.h
+++ b/ui/views/view.h
@@ -315,6 +315,10 @@
   // the views are deleted, unless marked as not parent owned.
   void RemoveAllChildViews(bool delete_children);
 
+  // TODO(https://crbug.com/940135): Remove child_count(), has_children(), and
+  // child_at() in favor of this.
+  const Views& children() const { return children_; }
+
   int child_count() const { return static_cast<int>(children_.size()); }
   // See also |GetChildrenInZOrder()| below that returns |children_|
   // in reverse z-order.
@@ -338,6 +342,10 @@
   // an indirect descendant. Will return true if child is also this view.
   bool Contains(const View* view) const;
 
+  // Returns an iterator pointing to |view|, or children_.cend() if |view| is
+  // not a child of this view.
+  Views::const_iterator FindChild(const View* view) const;
+
   // Returns the index of |view|, or -1 if |view| is not a child of this view.
   int GetIndexOf(const View* view) const;
 
diff --git a/ui/views/view_unittest.cc b/ui/views/view_unittest.cc
index b101a0a..20307e4 100644
--- a/ui/views/view_unittest.cc
+++ b/ui/views/view_unittest.cc
@@ -79,9 +79,8 @@
   const views::View* parent = v->parent();
   if (!parent)
     return nullptr;
-  const int next = parent->GetIndexOf(v) + 1;
-  return (next == parent->child_count()) ? parent
-                                         : FirstView(parent->child_at(next));
+  const auto next = std::next(parent->FindChild(v));
+  return (next == parent->children().cend()) ? parent : FirstView(*next);
 }
 
 // Convenience functions for walking a Layer tree.