Fix bug in "Add to Existing Tab Group"

This regression was caused by this refactor:
https://chromium-review.googlesource.com/c/chromium/src/+/2324113

The original code for building the submenu would skip ID values for
groups that would not appear in the submenu. (The Build() method would
call GetOrderedTabGroups then skip over groups that should not show in
the submenu.) Skipping ID values was not done after the refactor since
the subclasses would call the base classes with a vector of MenuItemInfo
to populate the submenu.

Bug: 1112719, 1112721, 1108062
Change-Id: I4a3490d646c553d1c60dd548fa13f53df9183561
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2337547Reviewed-by: Charlene Yan <cyan@chromium.org>
Reviewed-by: Connie Wan <connily@chromium.org>
Commit-Queue: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795293}

chrome/browser/ui/tabs/existing_base_sub_menu_model.cc

@@ -29,6 +29,9 @@ bool ExistingBaseSubMenuModel::IsCommandIdEnabled(int command_id) const {
   return true;
 }
 
+constexpr int ExistingBaseSubMenuModel::kMinExistingWindowCommandId;
+constexpr int ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId;
+
 void ExistingBaseSubMenuModel::ExecuteCommand(int command_id, int event_flags) {
   if (IsNewCommand(command_id)) {
     ExecuteNewCommand(event_flags);

chrome/browser/ui/tabs/existing_base_sub_menu_model.h

@@ -38,6 +38,10 @@ class ExistingBaseSubMenuModel : public ui::SimpleMenuModel,
   bool IsCommandIdEnabled(int command_id) const override;
   void ExecuteCommand(int command_id, int event_flags) final;
 
+  // Command IDs for various submenus.
+  static constexpr int kMinExistingWindowCommandId = 1001;
+  static constexpr int kMinExistingTabGroupCommandId = 2001;
+
   ~ExistingBaseSubMenuModel() override;
 
  protected:
@@ -75,11 +79,7 @@ class ExistingBaseSubMenuModel : public ui::SimpleMenuModel,
   virtual void ExecuteExistingCommand(int command_index);
 
   // Maximum number of entries for a submenu.
-  static const int max_size = 200;
-
-  // Command IDs for various submenus.
-  static const int kMinExistingWindowCommandId = 1001;
-  static const int kMinExistingTabGroupCommandId = 2001;
+  static constexpr int max_size = 200;
 
   ui::SimpleMenuModel::Delegate* parent_delegate() const {
     return parent_delegate_;

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.cc

@@ -36,33 +36,33 @@ ExistingTabGroupSubMenuModel::ExistingTabGroupSubMenuModel(
   constexpr int kIconSize = 14;
   std::vector<MenuItemInfo> menu_item_infos;
 
-  for (tab_groups::TabGroupId group : GetOrderedTabGroups()) {
-    if (ShouldShowGroup(model, context_index, group)) {
-      const TabGroup* tab_group = model->group_model()->GetTabGroup(group);
-      const base::string16 group_title = tab_group->visual_data()->title();
-      const base::string16 displayed_title =
-          group_title.empty() ? tab_group->GetContentString() : group_title;
-      const int color_id =
-          GetTabGroupContextMenuColorId(tab_group->visual_data()->color());
-      // TODO (kylixrd): Investigate passing in color_id in order to color the
-      // icon using the ColorProvider.
-      ui::ImageModel image_model = ui::ImageModel::FromVectorIcon(
-          kTabGroupIcon, tp.GetColor(color_id), kIconSize);
-      menu_item_infos.emplace_back(MenuItemInfo{displayed_title, image_model});
-    }
+  for (tab_groups::TabGroupId group : GetOrderedTabGroupsInSubMenu()) {
+    const TabGroup* tab_group = model->group_model()->GetTabGroup(group);
+    const base::string16 group_title = tab_group->visual_data()->title();
+    const base::string16 displayed_title =
+        group_title.empty() ? tab_group->GetContentString() : group_title;
+    const int color_id =
+        GetTabGroupContextMenuColorId(tab_group->visual_data()->color());
+    // TODO (kylixrd): Investigate passing in color_id in order to color the
+    // icon using the ColorProvider.
+    ui::ImageModel image_model = ui::ImageModel::FromVectorIcon(
+        kTabGroupIcon, tp.GetColor(color_id), kIconSize);
+    menu_item_infos.emplace_back(MenuItemInfo{displayed_title, image_model});
   }
   Build(IDS_TAB_CXMENU_SUBMENU_NEW_GROUP, menu_item_infos);
 }
 
 std::vector<tab_groups::TabGroupId>
-ExistingTabGroupSubMenuModel::GetOrderedTabGroups() {
+ExistingTabGroupSubMenuModel::GetOrderedTabGroupsInSubMenu() {
   std::vector<tab_groups::TabGroupId> ordered_groups;
   base::Optional<tab_groups::TabGroupId> current_group = base::nullopt;
   for (int i = 0; i < model()->count(); ++i) {
     base::Optional<tab_groups::TabGroupId> new_group =
         model()->GetTabGroupForTab(i);
-    if (new_group.has_value() && new_group != current_group)
+    if (new_group.has_value() && new_group != current_group &&
+        ShouldShowGroup(model(), context_index(), new_group.value())) {
       ordered_groups.push_back(new_group.value());
+    }
     current_group = new_group;
   }
   return ordered_groups;
@@ -89,7 +89,7 @@ void ExistingTabGroupSubMenuModel::ExecuteExistingCommand(int command_index) {
             model()->group_model()->ListTabGroups().size());
   base::RecordAction(base::UserMetricsAction("TabContextMenu_NewTabInGroup"));
   model()->ExecuteAddToExistingGroupCommand(
-      context_index(), GetOrderedTabGroups()[command_index]);
+      context_index(), GetOrderedTabGroupsInSubMenu()[command_index]);
 }
 
 // static

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.h

@@ -35,11 +35,12 @@ class ExistingTabGroupSubMenuModel : public ExistingBaseSubMenuModel {
   void ExecuteNewCommand(int event_flags) override;
   void ExecuteExistingCommand(int command_index) override;
 
-  // Returns the group ids in the order that they appear in the tab strip model,
-  // so that the user sees an ordered display. Only needed for creating items
-  // and executing commands, which must be in order. Otherwise, ListTabGroups()
-  // is cheaper and sufficient for determining visibility and size of the menu.
-  std::vector<tab_groups::TabGroupId> GetOrderedTabGroups();
+  // Returns the group ids that appear in the submenu in the order that they
+  // appear in the tab strip model, so that the user sees an ordered display.
+  // Only needed for creating items and executing commands, which must be in
+  // order. Otherwise, ListTabGroups() is cheaper and sufficient for determining
+  // visibility and size of the menu.
+  std::vector<tab_groups::TabGroupId> GetOrderedTabGroupsInSubMenu();
 
   // Whether the submenu should contain the group |group|. True iff at least
   // one tab that would be affected by the command is not in |group|.

chrome/browser/ui/tabs/tab_menu_model_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/tabs/existing_base_sub_menu_model.h"
 #include "chrome/browser/ui/ui_features.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/menu_model_test.h"
@@ -61,6 +62,47 @@ TEST_F(TabMenuModelTest, AddToExistingGroupSubmenu) {
       menu.GetIndexOfCommandId(TabStripModel::CommandAddToExistingGroup);
   ui::MenuModel* submenu = menu.GetSubmenuModelAt(submenu_index);
 
+  EXPECT_TRUE(submenu->HasIcons());
   EXPECT_EQ(submenu->GetItemCount(), 5);
+  EXPECT_EQ(submenu->GetCommandIdAt(0),
+            ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId);
+  EXPECT_EQ(submenu->GetTypeAt(1), ui::MenuModel::TYPE_SEPARATOR);
+  EXPECT_EQ(submenu->GetCommandIdAt(2),
+            ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId + 1);
+  EXPECT_EQ(submenu->GetCommandIdAt(3),
+            ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId + 2);
+  EXPECT_EQ(submenu->GetCommandIdAt(4),
+            ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId + 3);
+}
+
+TEST_F(TabMenuModelTest, AddToExistingGroupSubmenu_DoesNotIncludeCurrentGroup) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kTabGroups);
+
+  chrome::NewTab(browser());
+  chrome::NewTab(browser());
+  chrome::NewTab(browser());
+  chrome::NewTab(browser());
+
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+
+  tab_strip_model->AddToNewGroup({0});
+  tab_strip_model->AddToNewGroup({1});
+  tab_strip_model->AddToNewGroup({2});
+
+  TabMenuModel menu(&delegate_, tab_strip_model, 1);
+
+  int submenu_index =
+      menu.GetIndexOfCommandId(TabStripModel::CommandAddToExistingGroup);
+  ui::MenuModel* submenu = menu.GetSubmenuModelAt(submenu_index);
+
   EXPECT_TRUE(submenu->HasIcons());
+  EXPECT_EQ(submenu->GetItemCount(), 4);
+  EXPECT_EQ(submenu->GetCommandIdAt(0),
+            ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId);
+  EXPECT_EQ(submenu->GetTypeAt(1), ui::MenuModel::TYPE_SEPARATOR);
+  EXPECT_EQ(submenu->GetCommandIdAt(2),
+            ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId + 1);
+  EXPECT_EQ(submenu->GetCommandIdAt(3),
+            ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId + 2);
 }