Misc. cleanup found while removing instances of View::child_at().

* Shorten code
* Use helpers for repeated code
* Fix case where we checked one child's classname, then cast a different child
* Use base::size()
* Use std::make_unique()
* Use base::nullopt
* Use View::Views
* constexpr kConstant
* Make GridLayoutTest.FixedSize consistent about loop nesting order
* Make ViewTest.ChildViewZOrderChanged consistent about checking children, then
  layers

Bug: none
Change-Id: I871af00669e629176aa8fd2cc4cac2736629b3f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1575166
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652712}

ui/views/accessibility/ax_aura_obj_cache.cc

@@ -173,13 +173,11 @@ View* AXAuraObjCache::GetFocusedView() {
     // If focused widget has non client view, falls back to first child view of
     // its client view. We don't expect that non client view gets keyboard
     // focus.
-    if (focused_widget->non_client_view() &&
-        focused_widget->non_client_view()->client_view() &&
-        !focused_widget->non_client_view()->client_view()->children().empty()) {
-      return focused_widget->non_client_view()->client_view()->child_at(0);
-    }
-
-    return focused_widget->GetRootView();
+    auto* non_client = focused_widget->non_client_view();
+    auto* client = non_client ? non_client->client_view() : nullptr;
+    return (client && !client->children().empty())
+               ? client->child_at(0)
+               : focused_widget->GetRootView();
   }
 
   return nullptr;

ui/views/controls/styled_label_unittest.cc

@@ -45,6 +45,13 @@ class StyledLabelTest : public ViewsTestBase, public StyledLabelListener {
  protected:
   StyledLabel* styled() { return styled_.get(); }
 
+  Label* LabelAt(int index,
+                 std::string expected_classname = Label::kViewClassName) {
+    View* const child = styled_->child_at(index);
+    EXPECT_EQ(expected_classname, child->GetClassName());
+    return static_cast<Label*>(child);
+  }
+
   void InitStyledLabel(const std::string& ascii_text) {
     styled_ = std::make_unique<StyledLabel>(ASCIIToUTF16(ascii_text), this);
     styled_->set_owned_by_client();
@@ -77,10 +84,7 @@ TEST_F(StyledLabelTest, TrailingWhitespaceiIgnored) {
   styled()->Layout();
 
   ASSERT_EQ(1u, styled()->children().size());
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(0)->GetClassName());
-  EXPECT_EQ(ASCIIToUTF16("This is a test block of text"),
-            static_cast<Label*>(styled()->child_at(0))->text());
+  EXPECT_EQ(ASCIIToUTF16("This is a test block of text"), LabelAt(0)->text());
 }
 
 TEST_F(StyledLabelTest, RespectLeadingWhitespace) {
@@ -91,10 +95,8 @@ TEST_F(StyledLabelTest, RespectLeadingWhitespace) {
   styled()->Layout();
 
   ASSERT_EQ(1u, styled()->children().size());
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(0)->GetClassName());
   EXPECT_EQ(ASCIIToUTF16("   This is a test block of text"),
-            static_cast<Label*>(styled()->child_at(0))->text());
+            LabelAt(0)->text());
 }
 
 TEST_F(StyledLabelTest, RespectLeadingSpacesInNonFirstLine) {
@@ -104,10 +106,7 @@ TEST_F(StyledLabelTest, RespectLeadingSpacesInNonFirstLine) {
   styled()->SetBounds(0, 0, 1000, 1000);
   styled()->Layout();
   ASSERT_EQ(2u, styled()->children().size());
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(0)->GetClassName());
-  EXPECT_EQ(ASCIIToUTF16(indented_line),
-            static_cast<Label*>(styled()->child_at(1))->text());
+  EXPECT_EQ(ASCIIToUTF16(indented_line), LabelAt(1)->text());
 }
 
 TEST_F(StyledLabelTest, CorrectWrapAtNewline) {
@@ -121,14 +120,10 @@ TEST_F(StyledLabelTest, CorrectWrapAtNewline) {
   styled()->SetBounds(0, 0, label_preferred_size.width(), 1000);
   styled()->Layout();
   ASSERT_EQ(2u, styled()->children().size());
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(1)->GetClassName());
-  EXPECT_EQ(ASCIIToUTF16(first_line),
-            static_cast<Label*>(styled()->child_at(0))->text());
-  EXPECT_EQ(ASCIIToUTF16(second_line),
-            static_cast<Label*>(styled()->child_at(1))->text());
-  EXPECT_EQ(styled()->GetHeightForWidth(1000),
-            styled()->child_at(1)->bounds().bottom());
+  EXPECT_EQ(ASCIIToUTF16(first_line), LabelAt(0)->text());
+  const auto* label_1 = LabelAt(1);
+  EXPECT_EQ(ASCIIToUTF16(second_line), label_1->text());
+  EXPECT_EQ(styled()->GetHeightForWidth(1000), label_1->bounds().bottom());
 }
 
 TEST_F(StyledLabelTest, FirstLineNotEmptyWhenLeadingWhitespaceTooLong) {
@@ -142,10 +137,7 @@ TEST_F(StyledLabelTest, FirstLineNotEmptyWhenLeadingWhitespaceTooLong) {
   styled()->Layout();
 
   ASSERT_EQ(1u, styled()->children().size());
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(0)->GetClassName());
-  EXPECT_EQ(ASCIIToUTF16("a"),
-            static_cast<Label*>(styled()->child_at(0))->text());
+  EXPECT_EQ(ASCIIToUTF16("a"), LabelAt(0)->text());
   EXPECT_EQ(label_preferred_size.height(),
             styled()->GetHeightForWidth(label_preferred_size.width() / 2));
 }
