From b6e180056b9782acde1e771e176d0317bc346375 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Mon, 2 Dec 2024 17:21:24 +0900 Subject: [PATCH] Fixed detached window has unnecessary tabs fix https://github.com/brave/brave-browser/issues/42588 When last tab is closed in the last tabbed browser, SessionService updates related flag but it's not the actual last tab because another new tab is created after that tab is closed when "Close window when closing last tab" is turned off. --- browser/ui/brave_browser_browsertest.cc | 51 +++++++++++++++++++ .../browser/sessions/session_service.cc | 19 +++++++ ...-browser-sessions-session_service.cc.patch | 12 +++++ 3 files changed, 82 insertions(+) create mode 100644 chromium_src/chrome/browser/sessions/session_service.cc create mode 100644 patches/chrome-browser-sessions-session_service.cc.patch diff --git a/browser/ui/brave_browser_browsertest.cc b/browser/ui/brave_browser_browsertest.cc index fb7ef9c90d2..b53e15514a9 100644 --- a/browser/ui/brave_browser_browsertest.cc +++ b/browser/ui/brave_browser_browsertest.cc @@ -10,7 +10,9 @@ #include "brave/components/constants/pref_names.h" #include "chrome/browser/devtools/devtools_window_testing.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser_commands.h" #include "chrome/browser/ui/browser_finder.h" +#include "chrome/browser/ui/browser_list.h" #include "chrome/browser/ui/browser_tabstrip.h" #include "chrome/browser/ui/startup/launch_mode_recorder.h" #include "chrome/browser/ui/startup/startup_browser_creator.h" @@ -161,3 +163,52 @@ IN_PROC_BROWSER_TEST_F(BraveBrowserBrowserTest, EXPECT_TRUE( base::test::RunUntil([] { return chrome::GetTotalBrowserCount() == 1; })); } + +IN_PROC_BROWSER_TEST_F(BraveBrowserBrowserTest, + CreateAnotherWindowWithExistingTab) { + ASSERT_TRUE(embedded_test_server()->Start()); + browser()->profile()->GetPrefs()->SetBoolean(kEnableClosingLastTab, false); + TabStripModel* tab_strip = browser()->tab_strip_model(); + + auto page_url = embedded_test_server()->GetURL("/empty.html"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), page_url)); + + ASSERT_EQ(1, tab_strip->count()); + EXPECT_EQ(page_url, + tab_strip->GetWebContentsAt(0)->GetURL().possibly_invalid_spec()); + + // Close the last tab. + tab_strip->GetActiveWebContents()->Close(); + ASSERT_EQ(0, tab_strip->count()); + + // Wait till another new tab is opened. + EXPECT_TRUE( + base::test::RunUntil([tab_strip] { return tab_strip->count() == 1; })); + EXPECT_EQ(browser()->GetNewTabURL(), + tab_strip->GetWebContentsAt(0)->GetURL().possibly_invalid_spec()); + + ui_test_utils::NavigateToURLWithDisposition( + browser(), GURL("https://www.brave.com/"), + WindowOpenDisposition::NEW_FOREGROUND_TAB, + ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP); + ASSERT_EQ(2, tab_strip->count()); + + // Create another browser with existing tab. + chrome::MoveTabsToNewWindow(browser(), {1}); + ASSERT_EQ(1, tab_strip->count()); + + // Get new browser. + Browser* new_browser = nullptr; + for (Browser* b : *BrowserList::GetInstance()) { + if (b != browser()) { + new_browser = b; + break; + } + } + ASSERT_TRUE(new_browser); + base::RunLoop().RunUntilIdle(); + + // Check new browser by detaching a tab from another window has + // one tab. + EXPECT_EQ(1, new_browser->tab_strip_model()->count()); +} diff --git a/chromium_src/chrome/browser/sessions/session_service.cc b/chromium_src/chrome/browser/sessions/session_service.cc new file mode 100644 index 00000000000..a0fdc1a99c2 --- /dev/null +++ b/chromium_src/chrome/browser/sessions/session_service.cc @@ -0,0 +1,19 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/constants/pref_names.h" + +// Prevent detached tab has unnecessary tab restore steps. +// When last window's last tab is closed, |has_open_trackable_browsers_| +// becomes false. If we turn off "Close window when closing last tab" option, +// another new tab is created after last tab is closed. So, it's not the last +// actually. It could make ShouldRestore() give true when creating +// new browser by detaching a tab. +#define BRAVE_SESSION_SERVICE_TAB_CLOSED \ + if (profile()->GetPrefs()->GetBoolean(kEnableClosingLastTab)) + +#include "src/chrome/browser/sessions/session_service.cc" + +#undef BRAVE_SESSION_SERVICE_TAB_CLOSED diff --git a/patches/chrome-browser-sessions-session_service.cc.patch b/patches/chrome-browser-sessions-session_service.cc.patch new file mode 100644 index 00000000000..d35f6c1d11c --- /dev/null +++ b/patches/chrome-browser-sessions-session_service.cc.patch @@ -0,0 +1,12 @@ +diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc +index 69e538baf515892624b9505a00f19268d6d6e5c7..4b120111dd92f971809e1ded4ce51a46f8ea722a 100644 +--- a/chrome/browser/sessions/session_service.cc ++++ b/chrome/browser/sessions/session_service.cc +@@ -371,6 +371,7 @@ void SessionService::TabClosed(SessionID window_id, SessionID tab_id) { + window_id) == window_closing_ids_.end()) && + IsOnlyOneTabLeft()) { + // This is the last tab in the last tabbed browser. ++ BRAVE_SESSION_SERVICE_TAB_CLOSED + has_open_trackable_browsers_ = false; + } + }