Skip to content

Commit

Permalink
fix adding to group that is deleted from the tab_menu_model
Browse files Browse the repository at this point in the history
The old way that target index was used to get the groupID for the tab
group to be added to was using the wrong source of truth. the fix is
to have a mapping of index to tabGroupID from when the model was
generated and then check against that mapping to check if the group
model still contains that ID.

(cherry picked from commit 8e276e7)

Bug: 1273397
Change-Id: I566505dc4267a1224de0dd2de95da8c12f79ed97
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3384389
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Commit-Queue: David Pennington <dpenning@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#962822}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3449665
Cr-Commit-Position: refs/branch-heads/4758@{#1123}
Cr-Branched-From: 4a2cf4b-refs/heads/main@{#950365}
  • Loading branch information
David Pennington authored and Chromium LUCI CQ committed Feb 9, 2022
1 parent 97342fd commit 2b608b9
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 5 deletions.
16 changes: 12 additions & 4 deletions chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.cc
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.h"

#include "base/containers/contains.h"
#include "base/metrics/user_metrics.h"
#include "base/metrics/user_metrics_action.h"
#include "chrome/app/vector_icons/vector_icons.h"
Expand Down Expand Up @@ -52,11 +53,15 @@ ExistingTabGroupSubMenuModel::ExistingTabGroupSubMenuModel(
kTabGroupIcon, tp.GetColor(color_id), kIconSize);
menu_item_infos.emplace_back(MenuItemInfo{displayed_title, image_model});
menu_item_infos.back().may_have_mnemonics = false;

menu_item_infos.back().target_index = static_cast<int>(i);
target_index_to_group_mapping_.emplace(i, group);
}
Build(IDS_TAB_CXMENU_SUBMENU_NEW_GROUP, menu_item_infos);
}

ExistingTabGroupSubMenuModel::~ExistingTabGroupSubMenuModel() = default;

std::vector<tab_groups::TabGroupId>
ExistingTabGroupSubMenuModel::GetOrderedTabGroupsInSubMenu() {
std::vector<tab_groups::TabGroupId> ordered_groups;
Expand Down Expand Up @@ -92,13 +97,16 @@ void ExistingTabGroupSubMenuModel::ExecuteNewCommand(int event_flags) {
void ExistingTabGroupSubMenuModel::ExecuteExistingCommand(int target_index) {
base::RecordAction(base::UserMetricsAction("TabContextMenu_NewTabInGroup"));

if (static_cast<size_t>(target_index) >=
model()->group_model()->ListTabGroups().size())
return;
if (!model()->ContainsIndex(GetContextIndex()))
return;

if (!base::Contains(target_index_to_group_mapping_, target_index) ||
!model()->group_model()->ContainsTabGroup(
target_index_to_group_mapping_.at(target_index)))
return;

model()->ExecuteAddToExistingGroupCommand(
GetContextIndex(), GetOrderedTabGroupsInSubMenu()[target_index]);
GetContextIndex(), target_index_to_group_mapping_.at(target_index));
}

// static
Expand Down
8 changes: 7 additions & 1 deletion chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.h
Expand Up @@ -23,7 +23,7 @@ class ExistingTabGroupSubMenuModel : public ExistingBaseSubMenuModel {
ExistingTabGroupSubMenuModel(const ExistingTabGroupSubMenuModel&) = delete;
ExistingTabGroupSubMenuModel& operator=(const ExistingTabGroupSubMenuModel&) =
delete;
~ExistingTabGroupSubMenuModel() override = default;
~ExistingTabGroupSubMenuModel() override;

// Whether the submenu should be shown in the provided context. True iff
// the submenu would show at least one group. Does not assume ownership of
Expand All @@ -47,6 +47,12 @@ class ExistingTabGroupSubMenuModel : public ExistingBaseSubMenuModel {
static bool ShouldShowGroup(TabStripModel* model,
int context_index,
tab_groups::TabGroupId group);

// mapping of the initial tab group to index in the menu model. this must
// be used in cases where the tab groups returned from
// GetOrderedTabGroupsInSubMenu changes after the menu has been opened but
// before the action is taken from the menumodel.
std::map<int, tab_groups::TabGroupId> target_index_to_group_mapping_;
};

#endif // CHROME_BROWSER_UI_TABS_EXISTING_TAB_GROUP_SUB_MENU_MODEL_H_

0 comments on commit 2b608b9

Please sign in to comment.