@@ -201,15 +193,14 @@ TEST_F(StyledLabelTest, WrapLongWords) {
 
   ASSERT_EQ(2u, styled()->children().size());
   ASSERT_EQ(gfx::Point(), styled()->origin());
-  EXPECT_EQ(gfx::Point(), styled()->child_at(0)->origin());
-  EXPECT_EQ(gfx::Point(0, styled()->height() / 2),
-            styled()->child_at(1)->origin());
-
-  EXPECT_FALSE(static_cast<Label*>(styled()->child_at(0))->text().empty());
-  EXPECT_FALSE(static_cast<Label*>(styled()->child_at(1))->text().empty());
-  EXPECT_EQ(ASCIIToUTF16(text),
-            static_cast<Label*>(styled()->child_at(0))->text() +
-                static_cast<Label*>(styled()->child_at(1))->text());
+  const auto* label_0 = LabelAt(0);
+  const auto* label_1 = LabelAt(1);
+  EXPECT_EQ(gfx::Point(), label_0->origin());
+  EXPECT_EQ(gfx::Point(0, styled()->height() / 2), label_1->origin());
+
+  EXPECT_FALSE(label_0->text().empty());
+  EXPECT_FALSE(label_1->text().empty());
+  EXPECT_EQ(ASCIIToUTF16(text), label_0->text() + label_1->text());
 }
 
 TEST_F(StyledLabelTest, CreateLinks) {
@@ -303,11 +294,7 @@ TEST_F(StyledLabelTest, StyledRangeCustomFontUnderlined) {
   styled()->Layout();
 
   ASSERT_EQ(2u, styled()->children().size());
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(1)->GetClassName());
-  EXPECT_EQ(
-      gfx::Font::UNDERLINE,
-      static_cast<Label*>(styled()->child_at(1))->font_list().GetFontStyle());
+  EXPECT_EQ(gfx::Font::UNDERLINE, LabelAt(1)->font_list().GetFontStyle());
 }
 
 TEST_F(StyledLabelTest, StyledRangeTextStyleBold) {
@@ -354,27 +341,17 @@ TEST_F(StyledLabelTest, StyledRangeTextStyleBold) {
   ASSERT_EQ(3u, styled()->children().size());
 
   // The bold text should be broken up into two parts.
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(0)->GetClassName());
-  EXPECT_EQ(
-      gfx::Font::Weight::BOLD,
-      static_cast<Label*>(styled()->child_at(0))->font_list().GetFontWeight());
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(1)->GetClassName());
-  EXPECT_EQ(
-      gfx::Font::Weight::BOLD,
-      static_cast<Label*>(styled()->child_at(1))->font_list().GetFontWeight());
-  ASSERT_EQ(std::string(Label::kViewClassName),
-            styled()->child_at(2)->GetClassName());
-  EXPECT_EQ(
-      gfx::Font::NORMAL,
-      static_cast<Label*>(styled()->child_at(2))->font_list().GetFontStyle());
+  const auto* label_0 = LabelAt(0);
+  const auto* label_1 = LabelAt(1);
+  const auto* label_2 = LabelAt(2);
+  EXPECT_EQ(gfx::Font::Weight::BOLD, label_0->font_list().GetFontWeight());
+  EXPECT_EQ(gfx::Font::Weight::BOLD, label_1->font_list().GetFontWeight());
+  EXPECT_EQ(gfx::Font::NORMAL, label_2->font_list().GetFontStyle());
 
   // The second bold part should start on a new line.
