0

Activate proper tab on restoring window from session

This fixes a regression introduced in
https://chromium-review.googlesource.com/c/chromium/src/+/5460410

TabRestoreServiceHelper is saving selected_tab_index in
TabRestoreServiceHelper::BrowserClosing() but the saved index wasn't
used later and TabRestoreServiceHelper was always activating the first
tab when the whole window was restored.

This CL adds a test TabRestoreTest.RestoreWindow_ActiveTabIndex that
verifies that the active tab is still active after closing and
restoring the window.

Bug: 344606398
Change-Id: I6c3262a465b8d902a4ba9a7213fdad6142a38723
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5594032
Commit-Queue: Tomasz Moniuszko <tmoniuszko@opera.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Darryl James <dljames@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1311219}
This commit is contained in:
Tomasz Moniuszko
2024-06-06 13:49:36 +00:00
committed by Chromium LUCI CQ
parent e0e3e40e81
commit dccb3c31b4
5 changed files with 63 additions and 15 deletions

@ -133,6 +133,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTabLoadingBrowserTest, RestoreTab) {
get_tab_titles(browser_to_restore->tab_strip_model()));
// Close and restore the browser; capturing the newly-restored browser.
const int active_tab_index =
browser_to_restore->tab_strip_model()->active_index();
CloseBrowserSynchronously(std::exchange(browser_to_restore, nullptr));
Browser* restored_browser = nullptr;
{
@ -145,7 +147,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTabLoadingBrowserTest, RestoreTab) {
EXPECT_EQ(kDesiredNumberOfTabs, restored_browser->tab_strip_model()->count())
<< ::testing::PrintToString(
get_tab_titles(restored_browser->tab_strip_model()));
EXPECT_EQ(0, restored_browser->tab_strip_model()->active_index());
EXPECT_EQ(active_tab_index,
restored_browser->tab_strip_model()->active_index());
// All tabs should be loaded by BackgroundTabLoadingPolicy.
int index = 0;
@ -180,6 +183,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTabLoadingBrowserTest,
kDesiredNumberOfTabs - browser_to_restore->tab_strip_model()->count());
EXPECT_EQ(kDesiredNumberOfTabs,
browser_to_restore->tab_strip_model()->count());
const int active_tab_index =
browser_to_restore->tab_strip_model()->active_index();
CloseBrowserSynchronously(browser_to_restore);
// Restore recently closed window.
@ -188,7 +193,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTabLoadingBrowserTest,
Browser* restored_browser = BrowserList::GetInstance()->get(1);
EXPECT_EQ(kDesiredNumberOfTabs, restored_browser->tab_strip_model()->count());
EXPECT_EQ(0, restored_browser->tab_strip_model()->active_index());
EXPECT_EQ(active_tab_index,
restored_browser->tab_strip_model()->active_index());
// These tabs should be loaded by BackgroundTabLoadingPolicy.
EnsureTabFinishedRestoring(

@ -447,12 +447,13 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreWindowAndTab) {
EXPECT_EQ(2u, active_browser_list_->size());
// Close the first browser.
const int active_tab_index = browser()->tab_strip_model()->active_index();
CloseBrowserSynchronously(browser());
EXPECT_EQ(1u, active_browser_list_->size());
// Restore the first window. The expected_tabstrip_index (second argument)
// indicates the expected active tab.
ASSERT_NO_FATAL_FAILURE(RestoreTab(1, 0));
ASSERT_NO_FATAL_FAILURE(RestoreTab(1, active_tab_index));
Browser* browser = GetBrowser(1);
EXPECT_EQ(starting_tab_count + 2, browser->tab_strip_model()->count());
@ -996,6 +997,7 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreWindow) {
ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
// Close the window.
const int active_tab_index = browser()->tab_strip_model()->active_index();
CloseBrowserSynchronously(browser());
EXPECT_EQ(window_count - 1, active_browser_list_->size());
@ -1008,7 +1010,7 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreWindow) {
EXPECT_EQ(initial_tab_count + 2, browser->tab_strip_model()->count());
EXPECT_TRUE(content::WaitForLoadStop(tab_added_waiter.Wait()));
EXPECT_EQ(0, browser->tab_strip_model()->active_index());
EXPECT_EQ(active_tab_index, browser->tab_strip_model()->active_index());
content::WebContents* restored_tab =
browser->tab_strip_model()->GetWebContentsAt(initial_tab_count + 1);
EnsureTabFinishedRestoring(restored_tab);
@ -1020,6 +1022,28 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreWindow) {
EXPECT_EQ(url1_, restored_tab->GetURL());
}
// Verifies that active tab index is the same as before closing.
IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreWindow_ActiveTabIndex) {
AddSomeTabs(browser(), 4);
// Create a second browser.
ui_test_utils::NavigateToURLWithDisposition(
browser(), GURL(chrome::kChromeUINewTabURL),
WindowOpenDisposition::NEW_WINDOW,
ui_test_utils::BROWSER_TEST_WAIT_FOR_BROWSER);
EXPECT_EQ(2u, active_browser_list_->size());
constexpr int kActiveTabIndex = 2;
browser()->tab_strip_model()->ActivateTabAt(kActiveTabIndex);
// Close the first browser.
CloseBrowserSynchronously(browser());
EXPECT_EQ(1u, active_browser_list_->size());
// Restore the first browser. Verify the active tab index.
ASSERT_NO_FATAL_FAILURE(RestoreTab(1, kActiveTabIndex));
}
// https://crbug.com/825305: Timeout flakiness on Mac10.13 Tests (dbg) and
// PASS/FAIL flakiness on Linux Chromium OS ASan LSan Tests (1) bot.
#if defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER) || \
@ -1217,6 +1241,7 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest,
const int tabs_count = 4;
AddSomeTabs(browser2, tabs_count - browser2->tab_strip_model()->count());
EXPECT_EQ(tabs_count, browser2->tab_strip_model()->count());
const int active_tab_index = browser2->tab_strip_model()->active_index();
CloseBrowserSynchronously(browser2);
// Limit the number of restored tabs that are loaded.
@ -1235,18 +1260,22 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest,
browser2 = GetBrowser(1);
EXPECT_EQ(tabs_count, browser2->tab_strip_model()->count());
EXPECT_EQ(0, browser2->tab_strip_model()->active_index());
EXPECT_EQ(active_tab_index, browser2->tab_strip_model()->active_index());
// These two tabs should be loaded by TabLoader.
EnsureTabFinishedRestoring(browser2->tab_strip_model()->GetWebContentsAt(0));
EnsureTabFinishedRestoring(browser2->tab_strip_model()->GetWebContentsAt(1));
EnsureTabFinishedRestoring(
browser2->tab_strip_model()->GetWebContentsAt(active_tab_index));
// The following isn't necessary but just to be sure there is no any async
// task that could have an impact on the expectations below.
content::RunAllPendingInMessageLoop();
// These tabs shouldn't want to be loaded.
for (int tab_idx = 2; tab_idx < tabs_count; ++tab_idx) {
for (int tab_idx = 1; tab_idx < tabs_count; ++tab_idx) {
if (tab_idx == active_tab_index) {
continue; // Active tab should be loaded.
}
auto* contents = browser2->tab_strip_model()->GetWebContentsAt(tab_idx);
EXPECT_FALSE(contents->IsLoading());
EXPECT_TRUE(contents->GetController().NeedsReload());
@ -1314,11 +1343,12 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreWindowWithName) {
EXPECT_EQ(2u, active_browser_list_->size());
// Close the first browser.
const int active_tab_index = browser()->tab_strip_model()->active_index();
CloseBrowserSynchronously(browser());
EXPECT_EQ(1u, active_browser_list_->size());
// Restore the first browser.
ASSERT_NO_FATAL_FAILURE(RestoreTab(1, 0));
ASSERT_NO_FATAL_FAILURE(RestoreTab(1, active_tab_index));
Browser* browser = GetBrowser(1);
EXPECT_EQ("foobar", browser->user_title());
}

@ -73,6 +73,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, RecentTabsMenuTabDisposition) {
EXPECT_EQ(2u, active_browser_list->size());
// Close the first browser.
const int active_tab_index = browser()->tab_strip_model()->active_index();
CloseBrowserSynchronously(browser());
EXPECT_EQ(1u, active_browser_list->size());
@ -108,8 +109,8 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, RecentTabsMenuTabDisposition) {
}
}
// Only the first tab should have visible disposition.
CheckVisbility(restored_browser->tab_strip_model(), 0);
// Previously active tab should have visible disposition.
CheckVisbility(restored_browser->tab_strip_model(), active_tab_index);
}
// Expect a selected restored tab to start loading synchronously.
@ -175,6 +176,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, DelegateRestoreTabDisposition) {
EXPECT_EQ(2u, active_browser_list->size());
// Close the first browser.
const int active_tab_index = browser()->tab_strip_model()->active_index();
CloseBrowserSynchronously(browser());
EXPECT_EQ(1u, active_browser_list->size());
@ -212,6 +214,6 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, DelegateRestoreTabDisposition) {
}
}
// Only the first tab should have visible disposition.
CheckVisbility(browser->tab_strip_model(), 0);
// Previously active tab should have visible disposition.
CheckVisbility(browser->tab_strip_model(), active_tab_index);
}

