[UI] Always create image view in Menu Item View

and use visibility checks instead of nullness checks.

Bug: 1044038
Change-Id: Ib3283e85559f03810d88e8ab8774214d4931c7f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2236432
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776641}

chrome/browser/ui/views/menu_view_drag_and_drop_test.cc

@@ -215,8 +215,8 @@ void MenuViewDragAndDropTest::DoTestWithMenuOpen() {
   ASSERT_TRUE(submenu->IsShowing());
   ASSERT_EQ(3u, submenu->GetMenuItems().size());
   const views::View* first_view = submenu->GetMenuItemAt(0);
-  ASSERT_EQ(1u, first_view->children().size());
-  const views::View* child_view = first_view->children().front();
+  ASSERT_EQ(2u, first_view->children().size());
+  const views::View* child_view = first_view->children()[1];
   EXPECT_EQ(child_view, target_view_);
 
   // The menu is showing, so it has a widget we can observe now.

ui/views/BUILD.gn

@@ -478,6 +478,7 @@ jumbo_component("views") {
   deps = [
     "//base:i18n",
     "//base/third_party/dynamic_annotations",
+    "//base/util/ranges:ranges",
     "//cc/paint",
     "//mojo/public/cpp/bindings",
     "//skia",

ui/views/controls/menu/menu_controller_unittest.cc

@@ -1145,11 +1145,11 @@ TEST_F(MenuControllerTest, SelectChildButtonView) {
   AddButtonMenuItems();
   View* buttons_view = menu_item()->GetSubmenu()->children()[4];
   ASSERT_NE(nullptr, buttons_view);
-  Button* button1 = Button::AsButton(buttons_view->children()[0]);
+  Button* button1 = Button::AsButton(buttons_view->children()[1]);
   ASSERT_NE(nullptr, button1);
-  Button* button2 = Button::AsButton(buttons_view->children()[1]);
+  Button* button2 = Button::AsButton(buttons_view->children()[2]);
   ASSERT_NE(nullptr, button2);
-  Button* button3 = Button::AsButton(buttons_view->children()[2]);
+  Button* button3 = Button::AsButton(buttons_view->children()[3]);
   ASSERT_NE(nullptr, button2);
 
   // Handle searching for 'f'; should find "Four".
@@ -1220,11 +1220,11 @@ TEST_F(MenuControllerTest, DeleteChildButtonView) {
 
   View* buttons_view = menu_item()->GetSubmenu()->children()[4];
   ASSERT_NE(nullptr, buttons_view);
-  Button* button1 = Button::AsButton(buttons_view->children()[0]);
+  Button* button1 = Button::AsButton(buttons_view->children()[1]);
   ASSERT_NE(nullptr, button1);
-  Button* button2 = Button::AsButton(buttons_view->children()[1]);
+  Button* button2 = Button::AsButton(buttons_view->children()[2]);
   ASSERT_NE(nullptr, button2);
-  Button* button3 = Button::AsButton(buttons_view->children()[2]);
+  Button* button3 = Button::AsButton(buttons_view->children()[3]);
   ASSERT_NE(nullptr, button2);
   EXPECT_FALSE(button1->IsHotTracked());
   EXPECT_FALSE(button2->IsHotTracked());
@@ -1262,11 +1262,11 @@ TEST_F(MenuControllerTest, ChildButtonHotTrackedWhenNested) {
 
   View* buttons_view = menu_item()->GetSubmenu()->children()[4];
   ASSERT_NE(nullptr, buttons_view);
-  Button* button1 = Button::AsButton(buttons_view->children()[0]);
+  Button* button1 = Button::AsButton(buttons_view->children()[1]);
   ASSERT_NE(nullptr, button1);
-  Button* button2 = Button::AsButton(buttons_view->children()[1]);
+  Button* button2 = Button::AsButton(buttons_view->children()[2]);
   ASSERT_NE(nullptr, button2);
-  Button* button3 = Button::AsButton(buttons_view->children()[2]);
+  Button* button3 = Button::AsButton(buttons_view->children()[3]);
   ASSERT_NE(nullptr, button2);
   EXPECT_FALSE(button1->IsHotTracked());
   EXPECT_FALSE(button2->IsHotTracked());
@@ -2369,9 +2369,9 @@ TEST_F(MenuControllerTest, SetSelectionIndices_Buttons) {
   MenuItemView* const item3 = menu_item()->GetSubmenu()->GetMenuItemAt(2);
   MenuItemView* const item4 = menu_item()->GetSubmenu()->GetMenuItemAt(3);
   MenuItemView* const item5 = menu_item()->GetSubmenu()->GetMenuItemAt(4);
-  Button* const button1 = Button::AsButton(item5->children()[0]);
-  Button* const button2 = Button::AsButton(item5->children()[1]);
-  Button* const button3 = Button::AsButton(item5->children()[2]);
+  Button* const button1 = Button::AsButton(item5->children()[1]);
+  Button* const button2 = Button::AsButton(item5->children()[2]);
+  Button* const button3 = Button::AsButton(item5->children()[3]);
   OpenMenu(menu_item());
 
   ui::AXNodeData data;
@@ -2411,11 +2411,11 @@ TEST_F(MenuControllerTest, SetSelectionIndices_Buttons_SkipHiddenAndDisabled) {
   MenuItemView* const item3 = menu_item()->GetSubmenu()->GetMenuItemAt(2);
   MenuItemView* const item4 = menu_item()->GetSubmenu()->GetMenuItemAt(3);
   MenuItemView* const item5 = menu_item()->GetSubmenu()->GetMenuItemAt(4);
-  Button* const button1 = Button::AsButton(item5->children()[0]);
+  Button* const button1 = Button::AsButton(item5->children()[1]);
   button1->SetEnabled(false);
-  Button* const button2 = Button::AsButton(item5->children()[1]);
+  Button* const button2 = Button::AsButton(item5->children()[2]);
   button2->SetVisible(false);
-  Button* const button3 = Button::AsButton(item5->children()[2]);
+  Button* const button3 = Button::AsButton(item5->children()[3]);
   OpenMenu(menu_item());
 
   ui::AXNodeData data;

ui/views/controls/menu/menu_item_view.cc

@@ -15,6 +15,7 @@
 #include "base/i18n/case_conversion.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/util/ranges/algorithm.h"
 #include "ui/accessibility/ax_action_data.h"
 #include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_node_data.h"
@@ -161,7 +162,7 @@ void MenuItemView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   if (IsContainer()) {
     // The first child is taking over, just use its accessible name instead of
     // |title_|.
-    View* child = children().front();
+    View* child = GetFirstVisibleChild();
     ui::AXNodeData child_node_data;
     child->GetAccessibleNodeData(&child_node_data);
     item_text =
@@ -435,13 +436,12 @@ void MenuItemView::SetIcon(const gfx::ImageSkia& icon) {
   vector_icon_.clear();
 
   if (icon.isNull()) {
-    SetIconView(nullptr);
+    SetIconViewVisibilityAndInvalidate(false);
     return;
   }
 
-  auto icon_view = std::make_unique<ImageView>();
-  icon_view->SetImage(&icon);
-  SetIconView(std::move(icon_view));
+  icon_view_->SetImage(icon);
+  SetIconViewVisibilityAndInvalidate(true);
 }
 
 void MenuItemView::SetIcon(const ui::ThemedVectorIcon& icon) {
@@ -452,8 +452,7 @@ void MenuItemView::UpdateIconViewFromVectorIconAndTheme() {
   if (vector_icon_.empty())
     return;
 
-  if (!icon_view_)
-    SetIconView(std::make_unique<ImageView>());
+  SetIconViewVisibilityAndInvalidate(true);
 
   const bool use_touchable_layout =
       GetMenuController() && GetMenuController()->use_touchable_layout();
@@ -461,14 +460,8 @@ void MenuItemView::UpdateIconViewFromVectorIconAndTheme() {
   icon_view_->SetImage(vector_icon_.GetImageSkia(GetNativeTheme(), icon_size));
 }
 
-void MenuItemView::SetIconView(std::unique_ptr<ImageView> icon_view) {
-  if (icon_view_) {
-    RemoveChildViewT(icon_view_);
-    icon_view_ = nullptr;
-  }
-
-  if (icon_view)
-    icon_view_ = AddChildView(std::move(icon_view));
+void MenuItemView::SetIconViewVisibilityAndInvalidate(bool is_visible) {
+  icon_view_->SetVisible(is_visible);
 
   InvalidateLayout();
   SchedulePaint();
@@ -490,8 +483,9 @@ int MenuItemView::GetHeightForWidth(int width) const {
     return GetPreferredSize().height();
 
   const gfx::Insets margins = GetContainerMargins();
-  int height = children().front()->GetHeightForWidth(width - margins.width());
-  if (!icon_view_ && GetRootMenuItem()->has_icons())
+  int height =
+      GetFirstVisibleChild()->GetHeightForWidth(width - margins.width());
+  if (!icon_view_->GetVisible() && GetRootMenuItem()->has_icons())
     height = std::max(height, MenuConfig::instance().check_height);
 
   height += margins.height();
@@ -613,7 +607,8 @@ void MenuItemView::ChildrenChanged() {
 }
 
 void MenuItemView::Layout() {
-  if (children().empty())
+  View* first_visible_child = GetFirstVisibleChild();
+  if (!first_visible_child)
     return;
 
   if (IsContainer()) {
@@ -623,7 +618,7 @@ void MenuItemView::Layout() {
     // bounds, less the margins.
     gfx::Rect bounds = GetContentsBounds();
     bounds.Inset(GetContainerMargins());
-    children().front()->SetBoundsRect(bounds);
+    first_visible_child->SetBoundsRect(bounds);
   } else {
     // Child views are laid out right aligned and given the full height. To
     // right align start with the last view and progress to the first.
@@ -643,7 +638,7 @@ void MenuItemView::Layout() {
     }
     // Position |icon_view|.
     const MenuConfig& config = MenuConfig::instance();
-    if (icon_view_) {
+    {
       icon_view_->SizeToPreferredSize();
       gfx::Size size = icon_view_->GetPreferredSize();
       int x = config.item_horizontal_padding + left_icon_margin_ +
@@ -776,6 +771,9 @@ void MenuItemView::Init(MenuItemView* parent,
   SetID(kMenuItemViewID);
   has_icons_ = false;
 
+  icon_view_ = AddChildView(std::make_unique<ImageView>());
+  icon_view_->SetVisible(false);
+
   if (type_ == Type::kCheckbox || type_ == Type::kRadio) {
     radio_check_image_view_ = AddChildView(std::make_unique<ImageView>());
     bool show_check_radio_icon =
@@ -1130,14 +1128,15 @@ int MenuItemView::GetBottomMargin() const {
 }
 
 gfx::Size MenuItemView::GetChildPreferredSize() const {
-  if (children().empty())
+  const View* first_visible_child = GetFirstVisibleChild();
+  if (!first_visible_child)
     return gfx::Size();
 
   if (IsContainer())
-    return children().front()->GetPreferredSize();
+    return first_visible_child->GetPreferredSize();
 
   const auto add_width = [this](int width, const View* child) {
-    if (child == icon_view_ || child == radio_check_image_view_ ||
+    if ((child == icon_view_) || child == radio_check_image_view_ ||
         child == submenu_arrow_image_view_ || child == vertical_separator_)
       return width;
     if (width)
@@ -1149,7 +1148,8 @@ gfx::Size MenuItemView::GetChildPreferredSize() const {
 
   // If there is no icon view it returns a height of 0 to indicate that
   // we should use the title height instead.
-  const int height = icon_view_ ? icon_view_->GetPreferredSize().height() : 0;
+  const int height =
+      icon_view_->GetVisible() ? icon_view_->GetPreferredSize().height() : 0;
 
   return gfx::Size(width, height);
 }
@@ -1173,7 +1173,7 @@ MenuItemView::MenuItemDimensions MenuItemView::CalculateDimensions() const {
 
     dimensions.standard_width = menu_config.touchable_menu_width;
 
-    if (icon_view_) {
+    if (icon_view_->GetVisible()) {
       dimensions.height = icon_view_->GetPreferredSize().height() +
                           2 * menu_config.vertical_touchable_menu_item_padding;
     }
@@ -1187,7 +1187,7 @@ MenuItemView::MenuItemDimensions MenuItemView::CalculateDimensions() const {
   dimensions.height = child_size.height();
   // Adjust item content height if menu has both items with and without icons.
   // This way all menu items will have the same height.
-  if (!icon_view_ && GetRootMenuItem()->has_icons()) {
+  if (!icon_view_->GetVisible() && GetRootMenuItem()->has_icons()) {
     dimensions.height =
         std::max(dimensions.height, MenuConfig::instance().check_height);
   }
@@ -1263,14 +1263,14 @@ int MenuItemView::GetLabelStartForThisItem() const {
 
   // Touchable items with icons do not respect |label_start_|.
   if (GetMenuController() && GetMenuController()->use_touchable_layout() &&
-      icon_view_) {
+      icon_view_->GetVisible()) {
     return 2 * config.touchable_item_horizontal_padding + icon_view_->width();
   }
 
   int label_start = label_start_ + left_icon_margin_ + right_icon_margin_;
   if ((config.icons_in_label || type_ == Type::kCheckbox ||
        type_ == Type::kRadio) &&
-      icon_view_) {
+      icon_view_->GetVisible()) {
     label_start += icon_view_->size().width() + config.item_horizontal_padding;
   }
 
@@ -1306,7 +1306,7 @@ gfx::Insets MenuItemView::GetContainerMargins() const {
   // Use the child's preferred margins but take the standard top and bottom
   // margins as minimums.
   const gfx::Insets* margins_prop =
-      children().front()->GetProperty(views::kMarginsKey);
+      GetFirstVisibleChild()->GetProperty(views::kMarginsKey);
   gfx::Insets margins = margins_prop ? *margins_prop : gfx::Insets();
   margins.set_top(std::max(margins.top(), GetTopMargin()));
   margins.set_bottom(std::max(margins.bottom(), GetBottomMargin()));
@@ -1314,8 +1314,8 @@ gfx::Insets MenuItemView::GetContainerMargins() const {
 }
 
 int MenuItemView::NonIconChildViewsCount() const {
-  return int{children().size()} - (icon_view_ ? 1 : 0) -
-         (radio_check_image_view_ ? 1 : 0) -
+  // Always subtract 1 for |icon_view_|.
+  return int{children().size()} - 1 - (radio_check_image_view_ ? 1 : 0) -
          (submenu_arrow_image_view_ ? 1 : 0) - (vertical_separator_ ? 1 : 0);
 }
 
@@ -1334,7 +1334,8 @@ int MenuItemView::GetMaxIconViewWidth() const {
     }
     if (item->HasSubmenu())
       return item->GetMaxIconViewWidth();
-    return (item->icon_view_ && !MenuConfig::instance().icons_in_label)
+    return (item->icon_view_->GetVisible() &&
+            !MenuConfig::instance().icons_in_label)
                ? item->icon_view_->GetPreferredSize().width()
                : 0;
   };
@@ -1354,6 +1355,11 @@ bool MenuItemView::HasChecksOrRadioButtons() const {
       [](const auto* item) { return item->HasChecksOrRadioButtons(); });
 }
 
+const View* MenuItemView::GetFirstVisibleChild() const {
+  const auto it = util::ranges::find(children(), true, &View::GetVisible);
+  return (it == children().cend()) ? nullptr : *it;
+}
+
 BEGIN_METADATA(MenuItemView)
 METADATA_PARENT_CLASS(View)
 END_METADATA()

ui/views/controls/menu/menu_item_view.h

@@ -258,9 +258,8 @@ class VIEWS_EXPORT MenuItemView : public View {
   // the included color.
   void SetIcon(const ui::ThemedVectorIcon& icon);
 
-  // Sets the view used to render the icon. This clobbers any icon set via
-  // SetIcon(). MenuItemView takes ownership of |icon_view|.
-  void SetIconView(std::unique_ptr<ImageView> icon_view);
+  // Sets the visibility of the view used to render the icon.
+  void SetIconViewVisibilityAndInvalidate(bool is_visible);
 
   void UpdateIconViewFromVectorIconAndTheme();
 
@@ -481,6 +480,14 @@ class VIEWS_EXPORT MenuItemView : public View {
   // Returns true if the menu has items with a checkbox or a radio button.
   bool HasChecksOrRadioButtons() const;
 
+  // Returns null if all children are hidden. For example, where there is only a
+  // hidden |icon_view_|.
+  const View* GetFirstVisibleChild() const;
+  View* GetFirstVisibleChild() {
+    return const_cast<View*>(
+        static_cast<const MenuItemView*>(this)->GetFirstVisibleChild());
+  }
+
   void invalidate_dimensions() { dimensions_.height = 0; }
   bool is_dimensions_valid() const { return dimensions_.height > 0; }
 
@@ -533,8 +540,9 @@ class VIEWS_EXPORT MenuItemView : public View {
   // Set if menu has icons or icon_views (applies to root menu item only).
   bool has_icons_ = false;
 
-  // Pointer to a view with a menu icon.
-  ImageView* icon_view_ = nullptr;
+  // Pointer to a view with a menu icon. Visible only when the menu item
+  // contains an icon.
+  ImageView* icon_view_;
 
   // The tooltip to show on hover for this menu item.
   base::string16 tooltip_;