[Tab Groups] Streamline tab context menu patterns.

Updating the context menu for Tab Groups to show a submenu with the
entry "Add to Group" if another group exists and the submenu to had
"New group" as the first item followed by a separator and then the
other existing groups.
If there are no other groups, a submenu will not appear and the menu
item will simply be "Add to new group".

Bug: 1064674
Change-Id: I88b470171d4f44e6969201fea7e991d5cd2905c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2133269
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Connie Wan <connily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756446}

chrome/app/generated_resources.grd

@@ -6343,11 +6343,14 @@ the Bookmarks menu.">
         <message name="IDS_TAB_CXMENU_SOUND_UNMUTE_SITE" desc="The label of the tab context menu item for unmuting one or more sites. NOTE: If having no explicit # makes the phrasing awkward, feel free to add a # as necessary. [ICU Syntax]">
           {NUM_TABS, plural, =1 {Unmute site} other {Unmute sites}}
         </message>
+        <message name="IDS_TAB_CXMENU_ADD_TAB_TO_GROUP" desc="The label of the tab context menu submenu for adding one or more tabs to either a new group or an existing tab group.">
+          Add to group
+        </message>
         <message name="IDS_TAB_CXMENU_ADD_TAB_TO_NEW_GROUP" desc="The label of the tab context menu item for creating a new tab group and adding one or more tabs to it.">
           Add to new group
         </message>
-        <message name="IDS_TAB_CXMENU_ADD_TAB_TO_EXISTING_GROUP" desc="The label of the tab context menu submenu for adding one or more tabs to an existing tab group.">
-          Add to existing group
+        <message name="IDS_TAB_CXMENU_SUBMENU_NEW_GROUP" desc="The label of the tab context menu sub-menu item for adding one more tabs to a new group.">
+          New group
         </message>
         <message name="IDS_TAB_CXMENU_REMOVE_TAB_FROM_GROUP" desc="The label of the tab context menu item for removing one or more tabs from the groups that contain them.">
           Remove from group
@@ -6403,11 +6406,14 @@ the Bookmarks menu.">
         <message name="IDS_TAB_CXMENU_SOUND_UNMUTE_SITE" desc="In Title Case: The label of the tab context menu item for unmuting one or more sites. NOTE: If having no explicit # makes the phrasing awkward, feel free to add a # as necessary. [ICU Syntax]">
           {NUM_TABS, plural, =1 {Unmute Site} other {Unmute Sites}}
         </message>
+        <message name="IDS_TAB_CXMENU_ADD_TAB_TO_GROUP" desc="In Title Case: The label of the tab context menu submenu for adding one or more tabs to either a new group or an existing tab group.">
+          Add to Group
+        </message>
         <message name="IDS_TAB_CXMENU_ADD_TAB_TO_NEW_GROUP" desc="In Title Case: The label of the tab context menu item for creating a new tab group and adding one or more tabs to it.">
           Add to New Group
         </message>
-        <message name="IDS_TAB_CXMENU_ADD_TAB_TO_EXISTING_GROUP" desc="In Title Case: The label of the tab context menu submenu for adding one or more tabs to an existing tab group.">
-          Add to Existing Group
+        <message name="IDS_TAB_CXMENU_SUBMENU_NEW_GROUP" desc="In Title Case: The label of the tab context menu sub-menu item for adding one more tabs to a new group.">
+          New Group
         </message>
         <message name="IDS_TAB_CXMENU_REMOVE_TAB_FROM_GROUP" desc="In Title Case: The label of the tab context menu item for removing one or more tabs from the groups that contain them.">
           Remove From Group

chrome/app/generated_resources_grd/IDS_TAB_CXMENU_ADD_TAB_TO_GROUP.png.sha1

+e8a529c4df5aae933c85f6dd8e1ec492cc1a0187
\ No newline at end of file

chrome/app/generated_resources_grd/IDS_TAB_CXMENU_SUBMENU_NEW_GROUP.png.sha1

+79eee8548fe3deb62ab62fa27b3c020d5ec26978
\ No newline at end of file

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/ui/tabs/tab_group_model.h"
 #include "chrome/browser/ui/tabs/tab_group_theme.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/tab_groups/tab_group_color.h"
 #include "components/tab_groups/tab_group_id.h"
 #include "components/tab_groups/tab_group_visual_data.h"
@@ -25,11 +26,14 @@
 constexpr int kFirstCommandIndex =
     TabStripModel::ContextMenuCommand::CommandLast + 1;
 
