Order the groups listed in the tab context menu

Previously this was using TabGroupModel::ListTabGroups(), which pulls from a map: https://cs.chromium.org/chromium/src/chrome/browser/ui/tabs/tab_group_model.cc?l=50&rcl=1158206625559148ac779e86e5d53da50ebf39b4

The consistently ordered experience is better for users, even if it's slightly more expensive.

Tested as part of crrev.com/c/1990423, which adds color bubbles that make it more obvious when groups are out of order with the tab strip.

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

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.cc

@@ -24,7 +24,7 @@ ExistingTabGroupSubMenuModel::ExistingTabGroupSubMenuModel(TabStripModel* model,
 void ExistingTabGroupSubMenuModel::Build() {
   // Start command ids after the parent menu's ids to avoid collisions.
   int group_index = kFirstCommandIndex;
-  for (tab_groups::TabGroupId group : model_->group_model()->ListTabGroups()) {
+  for (tab_groups::TabGroupId group : GetOrderedTabGroups()) {
     if (ShouldShowGroup(model_, context_index_, group))
       AddItem(group_index,
               model_->group_model()->GetTabGroup(group)->GetDisplayedTitle());
@@ -32,6 +32,20 @@ void ExistingTabGroupSubMenuModel::Build() {
   }
 }
 
+std::vector<tab_groups::TabGroupId>
+ExistingTabGroupSubMenuModel::GetOrderedTabGroups() {
+  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)
+      ordered_groups.push_back(new_group.value());
+    current_group = new_group;
+  }
+  return ordered_groups;
+}
+
 bool ExistingTabGroupSubMenuModel::IsCommandIdChecked(int command_id) const {
   return false;
 }
@@ -43,11 +57,9 @@ bool ExistingTabGroupSubMenuModel::IsCommandIdEnabled(int command_id) const {
 void ExistingTabGroupSubMenuModel::ExecuteCommand(int command_id,
                                                   int event_flags) {
   const int group_index = command_id - kFirstCommandIndex;
-  // TODO(https://crbug.com/922736): If a group has been deleted, |group_index|
-  // may refer to a different group than it did when the menu was created.
   DCHECK_LT(size_t{group_index}, model_->group_model()->ListTabGroups().size());
-  model_->ExecuteAddToExistingGroupCommand(
-      context_index_, model_->group_model()->ListTabGroups()[group_index]);
+  model_->ExecuteAddToExistingGroupCommand(context_index_,
+                                           GetOrderedTabGroups()[group_index]);
 }
 
 // static

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.h

@@ -36,6 +36,12 @@ class ExistingTabGroupSubMenuModel : public ui::SimpleMenuModel,
  private:
   void Build();
 
+  // 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();
+
   // Unowned; |model_| must outlive this instance.
   TabStripModel* model_;