@ -289,6 +289,7 @@ IN_PROC_BROWSER_TEST_F(ThumbnailTabHelperInteractiveTest,
constexpr int kTabCount = 4;
AddSomeTabs(browser2, kTabCount - browser2->tab_strip_model()->count());
EXPECT_EQ(kTabCount, browser2->tab_strip_model()->count());
const int active_tab_index = browser2->tab_strip_model()->active_index();
CloseBrowserSynchronously(browser2);
// Set up the tab loader to ensure tabs are left unloaded.
@ -303,7 +304,7 @@ IN_PROC_BROWSER_TEST_F(ThumbnailTabHelperInteractiveTest,
browser2 = GetBrowser(1);
EXPECT_EQ(kTabCount, browser2->tab_strip_model()->count());
EXPECT_EQ(0, browser2->tab_strip_model()->active_index());
EXPECT_EQ(active_tab_index, browser2->tab_strip_model()->active_index());
// These tabs shouldn't want to be loaded.
for (int tab_idx = 1; tab_idx < kTabCount - 1; ++tab_idx) {

@ -622,10 +622,19 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
window.show_state, window.workspace, window.user_title,
window.extra_data);
CHECK(!window.tabs.empty());
const int selected_tab_index =
window.selected_tab_index >= 0 &&
window.selected_tab_index <
static_cast<int>(window.tabs.size())
? window.selected_tab_index
: 0;
const SessionID selected_tab_id = window.tabs[selected_tab_index]->id;
for (const auto& tab : window.tabs) {
const bool first_tab = window.tabs[0]->id == tab->id;
const bool select_tab = tab->id == selected_tab_id;
LiveTab* restored_tab = context->AddRestoredTab(
*tab.get(), context->GetTabCount(), first_tab);
*tab.get(), context->GetTabCount(), select_tab);
if (restored_tab) {
client_->OnTabRestored(