-ExistingTabGroupSubMenuModel::ExistingTabGroupSubMenuModel(TabStripModel* model,
-                                                           int context_index)
-    : SimpleMenuModel(this) {
-  model_ = model;
-  context_index_ = context_index;
+ExistingTabGroupSubMenuModel::ExistingTabGroupSubMenuModel(
+    ui::SimpleMenuModel::Delegate* parent_delegate,
+    TabStripModel* model,
+    int context_index)
+    : SimpleMenuModel(this),
+      parent_delegate_(parent_delegate),
+      model_(model),
+      context_index_(context_index) {
   Build();
 }
 
@@ -37,6 +41,9 @@ void ExistingTabGroupSubMenuModel::Build() {
   // Start command ids after the parent menu's ids to avoid collisions.
   int group_index = kFirstCommandIndex;
   const auto& tp = ThemeService::GetThemeProviderForProfile(model_->profile());
+  AddItemWithStringId(TabStripModel::CommandAddToNewGroup,
+                      IDS_TAB_CXMENU_SUBMENU_NEW_GROUP);
+  AddSeparator(ui::NORMAL_SEPARATOR);
   for (tab_groups::TabGroupId group : GetOrderedTabGroups()) {
     if (ShouldShowGroup(model_, context_index_, group)) {
       const TabGroup* tab_group = model_->group_model()->GetTabGroup(group);
@@ -78,6 +85,11 @@ bool ExistingTabGroupSubMenuModel::IsCommandIdEnabled(int command_id) const {
 
 void ExistingTabGroupSubMenuModel::ExecuteCommand(int command_id,
                                                   int event_flags) {
+  if (command_id == TabStripModel::CommandAddToNewGroup) {
+    parent_delegate_->ExecuteCommand(TabStripModel::CommandAddToNewGroup,
+                                     event_flags);
+    return;
+  }
   const int group_index = command_id - kFirstCommandIndex;
   DCHECK_LT(size_t{group_index}, model_->group_model()->ListTabGroups().size());
   base::RecordAction(base::UserMetricsAction("TabContextMenu_NewTabInGroup"));

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.h

@@ -19,7 +19,9 @@ class TabGroupId;
 class ExistingTabGroupSubMenuModel : public ui::SimpleMenuModel,
                                      ui::SimpleMenuModel::Delegate {
  public:
-  ExistingTabGroupSubMenuModel(TabStripModel* model, int context_index);
+  ExistingTabGroupSubMenuModel(ui::SimpleMenuModel::Delegate* parent_delegate,
+                               TabStripModel* model,
+                               int context_index);
   ~ExistingTabGroupSubMenuModel() override = default;
 
   bool IsCommandIdChecked(int command_id) const override;
@@ -42,6 +44,8 @@ class ExistingTabGroupSubMenuModel : public ui::SimpleMenuModel,
   // is cheaper and sufficient for determining visibility and size of the menu.
   std::vector<tab_groups::TabGroupId> GetOrderedTabGroups();
 
+  ui::SimpleMenuModel::Delegate* parent_delegate_;
+
   // Unowned; |model_| must outlive this instance.
   TabStripModel* model_;
 

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.h"
+
+#include "chrome/browser/ui/tabs/tab_group_model.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "url/gurl.h"
+
+namespace {
+
+class ExistingTabGroupSubMenuModelTest : public BrowserWithTestWindowTest {
+ public:
+  ExistingTabGroupSubMenuModelTest() : BrowserWithTestWindowTest() {}
+};
+
+// Ensure that add to group submenu only appears when there is another group to
+// move the tab into.
+TEST_F(ExistingTabGroupSubMenuModelTest, ShouldShowSubmenu) {
+  AddTab(browser(), GURL("chrome://newtab"));
+  AddTab(browser(), GURL("chrome://newtab"));
+
+  TabStripModel* model = browser()->tab_strip_model();
+  ASSERT_EQ(model->group_model()->ListTabGroups().size(), 0U);
+  model->AddToNewGroup({0});
+  ASSERT_EQ(model->group_model()->ListTabGroups().size(), 1U);
+  ASSERT_TRUE(model->GetTabGroupForTab(0).has_value());
+  ASSERT_FALSE(model->GetTabGroupForTab(1).has_value());
+  ASSERT_EQ(model->count(), 2);
+
+  EXPECT_FALSE(ExistingTabGroupSubMenuModel::ShouldShowSubmenu(model, 0));
+  EXPECT_TRUE(ExistingTabGroupSubMenuModel::ShouldShowSubmenu(model, 1));
+}
+
+// Validate that the submenu has the correct items.
+TEST_F(ExistingTabGroupSubMenuModelTest, BuildSubmenuItems) {
+  AddTab(browser(), GURL("chrome://newtab"));
+  AddTab(browser(), GURL("chrome://newtab"));
+  AddTab(browser(), GURL("chrome://newtab"));
+
+  TabStripModel* model = browser()->tab_strip_model();
+  model->AddToNewGroup({0});
+  model->AddToNewGroup({1});
+  ASSERT_EQ(model->group_model()->ListTabGroups().size(), 2U);
+  ASSERT_TRUE(model->GetTabGroupForTab(0).has_value());
+  ASSERT_TRUE(model->GetTabGroupForTab(1).has_value());
+  ASSERT_FALSE(model->GetTabGroupForTab(2).has_value());
+  ASSERT_EQ(model->count(), 3);
+
+  ExistingTabGroupSubMenuModel menu1(nullptr, model, 0);
+  EXPECT_EQ(3, menu1.GetItemCount());
+
+  ExistingTabGroupSubMenuModel menu2(nullptr, model, 1);
+  EXPECT_EQ(3, menu2.GetItemCount());
+
+  ExistingTabGroupSubMenuModel menu3(nullptr, model, 2);
+  EXPECT_EQ(4, menu3.GetItemCount());
+}
+
+}  // namespace

chrome/browser/ui/tabs/tab_menu_model.cc

@@ -41,16 +41,17 @@ void TabMenuModel::Build(TabStripModel* tab_strip, int index) {
   AddItemWithStringId(TabStripModel::CommandNewTabToRight,
                       IDS_TAB_CXMENU_NEWTABTORIGHT);
   if (base::FeatureList::IsEnabled(features::kTabGroups)) {
-    AddItemWithStringId(TabStripModel::CommandAddToNewGroup,
-                        IDS_TAB_CXMENU_ADD_TAB_TO_NEW_GROUP);
-
-    // Create submenu with existing groups
     if (ExistingTabGroupSubMenuModel::ShouldShowSubmenu(tab_strip, index)) {
+      // Create submenu with existing groups
       add_to_existing_group_submenu_ =
-          std::make_unique<ExistingTabGroupSubMenuModel>(tab_strip, index);
+          std::make_unique<ExistingTabGroupSubMenuModel>(delegate(), tab_strip,
+                                                         index);
       AddSubMenuWithStringId(TabStripModel::CommandAddToExistingGroup,
-                             IDS_TAB_CXMENU_ADD_TAB_TO_EXISTING_GROUP,
+                             IDS_TAB_CXMENU_ADD_TAB_TO_GROUP,
                              add_to_existing_group_submenu_.get());
+    } else {
+      AddItemWithStringId(TabStripModel::CommandAddToNewGroup,
+                          IDS_TAB_CXMENU_ADD_TAB_TO_NEW_GROUP);
     }
 
     for (size_t index = 0; index < affected_indices.size(); index++) {
@@ -62,11 +63,7 @@ void TabMenuModel::Build(TabStripModel* tab_strip, int index) {
     }
   }
 
-  if (!ExistingWindowSubMenuModel::ShouldShowSubmenu(tab_strip->profile())) {
-    AddItem(TabStripModel::CommandMoveTabsToNewWindow,
-            l10n_util::GetPluralStringFUTF16(
-                IDS_TAB_CXMENU_MOVE_TABS_TO_NEW_WINDOW, num_affected_tabs));
-  } else {
+  if (ExistingWindowSubMenuModel::ShouldShowSubmenu(tab_strip->profile())) {
     // Create submenu with existing windows
     add_to_existing_window_submenu_ =
         std::make_unique<ExistingWindowSubMenuModel>(delegate(), tab_strip,
@@ -75,6 +72,10 @@ void TabMenuModel::Build(TabStripModel* tab_strip, int index) {
                l10n_util::GetPluralStringFUTF16(
                    IDS_TAB_CXMENU_MOVETOANOTHERWINDOW, num_affected_tabs),
                add_to_existing_window_submenu_.get());
+  } else {
+    AddItem(TabStripModel::CommandMoveTabsToNewWindow,
+            l10n_util::GetPluralStringFUTF16(
+                IDS_TAB_CXMENU_MOVE_TABS_TO_NEW_WINDOW, num_affected_tabs));
   }
 
   AddSeparator(ui::NORMAL_SEPARATOR);

chrome/browser/ui/tabs/tab_menu_model_unittest.cc

@@ -61,6 +61,6 @@ TEST_F(TabMenuModelTest, AddToExistingGroupSubmenu) {
       menu.GetIndexOfCommandId(TabStripModel::CommandAddToExistingGroup);
   ui::MenuModel* submenu = menu.GetSubmenuModelAt(submenu_index);
 
-  EXPECT_EQ(submenu->GetItemCount(), 3);
+  EXPECT_EQ(submenu->GetItemCount(), 5);
   EXPECT_TRUE(submenu->HasIcons());
 }

chrome/test/BUILD.gn

@@ -4104,6 +4104,7 @@ test("unit_tests") {
       "../browser/ui/tab_contents/chrome_web_contents_view_handle_drop_unittest.cc",
       "../browser/ui/tab_contents/tab_contents_iterator_unittest.cc",
       "../browser/ui/tab_sharing/tab_sharing_infobar_delegate_unittest.cc",
+      "../browser/ui/tabs/existing_tab_group_sub_menu_model_unittest.cc",
       "../browser/ui/tabs/existing_window_sub_menu_model_unittest.cc",
       "../browser/ui/tabs/pinned_tab_codec_unittest.cc",
       "../browser/ui/tabs/pinned_tab_service_unittest.cc",