Skip to content

Commit

Permalink
Revert "Add 'move tab to existing window' submenu for tabbed apps"
Browse files Browse the repository at this point in the history
This reverts commit 3539766.

Reason for revert:
WebAppTabStripBrowserTest.MoveTabsToExistingWindow is broken:
https://ci.chromium.org/p/chromium/builders/ci/Mac12%20Tests

Original change's description:
> Add 'move tab to existing window' submenu for tabbed apps
>
> The menu will only show other windows that belong to the same app.
>
> Bug: 1469955
> Change-Id: I167202a8aae52a008e5004fc245554e0558ea156
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4778024
> Commit-Queue: Louise Brett <loubrett@google.com>
> Reviewed-by: Dana Fried <dfried@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1187593}

Bug: 1469955
Change-Id: I4f89fff26e101371d6855641d88383a1d3d8ccb0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4809415
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Nancy Wang <nancylingwang@chromium.org>
Owners-Override: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Louise Brett <loubrett@google.com>
Cr-Commit-Position: refs/heads/main@{#1187638}
  • Loading branch information
Nancy Wang authored and Chromium LUCI CQ committed Aug 24, 2023
1 parent 5e28085 commit eb88264
Show file tree
Hide file tree
Showing 11 changed files with 20 additions and 89 deletions.
16 changes: 5 additions & 11 deletions chrome/browser/ui/browser_tab_menu_model_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/browser_tab_strip_model_delegate.h"
#include "chrome/browser/ui/web_applications/app_browser_controller.h"

namespace chrome {

Expand All @@ -16,21 +15,16 @@ BrowserTabMenuModelDelegate::BrowserTabMenuModelDelegate(Browser* browser)

BrowserTabMenuModelDelegate::~BrowserTabMenuModelDelegate() = default;

std::vector<Browser*> BrowserTabMenuModelDelegate::GetOtherBrowserWindows(
bool is_app) {
std::vector<Browser*>
BrowserTabMenuModelDelegate::GetOtherTabbedBrowserWindows() {
std::vector<Browser*> browsers;

for (Browser* browser : BrowserList::GetInstance()->OrderedByActivation()) {
// We can only move into a tabbed view of the same profile, and not the same
// window we're currently in.
if (browser != browser_ && browser->profile() == browser_->profile()) {
if (is_app && browser->is_type_app() &&
browser->app_controller()->app_id() ==
browser_->app_controller()->app_id()) {
browsers.push_back(browser);
} else if (!is_app && browser->is_type_normal()) {
browsers.push_back(browser);
}
if (browser != browser_ && browser->is_type_normal() &&
browser->profile() == browser_->profile()) {
browsers.push_back(browser);
}
}
return browsers;
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/browser_tab_menu_model_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class BrowserTabMenuModelDelegate : public TabMenuModelDelegate {

private:
// TabMenuModelDelegate:
std::vector<Browser*> GetOtherBrowserWindows(bool is_app) override;
std::vector<Browser*> GetOtherTabbedBrowserWindows() override;

const raw_ptr<Browser, DanglingUntriaged> browser_;
};
Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/ui/browser_tab_strip_model_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,8 @@ void BrowserTabStripModelDelegate::DuplicateContentsAt(int index) {
void BrowserTabStripModelDelegate::MoveToExistingWindow(
const std::vector<int>& indices,
int browser_index) {
std::vector<Browser*> existing_browsers =
browser_->tab_menu_model_delegate()->GetOtherBrowserWindows(
web_app::AppBrowserController::IsWebApp(browser_));
auto existing_browsers =
browser_->tab_menu_model_delegate()->GetOtherTabbedBrowserWindows();
size_t existing_browser_count = existing_browsers.size();
if (static_cast<size_t>(browser_index) < existing_browser_count &&
existing_browsers[browser_index]) {
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ ExistingTabGroupSubMenuModel::ExistingTabGroupSubMenuModel(
// For each window, append the tab groups to the end of the menu items.
if (tab_menu_model_delegate_) {
for (Browser* browser :
tab_menu_model_delegate_->GetOtherBrowserWindows(/*is_app=*/false)) {
tab_menu_model_delegate_->GetOtherTabbedBrowserWindows()) {
if (browser->tab_strip_model() == model)
continue;
const std::vector<MenuItemInfo> retrieved_menu_item_infos =
Expand Down Expand Up @@ -159,7 +159,7 @@ bool ExistingTabGroupSubMenuModel::ShouldShowSubmenu(
// Look at tab groups in all other windows
if (tab_menu_model_delegate) {
for (Browser* browser :
tab_menu_model_delegate->GetOtherBrowserWindows(/*is_app=*/false)) {
tab_menu_model_delegate->GetOtherTabbedBrowserWindows()) {
TabGroupModel* browser_group_model =
browser->tab_strip_model()->group_model();
if (!browser_group_model)
Expand Down Expand Up @@ -201,7 +201,7 @@ void ExistingTabGroupSubMenuModel::ExecuteExistingCommand(size_t target_index) {
// Find the index of the browser with the group we are looking for.
absl::optional<size_t> browser_index;
std::vector<Browser*> browsers =
tab_menu_model_delegate_->GetOtherBrowserWindows(/*is_app=*/false);
tab_menu_model_delegate_->GetOtherTabbedBrowserWindows();
for (size_t i = 0; i < browsers.size(); ++i) {
TabStripModel* potential_model = browsers[i]->tab_strip_model();
if (potential_model && potential_model != model() &&
Expand Down
10 changes: 1 addition & 9 deletions chrome/browser/ui/tabs/existing_window_sub_menu_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/tabs/tab_menu_model_delegate.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/tabs/tab_strip_model_delegate.h"
#include "chrome/grit/generated_resources.h"
#include "ui/base/accelerators/accelerator.h"

Expand Down Expand Up @@ -54,8 +53,7 @@ ExistingWindowSubMenuModel::ExistingWindowSubMenuModel(
TabStripModel::CommandMoveTabsToNewWindow) {
Build(IDS_TAB_CXMENU_MOVETOANOTHERNEWWINDOW,
BuildMenuItemInfoVectorForBrowsers(
tab_menu_model_delegate->GetOtherBrowserWindows(
model->delegate()->IsForWebApp())));
tab_menu_model_delegate->GetOtherTabbedBrowserWindows()));
}

ExistingWindowSubMenuModel::~ExistingWindowSubMenuModel() = default;
Expand Down Expand Up @@ -91,12 +89,6 @@ bool ExistingWindowSubMenuModel::ShouldShowSubmenu(Profile* profile) {
return chrome::GetTabbedBrowserCount(profile) > 1;
}

bool ExistingWindowSubMenuModel::ShouldShowSubmenuForApp(
TabMenuModelDelegate* tab_menu_model_delegate) {
return tab_menu_model_delegate->GetOtherBrowserWindows(/*is_app=*/true)
.size() >= 1;
}

// static:
base::PassKey<ExistingWindowSubMenuModel>
ExistingWindowSubMenuModel::GetPassKey() {
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/ui/tabs/existing_window_sub_menu_model.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ class ExistingWindowSubMenuModel : public ExistingBaseSubMenuModel {
// the submenu would show at least one window. Does not assume ownership of
// |model|; |model| must outlive this instance.
static bool ShouldShowSubmenu(Profile* profile);
static bool ShouldShowSubmenuForApp(
TabMenuModelDelegate* tab_menu_model_delegate);

protected:
// Retrieves a base::Passkey which can be used to construct an instance of
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_menu_model_delegate.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/tabs/tab_strip_model_delegate.h"
#include "chrome/grit/generated_resources.h"
#include "chromeos/ui/wm/desks/desks_helper.h"
#include "ui/aura/window.h"
Expand Down Expand Up @@ -65,9 +64,8 @@ ExistingWindowSubMenuModelChromeOS::ExistingWindowSubMenuModelChromeOS(
context_index) {
// If we shouldn't group by desk, ExistingWindowSubMenuModel's ctor has
// already built the menu.
std::vector<Browser*> tabbed_browser_windows =
tab_menu_model_delegate->GetOtherBrowserWindows(
model->delegate()->IsForWebApp());
const std::vector<Browser*> tabbed_browser_windows =
tab_menu_model_delegate->GetOtherTabbedBrowserWindows();
if (!ShouldGroupByDesk(GetDesksHelper(tabbed_browser_windows))) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,7 @@ TEST_F(ExistingWindowSubMenuModelTest, BuildSubmenuGroupedByDesks) {
browser_7.get(), browser_5.get(), browser_4.get(),
browser_2.get(), browser_3.get(), browser_6.get()};
const auto& mru_ordered_windows =
browser()->tab_menu_model_delegate()->GetOtherBrowserWindows(
/*is_app=*/false);
browser()->tab_menu_model_delegate()->GetOtherTabbedBrowserWindows();
ASSERT_EQ(6u, mru_ordered_windows.size());
ASSERT_EQ(mru_ordered_windows, kExpectedMRUOrder);

Expand Down Expand Up @@ -445,8 +444,7 @@ TEST_F(ExistingWindowSubMenuModelTest, EnsureGroupedByDesksCommands) {
const std::vector<Browser*> kExpectedMRUOrder{
browser_4.get(), browser_2.get(), browser_3.get(), browser_5.get()};
const auto& mru_ordered_windows =
browser()->tab_menu_model_delegate()->GetOtherBrowserWindows(
/*is_app=*/false);
browser()->tab_menu_model_delegate()->GetOtherTabbedBrowserWindows();
ASSERT_EQ(4u, mru_ordered_windows.size());
ASSERT_EQ(mru_ordered_windows, kExpectedMRUOrder);

Expand Down
17 changes: 3 additions & 14 deletions chrome/browser/ui/tabs/tab_menu_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -56,20 +56,9 @@ void TabMenuModel::BuildForWebApp(TabStripModel* tab_strip, int index) {
(!web_app::HasPinnedHomeTab(tab_strip) ||
*tab_strip->selection_model().selected_indices().begin() != 0)) {
int num_tabs = tab_strip->selection_model().selected_indices().size();
if (ExistingWindowSubMenuModel::ShouldShowSubmenuForApp(
tab_menu_model_delegate_)) {
// Create submenu with existing windows
add_to_existing_window_submenu_ = ExistingWindowSubMenuModel::Create(
delegate(), tab_menu_model_delegate_, tab_strip, index);
AddSubMenu(TabStripModel::CommandMoveToExistingWindow,
l10n_util::GetPluralStringFUTF16(
IDS_TAB_CXMENU_MOVETOANOTHERWINDOW, num_tabs),
add_to_existing_window_submenu_.get());
} else {
AddItem(TabStripModel::CommandMoveTabsToNewWindow,
l10n_util::GetPluralStringFUTF16(
IDS_TAB_CXMENU_MOVE_TABS_TO_NEW_WINDOW, num_tabs));
}
AddItem(TabStripModel::CommandMoveTabsToNewWindow,
l10n_util::GetPluralStringFUTF16(
IDS_TAB_CXMENU_MOVE_TABS_TO_NEW_WINDOW, num_tabs));
}

AddSeparator(ui::NORMAL_SEPARATOR);
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/tabs/tab_menu_model_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class TabMenuModelDelegate {
// Returns a list of other existing browser windows that can accept menu
// operations (i.e. Move tab to new window, Add tab to group) that are not the
// current browser this was called on.
virtual std::vector<Browser*> GetOtherBrowserWindows(bool is_app) = 0;
virtual std::vector<Browser*> GetOtherTabbedBrowserWindows() = 0;
};

#endif // CHROME_BROWSER_UI_TABS_TAB_MENU_MODEL_DELEGATE_H_
37 changes: 0 additions & 37 deletions chrome/browser/ui/views/web_apps/web_app_tab_strip_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/browser_tabstrip.h"
#include "chrome/browser/ui/tab_ui_helper.h"
#include "chrome/browser/ui/tabs/existing_window_sub_menu_model.h"
#include "chrome/browser/ui/tabs/tab_menu_model.h"
#include "chrome/browser/ui/unload_controller.h"
#include "chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
Expand Down Expand Up @@ -638,7 +636,6 @@ IN_PROC_BROWSER_TEST_F(WebAppTabStripBrowserTest, MoveTabsToNewWindow) {
Browser* new_browser = BrowserList::GetInstance()->GetLastActive();
EXPECT_NE(app_browser, new_browser);
EXPECT_TRUE(AppBrowserController::IsForWebApp(new_browser, app_id));
EXPECT_EQ(app_browser->tab_strip_model()->count(), 1);

// Check the new browser contains the moved tab and a pinned home tab.
EXPECT_EQ(new_browser->tab_strip_model()->count(), 2);
Expand All @@ -649,40 +646,6 @@ IN_PROC_BROWSER_TEST_F(WebAppTabStripBrowserTest, MoveTabsToNewWindow) {
EXPECT_EQ(new_browser->tab_strip_model()->active_index(), 1);
}

IN_PROC_BROWSER_TEST_F(WebAppTabStripBrowserTest, MoveTabsToExistingWindow) {
GURL start_url =
embedded_test_server()->GetURL("/web_apps/tab_strip_customizations.html");
AppId app_id = InstallTestWebApp(start_url);
Browser* app_browser = LaunchWebAppBrowser(app_id);
chrome::NewTab(app_browser);

// Open a second app browser window.
chrome::MoveTabsToNewWindow(app_browser, {1});
Browser* app_browser2 = BrowserList::GetInstance()->GetLastActive();

EXPECT_EQ(app_browser->tab_strip_model()->count(), 1);
EXPECT_EQ(app_browser2->tab_strip_model()->count(), 2);

// Test the "open in existing window" menu option.
TabMenuModel menu(nullptr, app_browser2->tab_menu_model_delegate(),
app_browser2->tab_strip_model(), 1);
size_t submenu_index =
menu.GetIndexOfCommandId(TabStripModel::CommandMoveToExistingWindow)
.value();
ExistingWindowSubMenuModel* submenu =
static_cast<ExistingWindowSubMenuModel*>(
menu.GetSubmenuModelAt(submenu_index));
EXPECT_EQ(submenu->GetItemCount(), 3u);
EXPECT_EQ(submenu->GetLabelAt(0), u"New window");
EXPECT_EQ(submenu->GetTypeAt(1), ui::MenuModel::TYPE_SEPARATOR);
EXPECT_EQ(submenu->GetLabelAt(2), u"Manifest with tab strip customizations");

submenu->ExecuteCommand(submenu->GetCommandIdAt(2), 0);

EXPECT_EQ(app_browser2->tab_strip_model()->count(), 1);
EXPECT_EQ(app_browser->tab_strip_model()->count(), 2);
}

IN_PROC_BROWSER_TEST_F(WebAppTabStripBrowserTest,
OnlyNavigateHomeTabIfDifferentUrl) {
GURL start_url =
Expand Down

0 comments on commit eb88264

Please sign in to comment.