Provide RemoveAllChildViews default arguments

This makes RemoveAllChildViews default to deleting children that are not
set_owned_by_client(). A separate method,
RemoveAllChildViewsWithoutDeleting(), is added for pre-existing calls
that do not delete children. This method is marked as deprecated.

Removing explicit arguments to RemoveAllChildViews() is so far only done
in ui/views, follow-up CLs will remove them everywhere and eventually
remove the `delete_children` parameter from RemoveAllChildViews()
indicating that there should not be a way to remove children from a View
without either removing them or retaining their `unique_ptrs`.

Bug: 1044687
Change-Id: I0c823949dd0c32a6af7f6bfcc9c8bf1287cd3650
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3057500
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#906559}

PRESUBMIT.py

@@ -935,6 +935,16 @@ _BANNED_CPP_FUNCTIONS = (
       False,
       (),
     ),
+    (
+      'RemoveAllChildViewsWithoutDeleting',
+      (
+        'RemoveAllChildViewsWithoutDeleting is deprecated.',
+        'This method is deemed dangerous as, unless raw pointers are re-added,',
+        'calls to this method introduce memory leaks.'
+      ),
+      False,
+      (),
+    ),
     (
       r'/\bTRACE_EVENT_ASYNC_',
       (

chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc

@@ -172,7 +172,7 @@ TEST_F(AXTreeSourceAuraTest, Serialize) {
   ASSERT_EQ(1U, out_update.nodes.size());
 
   // Try removing some child views and re-adding which should fire some events.
-  content_->RemoveAllChildViews(false /* delete_children */);
+  content_->RemoveAllChildViewsWithoutDeleting();
   content_->AddChildView(textfield_);
 
   // Grab the textfield since serialization only walks up the tree (not down

ui/views/bubble/bubble_dialog_model_host.cc

@@ -275,7 +275,7 @@ BubbleDialogModelHost::BubbleDialogModelHost(
 
 BubbleDialogModelHost::~BubbleDialogModelHost() {
   // Remove children as they may refer to the soon-to-be-destructed model.
-  contents_view_->RemoveAllChildViews(true);
+  contents_view_->RemoveAllChildViews();
 }
 
 std::unique_ptr<BubbleDialogModelHost> BubbleDialogModelHost::CreateModal(
@@ -340,7 +340,7 @@ void BubbleDialogModelHost::Close() {
   // TODO(pbos): Consider turning this into for-each-field remove field.
   // TODO(pbos): Move this into a better-named call inside contents_view_ to
   // make it clear that the model_ is about to be destroyed.
-  contents_view_->RemoveAllChildViews(true);
+  contents_view_->RemoveAllChildViews();
   fields_.clear();
   model_.reset();
 }

ui/views/cocoa/bridged_native_widget_unittest.mm

@@ -543,7 +543,7 @@ Textfield* BridgedNativeWidgetTest::InstallTextField(
   textfield->SetText(text);
   textfield->SetTextInputType(text_input_type);
   textfield->set_controller(this);
-  view_->RemoveAllChildViews(true);
+  view_->RemoveAllChildViews();
   view_->AddChildView(textfield);
   textfield->SetBoundsRect(bounds_);
 

ui/views/controls/menu/menu_item_view.cc

@@ -402,7 +402,7 @@ void MenuItemView::RemoveAllMenuItems() {
   removed_items_.insert(removed_items_.end(), submenu_->children().begin(),
                         submenu_->children().end());
 
-  submenu_->RemoveAllChildViews(false);
+  submenu_->RemoveAllChildViewsWithoutDeleting();
 }
 
 MenuItemView* MenuItemView::AppendMenuItem(int item_id,

ui/views/examples/examples_window.cc

@@ -206,7 +206,7 @@ class ExamplesWindowContents : public WidgetDelegateView {
   void ComboboxChanged() {
     int index = combobox_->GetSelectedIndex();
     DCHECK_LT(index, combobox_model_->GetItemCount());
-    example_shown_->RemoveAllChildViews(false);
+    example_shown_->RemoveAllChildViewsWithoutDeleting();
     example_shown_->AddChildView(combobox_model_->GetItemViewAt(index));
     example_shown_->RequestFocus();
     SetStatus(std::string());

ui/views/layout/box_layout_unittest.cc

@@ -196,7 +196,7 @@ TEST_F(BoxLayoutTest, EmptyPreferredSize) {
     BoxLayout::Orientation orientation =
         i == 0 ? BoxLayout::Orientation::kHorizontal
                : BoxLayout::Orientation::kVertical;
-    host_->RemoveAllChildViews(true);
+    host_->RemoveAllChildViews();
     host_->SetLayoutManager(
         std::make_unique<BoxLayout>(orientation, gfx::Insets(), 5));
     View* v1 = new StaticSizedView(gfx::Size());
@@ -218,7 +218,7 @@ TEST_F(BoxLayoutTest, EmptyPreferredSize) {
 // Verifies that a BoxLayout correctly handles child spacing, flex layout, and
 // empty preferred size, simultaneously.
 TEST_F(BoxLayoutTest, EmptyPreferredSizeWithFlexLayoutAndChildSpacing) {
-  host_->RemoveAllChildViews(true);
+  host_->RemoveAllChildViews();
   BoxLayout* layout = host_->SetLayoutManager(std::make_unique<BoxLayout>(
       BoxLayout::Orientation::kHorizontal, gfx::Insets(), 5));
   View* v1 = new StaticSizedView(gfx::Size());
@@ -862,7 +862,7 @@ TEST_F(BoxLayoutTest, OverlappingCrossMarginsAlignEnd) {
 
     EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
   }
-  host_->RemoveAllChildViews(true);
+  host_->RemoveAllChildViews();
   {
     BoxLayout* layout = host_->SetLayoutManager(std::make_unique<BoxLayout>(
         BoxLayout::Orientation::kHorizontal, gfx::Insets(0, 0), 0, true));
@@ -892,7 +892,7 @@ TEST_F(BoxLayoutTest, OverlappingCrossMarginsAlignStretch) {
 
     EXPECT_EQ(10, layout->GetPreferredSize(host_.get()).height());
   }
-  host_->RemoveAllChildViews(true);
+  host_->RemoveAllChildViews();
   {
     BoxLayout* layout = host_->SetLayoutManager(std::make_unique<BoxLayout>(
         BoxLayout::Orientation::kHorizontal, gfx::Insets(0, 0), 0, true));
@@ -922,7 +922,7 @@ TEST_F(BoxLayoutTest, OverlappingCrossMarginsAlignStart) {
 
     EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
   }
-  host_->RemoveAllChildViews(true);
+  host_->RemoveAllChildViews();
   {
     BoxLayout* layout = host_->SetLayoutManager(std::make_unique<BoxLayout>(
         BoxLayout::Orientation::kHorizontal, gfx::Insets(0, 0), 0, true));

ui/views/view.cc

@@ -316,6 +316,12 @@ void View::RemoveAllChildViews(bool delete_children) {
   UpdateTooltip();
 }
 
+void View::RemoveAllChildViewsWithoutDeleting() {
+  while (!children_.empty())
+    DoRemoveChildView(children_.front(), false, false, nullptr);
+  UpdateTooltip();
+}
+
 bool View::Contains(const View* view) const {
   for (const View* v = view; v; v = v->parent_) {
     if (v == this)

ui/views/view.h

@@ -450,9 +450,16 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
     return base::WrapUnique(view);
   }
 
-  // Removes all the children from this view. If |delete_children| is true,
+  // Removes all the children from this view. This deletes all children that are
-  // the views are deleted, unless marked as not parent owned.
+  // not set_owned_by_client(), which is deprecated.
-  void RemoveAllChildViews(bool delete_children);
+  // TODO(pbos): Remove `delete_children` argument and migrate past `false` uses
+  // to RemoveAllChildViewsWithoutDeleting().
+  void RemoveAllChildViews(bool delete_children = true);
+
+  // TODO(pbos): Remove this method, deleting children when removing them should
+  // not be optional. If ownership needs to be preserved, use RemoveChildViewT()
+  // to retain ownership of the removed children.
+  void RemoveAllChildViewsWithoutDeleting();
 
   const Views& children() const { return children_; }
 

ui/views/view_unittest.cc

@@ -3499,7 +3499,7 @@ TEST_F(ViewTest, RemoveAllChildViews) {
   EXPECT_EQ(3u, foo->children().size());
 
   // Now remove all child views from root.
-  root.RemoveAllChildViews(true);
+  root.RemoveAllChildViews();
 
   EXPECT_TRUE(root.children().empty());
 }
@@ -5499,7 +5499,7 @@ TEST_F(ViewTest, RemoveAllChildViewsNullsFocusListPointers) {
   View* const second = parent.AddChildView(std::make_unique<View>());
   View* const last = parent.AddChildView(std::make_unique<View>());
 
-  parent.RemoveAllChildViews(false /* delete_children */);
+  parent.RemoveAllChildViewsWithoutDeleting();
 
   EXPECT_EQ(first->GetPreviousFocusableView(), nullptr);
   EXPECT_EQ(first->GetNextFocusableView(), nullptr);

ui/views/widget/root_view.cc

@@ -209,7 +209,7 @@ RootView::RootView(Widget* widget)
 RootView::~RootView() {
   // If we have children remove them explicitly so to make sure a remove
   // notification is sent for each one of them.
-  RemoveAllChildViews(true);
+  RemoveAllChildViews();
 }
 
 // Tree operations -------------------------------------------------------------
@@ -221,7 +221,7 @@ void RootView::SetContentsView(View* contents_view) {
   // Widget pointer is valid.
   SetUseDefaultFillLayout(true);
   if (!children().empty())
-    RemoveAllChildViews(true);
+    RemoveAllChildViews();
   AddChildView(contents_view);
 }
 

ui/views/widget/widget.cc

@@ -1760,7 +1760,7 @@ void Widget::DestroyRootView() {
   // Remove all children before the unique_ptr reset so that
   // GetWidget()->GetRootView() doesn't return nullptr while the views hierarchy
   // is being torn down.
-  root_view_->RemoveAllChildViews(true);
+  root_view_->RemoveAllChildViews();
   root_view_.reset();
 }