-  EXPECT_EQ(0, styled()->child_at(0)->x());
-  EXPECT_EQ(0, styled()->child_at(1)->x());
-  EXPECT_EQ(styled()->child_at(1)->bounds().right(),
-            styled()->child_at(2)->x());
+  EXPECT_EQ(0, label_0->x());
+  EXPECT_EQ(0, label_1->x());
+  EXPECT_EQ(label_1->bounds().right(), label_2->x());
 }
 
 TEST_F(StyledLabelTest, Color) {
@@ -413,12 +390,10 @@ TEST_F(StyledLabelTest, Color) {
   container->AddChildView(link);
   const SkColor kDefaultLinkColor = link->enabled_color();
 
-  EXPECT_EQ(SK_ColorBLUE,
-            static_cast<Label*>(styled()->child_at(0))->enabled_color());
+  EXPECT_EQ(SK_ColorBLUE, LabelAt(0)->enabled_color());
   EXPECT_EQ(kDefaultLinkColor,
-            static_cast<Label*>(styled()->child_at(1))->enabled_color());
-  EXPECT_EQ(kDefaultTextColor,
-            static_cast<Label*>(styled()->child_at(2))->enabled_color());
+            LabelAt(1, Link::kViewClassName)->enabled_color());
+  EXPECT_EQ(kDefaultTextColor, LabelAt(2)->enabled_color());
 
   // Test adjusted color readability.
   styled()->SetDisplayedOnBackgroundColor(SK_ColorBLACK);
@@ -427,8 +402,7 @@ TEST_F(StyledLabelTest, Color) {
 
   const SkColor kAdjustedTextColor = label->enabled_color();
   EXPECT_NE(kAdjustedTextColor, kDefaultTextColor);
-  EXPECT_EQ(kAdjustedTextColor,
-            static_cast<Label*>(styled()->child_at(2))->enabled_color());
+  EXPECT_EQ(kAdjustedTextColor, LabelAt(2)->enabled_color());
 
   widget->CloseNow();
 }
@@ -503,7 +477,7 @@ TEST_F(StyledLabelTest, SetTextContextAndDefaultStyle) {
 
   styled()->Layout();
   ASSERT_EQ(1u, styled()->children().size());
-  Label* sublabel = static_cast<Label*>(styled()->child_at(0));
+  Label* sublabel = LabelAt(0);
   EXPECT_EQ(style::CONTEXT_DIALOG_TITLE, sublabel->text_context());
 
   EXPECT_NE(SK_ColorBLACK, label.enabled_color());  // Sanity check,
@@ -756,12 +730,13 @@ TEST_F(StyledLabelTest, ViewsCenteredWithLinkAndCustomView) {
   int height = styled()->GetPreferredSize().height();
 
   ASSERT_EQ(3u, styled()->children().size());
-  EXPECT_EQ((height - styled()->child_at(0)->bounds().height()) / 2,
-            styled()->child_at(0)->bounds().y());
-  EXPECT_EQ((height - styled()->child_at(1)->bounds().height()) / 2,
-            styled()->child_at(1)->bounds().y());
-  EXPECT_EQ((height - styled()->child_at(2)->bounds().height()) / 2,
-            styled()->child_at(2)->bounds().y());
+  const auto is_centered = [this, height](int index) {
+    const auto* child = styled()->child_at(index);
+    return child->bounds().y() == ((height - child->bounds().height()) / 2);
+  };
+  EXPECT_TRUE(is_centered(0));
+  EXPECT_TRUE(is_centered(1));
+  EXPECT_TRUE(is_centered(2));
 }
 
 }  // namespace views

ui/views/debug_utils.cc

@@ -17,11 +17,9 @@
 namespace views {
 namespace {
 void PrintViewHierarchyImp(const View* view,
-                           int indent,
+                           size_t indent,
                            std::ostringstream* out) {
-  int ind = indent;
-  while (ind-- > 0)
-    *out << ' ';
+  *out << std::string(indent, ' ');
   *out << view->GetClassName();
   *out << ' ';
   *out << view->id();
@@ -37,11 +35,9 @@ void PrintViewHierarchyImp(const View* view,
 }
 
 void PrintFocusHierarchyImp(const View* view,
-                            int indent,
+                            size_t indent,
                             std::ostringstream* out) {
-  int ind = indent;
-  while (ind-- > 0)
-    *out << ' ';
+  *out << std::string(indent, ' ');
   *out << view->GetClassName();
   *out << ' ';
   *out << view->id();

ui/views/examples/layout_example_base.cc

@@ -63,7 +63,7 @@ LayoutExampleBase::ChildPanel::ChildPanel(LayoutExampleBase* example,
                                           const gfx::Size& preferred_size)
     : example_(example), preferred_size_(preferred_size) {
   SetBorder(CreateSolidBorder(1, SK_ColorGRAY));
-  for (unsigned i = 0; i < sizeof(margin_) / sizeof(margin_[0]); ++i)
+  for (size_t i = 0; i < base::size(margin_); ++i)
     margin_[i] = CreateTextfield();
   flex_ = CreateTextfield();
   flex_->SetText(base::ASCIIToUTF16(""));
@@ -86,7 +86,7 @@ void LayoutExampleBase::ChildPanel::Layout() {
   const int kSpacing = 2;
   if (selected_) {
     gfx::Rect client = GetContentsBounds();
-    for (unsigned i = 0; i < sizeof(margin_) / sizeof(margin_[0]); ++i) {
+    for (size_t i = 0; i < base::size(margin_); ++i) {
       gfx::Point pos;
       Textfield* textfield = margin_[i];
       switch (i) {
@@ -266,8 +266,7 @@ void LayoutExampleBase::CreateExampleView(View* container) {
   add_button_->SizeToPreferredSize();
   control_panel_->AddChildView(add_button_);
   horizontal_pos += add_button_->width() + kLayoutExampleVerticalSpacing;
-  for (unsigned i = 0;
-       i < sizeof(child_panel_size_) / sizeof(child_panel_size_[0]); ++i) {
+  for (size_t i = 0; i < base::size(child_panel_size_); ++i) {
     child_panel_size_[i] =
         CreateRawTextfield(vertical_pos, true, &horizontal_pos);
     child_panel_size_[i]->SetY(

ui/views/layout/flex_layout_unittest.cc

@@ -1850,18 +1850,18 @@ class NestedFlexLayoutTest : public FlexLayoutTest {
  public:
   void AddChildren(int num_children) {
     for (int i = 0; i < num_children; ++i) {
-      View* v = new View;
+      auto v = std::make_unique<View>();
       FlexLayout* layout = v->SetLayoutManager(std::make_unique<FlexLayout>());
-      host_->AddChildView(v);
-      children_.push_back(v);
+      children_.push_back(v.get());
       layouts_.push_back(layout);
+      host_->AddChildView(std::move(v));
     }
   }
 
   View* AddGrandchild(
       int child_index,
       const gfx::Size& preferred,
-      const base::Optional<gfx::Size>& minimum = base::Optional<gfx::Size>()) {
+      const base::Optional<gfx::Size>& minimum = base::nullopt) {
     return AddChild(children_[child_index - 1], preferred, minimum);
   }
 
@@ -1877,7 +1877,7 @@ class NestedFlexLayoutTest : public FlexLayoutTest {
 
  private:
   std::vector<FlexLayout*> layouts_;
-  std::vector<View*> children_;
+  View::Views children_;
 };
 
 TEST_F(NestedFlexLayoutTest, Layout_OppositeOrientation) {

ui/views/layout/grid_layout_unittest.cc

@@ -567,44 +567,37 @@ TEST_F(GridLayoutTest, FixedSize) {
 
   ColumnSet* set = layout()->AddColumnSet(0);
 
-  int column_count = 4;
-  int title_width = 100;
-  int row_count = 2;
-  int pref_width = 10;
-  int pref_height = 20;
-
-  for (int i = 0; i < column_count; ++i) {
-    set->AddColumn(GridLayout::CENTER,
-                   GridLayout::CENTER,
-                   0,
-                   GridLayout::FIXED,
-                   title_width,
-                   title_width);
+  constexpr size_t kRowCount = 2;
+  constexpr size_t kColumnCount = 4;
+  constexpr int kTitleWidth = 100;
+  constexpr int kPrefWidth = 10;
+  constexpr int kPrefHeight = 20;
+
+  for (size_t i = 0; i < kColumnCount; ++i) {
+    set->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0, GridLayout::FIXED,
+                   kTitleWidth, kTitleWidth);
   }
 
-  for (int row = 0; row < row_count; ++row) {
+  for (size_t row = 0; row < kRowCount; ++row) {
     layout()->StartRow(0, 0);
-    for (int col = 0; col < column_count; ++col) {
-      layout()->AddView(CreateSizedView(gfx::Size(pref_width, pref_height)));
-    }
+    for (size_t column = 0; column < kColumnCount; ++column)
+      layout()->AddView(CreateSizedView(gfx::Size(kPrefWidth, kPrefHeight)));
   }
 
   layout()->Layout(&host());
 
-  for (int i = 0; i < column_count; ++i) {
-    for (int row = 0; row < row_count; ++row) {
-      View* view = host().child_at(row * column_count + i);
+  for (size_t row = 0; row < kRowCount; ++row) {
+    for (size_t column = 0; column < kColumnCount; ++column) {
+      View* view = host().child_at(row * kColumnCount + column);
       ExpectViewBoundsEquals(
-          2 + title_width * i + (title_width - pref_width) / 2,
-          2 + pref_height * row,
-          pref_width,
-          pref_height, view);
+          2 + kTitleWidth * column + (kTitleWidth - kPrefWidth) / 2,
+          2 + kPrefHeight * row, kPrefWidth, kPrefHeight, view);
     }
   }
 
-  gfx::Size pref = GetPreferredSize();
-  EXPECT_EQ(gfx::Size(column_count * title_width + 4,
-                      row_count * pref_height + 4), pref);
+  EXPECT_EQ(
+      gfx::Size(kColumnCount * kTitleWidth + 4, kRowCount * kPrefHeight + 4),
+      GetPreferredSize());
 }
 
 TEST_F(GridLayoutTest, RowSpanWithPaddingRow) {

ui/views/view_unittest.cc

@@ -70,9 +70,9 @@ bool LayerIsAncestor(const ui::Layer* ancestor, const ui::Layer* layer) {
 
 // Convenience functions for walking a View tree.
 const views::View* FirstView(const views::View* view) {
-  const views::View* v = view;
-  while (!v->children().empty())
-    v = v->child_at(0);
+  const views::View* v;
+  for (v = view; !v->children().empty(); v = v->child_at(0)) {
+  }
   return v;
 }
 
@@ -4919,7 +4919,7 @@ class OrderableView : public View {
 
 TEST_F(ViewTest, ChildViewZOrderChanged) {
   const int kChildrenCount = 4;
-  std::unique_ptr<View> view(new OrderableView());
+  auto view = std::make_unique<OrderableView>();
   view->SetPaintToLayer();
   for (int i = 0; i < kChildrenCount; ++i)
     AddViewWithChildLayer(view.get());
@@ -4928,8 +4928,8 @@ TEST_F(ViewTest, ChildViewZOrderChanged) {
   EXPECT_EQ(kChildrenCount, static_cast<int>(layers.size()));
   EXPECT_EQ(kChildrenCount, static_cast<int>(children.size()));
   for (int i = 0; i < kChildrenCount; ++i) {
-    EXPECT_EQ(view->child_at(i)->layer(), layers[i]);
     EXPECT_EQ(view->child_at(i), children[i]);
+    EXPECT_EQ(view->child_at(i)->layer(), layers[i]);
   }
 
   // Raise one of the children in z-order and add another child to reorder.

ui/views/widget/widget_interactive_uitest.cc

@@ -802,9 +802,9 @@ TEST_F(WidgetTestInteractive, ViewFocusOnHWNDEnabledChanges) {
   Widget* widget = CreateTopLevelFramelessPlatformWidget();
   widget->SetContentsView(new View);
   for (int i = 0; i < 2; ++i) {
-    widget->GetContentsView()->AddChildView(new View);
-    widget->GetContentsView()->child_at(i)->SetFocusBehavior(
-        View::FocusBehavior::ALWAYS);
+    auto child = std::make_unique<View>();
+    child->SetFocusBehavior(View::FocusBehavior::ALWAYS);
+    widget->GetContentsView()->AddChildView(std::move(child));
   }
 
   widget->Show();