Save and restore tab groups
This implements basic session restore functionality for tab
groups. Currently, only whole browser session restore is supported;
restoring a window or a tab from the history menu won't restore
groups.
Bug: 905491
Change-Id: Ib6ad643c7efd75f468bb518fa5881b17a3d9f05f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1627947
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666500}
diff --git a/chrome/browser/sessions/session_restore.cc b/chrome/browser/sessions/session_restore.cc
index 6fc31f3..b858066 100644
--- a/chrome/browser/sessions/session_restore.cc
+++ b/chrome/browser/sessions/session_restore.cc
@@ -226,7 +226,7 @@
use_new_window ? 0 : browser->tab_strip_model()->active_index() + 1;
web_contents = chrome::AddRestoredTab(
browser, tab.navigations, tab_index, selected_index,
- tab.extension_app_id,
+ tab.extension_app_id, base::nullopt,
disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB, // selected
tab.pinned, true, base::TimeTicks(), nullptr, tab.user_agent_override,
true /* from_session_restore */);
@@ -531,6 +531,11 @@
latest_last_active_time = tab.last_active_time;
}
+ // TODO(crbug.com/930991): Check that tab groups are contiguous in |window|
+ // to ensure tabs will not be reordered when restoring. This is not possible
+ // yet due the ordering of TabStripModelObserver notifications in an edge
+ // case.
+
for (int i = 0; i < static_cast<int>(window.tabs.size()); ++i) {
const sessions::SessionTab& tab = *(window.tabs[i]);
@@ -585,16 +590,14 @@
WebContents* web_contents = chrome::AddRestoredTab(
browser, tab.navigations, tab_index, selected_index,
- tab.extension_app_id, is_selected_tab, tab.pinned, true,
+ tab.extension_app_id, tab.group, is_selected_tab, tab.pinned, true,
last_active_time, session_storage_namespace.get(),
tab.user_agent_override, true /* from_session_restore */);
-
- // RestoreTab can return nullptr if |tab| doesn't have valid data.
- if (!web_contents)
- return;
+ DCHECK(web_contents);
RestoredTab restored_tab(web_contents, is_selected_tab,
- tab.extension_app_id.empty(), tab.pinned);
+ tab.extension_app_id.empty(), tab.pinned,
+ tab.group);
created_contents->push_back(restored_tab);
// If this isn't the selected tab, there's nothing else to do.
diff --git a/chrome/browser/sessions/session_restore_browsertest.cc b/chrome/browser/sessions/session_restore_browsertest.cc
index 9a0b0a3..e50cb2cd0 100644
--- a/chrome/browser/sessions/session_restore_browsertest.cc
+++ b/chrome/browser/sessions/session_restore_browsertest.cc
@@ -9,6 +9,7 @@
#include "base/base_switches.h"
#include "base/command_line.h"
+#include "base/containers/span.h"
#include "base/files/file_path.h"
#include "base/files/scoped_temp_dir.h"
#include "base/macros.h"
@@ -39,6 +40,7 @@
#include "chrome/browser/ui/browser_navigator_params.h"
#include "chrome/browser/ui/browser_tabstrip.h"
#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/tabs/tab_group_id.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/url_constants.h"
@@ -872,6 +874,134 @@
new_browser->tab_strip_model()->GetActiveWebContents()->GetURL());
}
+namespace {
+
+// Groups the tabs in |model| according to |specified_groups|.
+void CreateTabGroups(TabStripModel* model,
+ base::span<const base::Optional<int>> specified_groups) {
+ ASSERT_EQ(model->count(), static_cast<int>(specified_groups.size()));
+
+ // Maps |specified_groups| IDs to actual group IDs in |model|.
+ base::flat_map<int, TabGroupId> group_map;
+
+ for (int i = 0; i < model->count(); ++i) {
+ if (specified_groups[i] == base::nullopt)
+ continue;
+
+ const int specified_group = specified_groups[i].value();
+ auto match = group_map.find(specified_group);
+
+ // If |group_map| doesn't contain a value for |specified_group|, we can
+ // assume we haven't created the group yet.
+ if (match == group_map.end()) {
+ const TabGroupId actual_group = model->AddToNewGroup({i});
+ group_map.insert(std::make_pair(specified_group, actual_group));
+ } else {
+ const content::WebContents* const contents = model->GetWebContentsAt(i);
+ model->AddToExistingGroup({i}, match->second);
+ // Make sure we didn't move the tab.
+ EXPECT_EQ(contents, model->GetWebContentsAt(i));
+ }
+ }
+}
+
+// Checks that the grouping of tabs in |model| is equivalent to that specified
+// in |specified_groups| up to relabeling of the group IDs.
+void CheckTabGrouping(TabStripModel* model,
+ base::span<const base::Optional<int>> specified_groups) {
+ ASSERT_EQ(model->count(), static_cast<int>(specified_groups.size()));
+
+ // Maps |specified_groups| IDs to actual group IDs in |model|.
+ base::flat_map<int, TabGroupId> group_map;
+
+ for (int i = 0; i < model->count(); ++i) {
+ SCOPED_TRACE(i);
+
+ const base::Optional<int> specified_group = specified_groups[i];
+ const base::Optional<TabGroupId> actual_group = model->GetTabGroupForTab(i);
+
+ // The tab should be grouped iff it's grouped in |specified_groups|.
+ EXPECT_EQ(actual_group.has_value(), specified_group.has_value());
+
+ if (actual_group.has_value() && specified_group.has_value()) {
+ auto match = group_map.find(specified_group.value());
+ if (match == group_map.end()) {
+ group_map.insert(
+ std::make_pair(specified_group.value(), actual_group.value()));
+ } else {
+ EXPECT_EQ(actual_group.value(), match->second);
+ }
+ }
+ }
+}
+
+// Returns the optional group ID for each tab in a vector.
+std::vector<base::Optional<TabGroupId>> GetTabGroups(
+ const TabStripModel* model) {
+ std::vector<base::Optional<TabGroupId>> result(model->count());
+ for (int i = 0; i < model->count(); ++i)
+ result[i] = model->GetTabGroupForTab(i);
+ return result;
+}
+
+} // namespace
+
+IN_PROC_BROWSER_TEST_F(SessionRestoreTest, TabsWithGroups) {
+ constexpr int kNumTabs = 6;
+ const std::array<base::Optional<int>, kNumTabs> group_spec = {
+ 0, 0, base::nullopt, base::nullopt, 1, 1};
+
+ // Open |kNumTabs| tabs.
+ ui_test_utils::NavigateToURL(browser(), url1_);
+ for (int i = 1; i < kNumTabs; ++i) {
+ ui_test_utils::NavigateToURLWithDisposition(
+ browser(), url1_, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+ ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+ }
+ ASSERT_EQ(kNumTabs, browser()->tab_strip_model()->count());
+
+ CreateTabGroups(browser()->tab_strip_model(), group_spec);
+ ASSERT_NO_FATAL_FAILURE(
+ CheckTabGrouping(browser()->tab_strip_model(), group_spec));
+ const auto groups = GetTabGroups(browser()->tab_strip_model());
+
+ Browser* new_browser = QuitBrowserAndRestore(browser(), kNumTabs);
+ ASSERT_EQ(kNumTabs, new_browser->tab_strip_model()->count());
+ EXPECT_EQ(groups, GetTabGroups(new_browser->tab_strip_model()));
+}
+
+// Test that tab groups are restored correctly after the command set is rebuilt
+// from the browser state.
+IN_PROC_BROWSER_TEST_F(SessionRestoreTest, TabsWithGroupsCommandReset) {
+ constexpr int kNumTabs = 6;
+ const std::array<base::Optional<int>, kNumTabs> group_spec = {
+ 0, 0, base::nullopt, base::nullopt, 1, 1};
+
+ // Open |kNumTabs| tabs.
+ ui_test_utils::NavigateToURL(browser(), url1_);
+ for (int i = 1; i < kNumTabs; ++i) {
+ ui_test_utils::NavigateToURLWithDisposition(
+ browser(), url1_, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+ ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+ }
+ ASSERT_EQ(kNumTabs, browser()->tab_strip_model()->count());
+
+ CreateTabGroups(browser()->tab_strip_model(), group_spec);
+ ASSERT_NO_FATAL_FAILURE(
+ CheckTabGrouping(browser()->tab_strip_model(), group_spec));
+ const auto groups = GetTabGroups(browser()->tab_strip_model());
+
+ // Rebuild commands.
+ SessionService* const session_service =
+ SessionServiceFactory::GetForProfile(browser()->profile());
+ ASSERT_TRUE(session_service);
+ session_service->ResetFromCurrentBrowsers();
+
+ Browser* new_browser = QuitBrowserAndRestore(browser(), kNumTabs);
+ ASSERT_EQ(kNumTabs, new_browser->tab_strip_model()->count());
+ EXPECT_EQ(groups, GetTabGroups(new_browser->tab_strip_model()));
+}
+
IN_PROC_BROWSER_TEST_F(SessionRestoreTest, RestoreAfterDelete) {
ui_test_utils::NavigateToURL(browser(), url1_);
ui_test_utils::NavigateToURL(browser(), url2_);
diff --git a/chrome/browser/sessions/session_restore_delegate.cc b/chrome/browser/sessions/session_restore_delegate.cc
index c5ca690..7a8cdaa 100644
--- a/chrome/browser/sessions/session_restore_delegate.cc
+++ b/chrome/browser/sessions/session_restore_delegate.cc
@@ -38,16 +38,21 @@
} // namespace
-SessionRestoreDelegate::RestoredTab::RestoredTab(content::WebContents* contents,
- bool is_active,
- bool is_app,
- bool is_pinned)
+SessionRestoreDelegate::RestoredTab::RestoredTab(
+ content::WebContents* contents,
+ bool is_active,
+ bool is_app,
+ bool is_pinned,
+ const base::Optional<base::Token>& group)
: contents_(contents),
is_active_(is_active),
is_app_(is_app),
is_internal_page_(IsInternalPage(contents->GetLastCommittedURL())),
- is_pinned_(is_pinned) {
-}
+ is_pinned_(is_pinned),
+ group_(group) {}
+
+SessionRestoreDelegate::RestoredTab::RestoredTab(const RestoredTab& other) =
+ default;
bool SessionRestoreDelegate::RestoredTab::operator<(
const RestoredTab& right) const {
diff --git a/chrome/browser/sessions/session_restore_delegate.h b/chrome/browser/sessions/session_restore_delegate.h
index 9e5b021..4d0e69d 100644
--- a/chrome/browser/sessions/session_restore_delegate.h
+++ b/chrome/browser/sessions/session_restore_delegate.h
@@ -8,7 +8,10 @@
#include <vector>
#include "base/macros.h"
+#include "base/optional.h"
#include "base/time/time.h"
+#include "base/token.h"
+#include "components/sessions/core/session_id.h"
namespace content {
class WebContents;
@@ -23,7 +26,9 @@
RestoredTab(content::WebContents* contents,
bool is_active,
bool is_app,
- bool is_pinned);
+ bool is_pinned,
+ const base::Optional<base::Token>& group);
+ RestoredTab(const RestoredTab& other);
bool operator<(const RestoredTab& right) const;
@@ -32,6 +37,7 @@
bool is_app() const { return is_app_; }
bool is_internal_page() const { return is_internal_page_; }
bool is_pinned() const { return is_pinned_; }
+ const base::Optional<base::Token>& group() const { return group_; }
private:
content::WebContents* contents_;
@@ -39,6 +45,9 @@
bool is_app_; // Browser window is an app.
bool is_internal_page_; // Internal web UI page, like NTP or Settings.
bool is_pinned_;
+ // The ID for the tab group that this tab belonged to, if any. See
+ // |TabStripModel::AddToNewGroup()| for more documentation.
+ base::Optional<base::Token> group_;
};
static void RestoreTabs(const std::vector<RestoredTab>& tabs,
diff --git a/chrome/browser/sessions/session_restore_observer_unittest.cc b/chrome/browser/sessions/session_restore_observer_unittest.cc
index 4a3fdfd..d4f7891 100644
--- a/chrome/browser/sessions/session_restore_observer_unittest.cc
+++ b/chrome/browser/sessions/session_restore_observer_unittest.cc
@@ -83,7 +83,8 @@
void SetUp() override {
ChromeRenderViewHostTestHarness::SetUp();
SetContents(CreateRestoredWebContents());
- restored_tabs_.emplace_back(web_contents(), false, false, false);
+ restored_tabs_.emplace_back(web_contents(), false, false, false,
+ base::nullopt);
}
void TearDown() override {
@@ -174,7 +175,7 @@
different_test_contents.emplace_back(CreateRestoredWebContents());
content::WebContents* test_contents = different_test_contents.back().get();
std::vector<RestoredTab> restored_tabs{
- RestoredTab(test_contents, false, false, false)};
+ RestoredTab(test_contents, false, false, false, base::nullopt)};
SessionRestore::NotifySessionRestoreStartedLoadingTabs();
SessionRestore::OnWillRestoreTab(test_contents);
@@ -198,7 +199,8 @@
TEST_F(SessionRestoreObserverTest, ConcurrentSessionRestores) {
std::vector<RestoredTab> another_restored_tabs;
auto test_contents = CreateRestoredWebContents();
- another_restored_tabs.emplace_back(test_contents.get(), false, false, false);
+ another_restored_tabs.emplace_back(test_contents.get(), false, false, false,
+ base::nullopt);
SessionRestore::NotifySessionRestoreStartedLoadingTabs();
SessionRestore::OnWillRestoreTab(web_contents());
@@ -226,7 +228,7 @@
std::vector<SessionRestoreDelegate::RestoredTab> restored_tabs{
SessionRestoreDelegate::RestoredTab(test_contents.get(), false, false,
- false)};
+ false, base::nullopt)};
resource_coordinator::TabManager* tab_manager =
g_browser_process->GetTabManager();
diff --git a/chrome/browser/sessions/session_restore_stats_collector_unittest.cc b/chrome/browser/sessions/session_restore_stats_collector_unittest.cc
index a57ec4b4..f0a7c5c 100644
--- a/chrome/browser/sessions/session_restore_stats_collector_unittest.cc
+++ b/chrome/browser/sessions/session_restore_stats_collector_unittest.cc
@@ -278,7 +278,8 @@
// Create a last active time in the past.
content::WebContentsTester::For(contents)->SetLastActiveTime(
base::TimeTicks::Now() - base::TimeDelta::FromMinutes(1));
- restored_tabs_.push_back(RestoredTab(contents, is_active, false, false));
+ restored_tabs_.push_back(
+ RestoredTab(contents, is_active, false, false, base::nullopt));
if (is_active)
Show(restored_tabs_.size() - 1);
}
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index 3add921..8e5be04 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -16,6 +16,7 @@
#include "base/command_line.h"
#include "base/metrics/histogram_macros.h"
#include "base/pickle.h"
+#include "base/stl_util.h"
#include "base/threading/thread.h"
#include "build/build_config.h"
#include "chrome/browser/background/background_mode_manager.h"
@@ -37,6 +38,7 @@
#include "chrome/browser/ui/browser_tabstrip.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/startup/startup_browser_creator.h"
+#include "chrome/browser/ui/tabs/tab_group_id.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/web_applications/components/web_app_helpers.h"
#include "components/sessions/content/content_serialized_navigation_builder.h"
@@ -198,6 +200,21 @@
sessions::CreateSetTabIndexInWindowCommand(tab_id, new_index));
}
+void SessionService::SetTabGroup(const SessionID& window_id,
+ const SessionID& tab_id,
+ base::Optional<base::Token> group) {
+ if (!ShouldTrackChangesToWindow(window_id))
+ return;
+
+ // Tabs get ungrouped as they close. However, if the whole window is closing
+ // tabs should stay in their groups. So, ignore this call in that case.
+ if (base::ContainsKey(pending_window_close_ids_, window_id) ||
+ base::ContainsKey(window_closing_ids_, window_id))
+ return;
+
+ ScheduleCommand(sessions::CreateTabGroupCommand(tab_id, group));
+}
+
void SessionService::SetPinnedState(const SessionID& window_id,
const SessionID& tab_id,
bool is_pinned) {
@@ -464,7 +481,9 @@
if (!ShouldTrackChangesToWindow(session_tab_helper->window_id()))
return;
- BuildCommandsForTab(session_tab_helper->window_id(), tab, -1, pinned, NULL);
+ // TODO(crbug.com/930991): handle tab groups here.
+ BuildCommandsForTab(session_tab_helper->window_id(), tab, -1, base::nullopt,
+ pinned, NULL);
base_session_service_->StartSaveTimer();
}
@@ -642,6 +661,7 @@
void SessionService::BuildCommandsForTab(const SessionID& window_id,
WebContents* tab,
int index_in_window,
+ base::Optional<base::Token> group,
bool is_pinned,
IdToRange* tab_to_available_range) {
DCHECK(tab);
@@ -710,6 +730,11 @@
index_in_window));
}
+ if (group.has_value()) {
+ base_session_service_->AppendRebuildCommand(
+ sessions::CreateTabGroupCommand(session_id, group));
+ }
+
// Record the association between the sessionStorage namespace and the tab.
content::SessionStorageNamespace* session_storage_namespace =
tab->GetController().GetDefaultSessionStorageNamespace();
@@ -750,11 +775,12 @@
for (int i = 0; i < tab_strip->count(); ++i) {
WebContents* tab = tab_strip->GetWebContentsAt(i);
DCHECK(tab);
- BuildCommandsForTab(browser->session_id(),
- tab,
- i,
- tab_strip->IsTabPinned(i),
- tab_to_available_range);
+ const base::Optional<TabGroupId> group_id = tab_strip->GetTabGroupForTab(i);
+ const base::Optional<base::Token> raw_group_id =
+ group_id.has_value() ? base::make_optional(group_id.value().token())
+ : base::nullopt;
+ BuildCommandsForTab(browser->session_id(), tab, i, raw_group_id,
+ tab_strip->IsTabPinned(i), tab_to_available_range);
}
base_session_service_->AppendRebuildCommand(
diff --git a/chrome/browser/sessions/session_service.h b/chrome/browser/sessions/session_service.h
index f6fb5ae..f4b2daef 100644
--- a/chrome/browser/sessions/session_service.h
+++ b/chrome/browser/sessions/session_service.h
@@ -13,8 +13,10 @@
#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
+#include "base/optional.h"
#include "base/task/cancelable_task_tracker.h"
#include "base/time/time.h"
+#include "base/token.h"
#include "chrome/browser/defaults.h"
#include "chrome/browser/sessions/session_common_utils.h"
#include "chrome/browser/sessions/session_service_utils.h"
@@ -129,6 +131,11 @@
const SessionID& tab_id,
int new_index);
+ // Sets a tab's group ID, if any.
+ void SetTabGroup(const SessionID& window_id,
+ const SessionID& tab_id,
+ base::Optional<base::Token> group);
+
// Sets the pinned state of the tab.
void SetPinnedState(const SessionID& window_id,
const SessionID& tab_id,
@@ -268,12 +275,12 @@
// direction from the current navigation index).
// A pair is added to tab_to_available_range indicating the range of
// indices that were written.
- void BuildCommandsForTab(
- const SessionID& window_id,
- content::WebContents* tab,
- int index_in_window,
- bool is_pinned,
- IdToRange* tab_to_available_range);
+ void BuildCommandsForTab(const SessionID& window_id,
+ content::WebContents* tab,
+ int index_in_window,
+ base::Optional<base::Token> group,
+ bool is_pinned,
+ IdToRange* tab_to_available_range);
// Adds commands to create the specified browser, and invokes
// BuildCommandsForTab for each of the tabs in the browser. This ignores
diff --git a/chrome/browser/sessions/session_service_unittest.cc b/chrome/browser/sessions/session_service_unittest.cc
index c09e9cd6..89eef283 100644
--- a/chrome/browser/sessions/session_service_unittest.cc
+++ b/chrome/browser/sessions/session_service_unittest.cc
@@ -21,6 +21,7 @@
#include "base/synchronization/atomic_flag.h"
#include "base/threading/platform_thread.h"
#include "base/time/time.h"
+#include "base/token.h"
#include "build/build_config.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/defaults.h"
@@ -39,7 +40,9 @@
#include "components/sessions/core/session_command.h"
#include "components/sessions/core/session_types.h"
#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/web_contents.h"
#include "content/public/common/page_state.h"
+#include "content/public/test/web_contents_tester.h"
#include "testing/gtest/include/gtest/gtest.h"
using content::NavigationEntry;
@@ -87,6 +90,16 @@
}
}
+ SessionID CreateTabWithTestNavigationData(SessionID window_id,
+ int visual_index) {
+ const SessionID tab_id = SessionID::NewUnique();
+ const SerializedNavigationEntry nav =
+ SerializedNavigationEntryTestHelper::CreateNavigationForTest();
+ helper_.PrepareTabInWindow(window_id, tab_id, visual_index, true);
+ UpdateNavigation(window_id, tab_id, nav, true);
+ return tab_id;
+ }
+
void ReadWindows(
std::vector<std::unique_ptr<sessions::SessionWindow>>* windows,
SessionID* active_window_id) {
@@ -1164,6 +1177,46 @@
helper_.AssertNavigationEquals(nav1, tab->navigations[0]);
}
+TEST_F(SessionServiceTest, TabGroupDefaultsToNone) {
+ CreateTabWithTestNavigationData(window_id, 0);
+
+ std::vector<std::unique_ptr<sessions::SessionWindow>> windows;
+ ReadWindows(&windows, nullptr);
+
+ ASSERT_EQ(1U, windows.size());
+ ASSERT_EQ(1U, windows[0]->tabs.size());
+
+ // Verify that the recorded tab has no group.
+ sessions::SessionTab* tab = windows[0]->tabs[0].get();
+ EXPECT_EQ(base::nullopt, tab->group);
+}
+
+TEST_F(SessionServiceTest, TabGroupIdsSaved) {
+ const auto group1_token = base::Token::CreateRandom();
+ const auto group2_token = base::Token::CreateRandom();
+ constexpr int kNumTabs = 5;
+ const std::array<base::Optional<base::Token>, kNumTabs> groups = {
+ base::nullopt, group1_token, group1_token, base::nullopt, group2_token};
+
+ // Create |kNumTabs| tabs with group IDs in |groups|.
+ for (int tab_ndx = 0; tab_ndx < kNumTabs; ++tab_ndx) {
+ const SessionID tab_id =
+ CreateTabWithTestNavigationData(window_id, tab_ndx);
+ service()->SetTabGroup(window_id, tab_id, groups[tab_ndx]);
+ }
+
+ std::vector<std::unique_ptr<sessions::SessionWindow>> windows;
+ ReadWindows(&windows, nullptr);
+
+ ASSERT_EQ(1U, windows.size());
+ ASSERT_EQ(kNumTabs, static_cast<int>(windows[0]->tabs.size()));
+
+ for (int tab_ndx = 0; tab_ndx < kNumTabs; ++tab_ndx) {
+ sessions::SessionTab* tab = windows[0]->tabs[tab_ndx].get();
+ EXPECT_EQ(groups[tab_ndx], tab->group);
+ }
+}
+
// Functions used by GetSessionsAndDestroy.
namespace {
diff --git a/chrome/browser/sessions/tab_loader_unittest.cc b/chrome/browser/sessions/tab_loader_unittest.cc
index 8d4c561..4f79c90 100644
--- a/chrome/browser/sessions/tab_loader_unittest.cc
+++ b/chrome/browser/sessions/tab_loader_unittest.cc
@@ -103,9 +103,9 @@
// TabLoadTracker needs the resource_coordinator WebContentsData to be
// initialized.
ResourceCoordinatorTabHelper::CreateForWebContents(test_contents);
- restored_tabs_.push_back(
- RestoredTab(test_contents, is_active /* is_active */,
- false /* is_app */, false /* is_pinned */));
+ restored_tabs_.push_back(RestoredTab(
+ test_contents, is_active /* is_active */, false /* is_app */,
+ false /* is_pinned */, base::nullopt /* group */));
// If the tab is active start "loading" it right away for consistency with
// session restore code.
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 54f5f800..e9165bee 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -1026,9 +1026,12 @@
replace->index);
break;
}
- case TabStripModelChange::kGroupChanged:
- // TODO(crbug.com/930991): Save Tab Group data to SessionService.
+ case TabStripModelChange::kGroupChanged: {
+ auto* group_change = change.GetGroupChange();
+ OnTabGroupChanged(group_change->index, group_change->old_group,
+ group_change->new_group);
break;
+ }
case TabStripModelChange::kSelectionOnly:
break;
}
@@ -2261,6 +2264,24 @@
}
}
+void Browser::OnTabGroupChanged(int index,
+ base::Optional<TabGroupId> old_group,
+ base::Optional<TabGroupId> new_group) {
+ content::WebContents* const web_contents =
+ tab_strip_model_->GetWebContentsAt(index);
+ SessionService* const session_service =
+ SessionServiceFactory::GetForProfile(profile_);
+ if (session_service) {
+ SessionTabHelper* const session_tab_helper =
+ SessionTabHelper::FromWebContents(web_contents);
+ const base::Optional<base::Token> raw_id =
+ new_group.has_value() ? base::make_optional(new_group.value().token())
+ : base::nullopt;
+ session_service->SetTabGroup(session_id(), session_tab_helper->session_id(),
+ raw_id);
+ }
+}
+
void Browser::OnDevToolsAvailabilityChanged() {
using DTPH = policy::DeveloperToolsPolicyHandler;
// We close all windows as a safety measure, even for
@@ -2429,6 +2450,15 @@
session_service->SetPinnedState(session_id(),
session_tab_helper->session_id(),
tab_strip_model_->IsTabPinned(i));
+
+ base::Optional<TabGroupId> group_id =
+ tab_strip_model_->GetTabGroupForTab(i);
+ base::Optional<base::Token> raw_group_id =
+ group_id.has_value()
+ ? base::Optional<base::Token>(group_id.value().token())
+ : base::nullopt;
+ session_service->SetTabGroup(
+ session_id(), session_tab_helper->session_id(), raw_group_id);
}
}
}
diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h
index c527ad1..5483fef1 100644
--- a/chrome/browser/ui/browser.h
+++ b/chrome/browser/ui/browser.h
@@ -836,6 +836,9 @@
void OnTabReplacedAt(content::WebContents* old_contents,
content::WebContents* new_contents,
int index);
+ void OnTabGroupChanged(int index,
+ base::Optional<TabGroupId> old_group,
+ base::Optional<TabGroupId> new_group);
// Handle changes to kDevToolsAvailability preference.
void OnDevToolsAvailabilityChanged();
diff --git a/chrome/browser/ui/browser_live_tab_context.cc b/chrome/browser/ui/browser_live_tab_context.cc
index 81d6154..a19d07d 100644
--- a/chrome/browser/ui/browser_live_tab_context.cc
+++ b/chrome/browser/ui/browser_live_tab_context.cc
@@ -90,8 +90,8 @@
WebContents* web_contents = chrome::AddRestoredTab(
browser_, navigations, tab_index, selected_navigation, extension_app_id,
- select, pin, from_last_session, base::TimeTicks(), storage_namespace,
- user_agent_override, false /* from_session_restore */);
+ base::nullopt, select, pin, from_last_session, base::TimeTicks(),
+ storage_namespace, user_agent_override, false /* from_session_restore */);
#if BUILDFLAG(ENABLE_SESSION_SERVICE)
// The focused tab will be loaded by Browser, and TabLoader will load the
@@ -104,7 +104,7 @@
}
std::vector<TabLoader::RestoredTab> restored_tabs;
restored_tabs.emplace_back(web_contents, select, !extension_app_id.empty(),
- pin);
+ pin, base::nullopt);
TabLoader::RestoreTabs(restored_tabs, base::TimeTicks::Now());
#else // BUILDFLAG(ENABLE_SESSION_SERVICE)
// Load the tab manually if there is no TabLoader.
diff --git a/chrome/browser/ui/browser_tabrestore.cc b/chrome/browser/ui/browser_tabrestore.cc
index 055965a..2b5f55bc 100644
--- a/chrome/browser/ui/browser_tabrestore.cc
+++ b/chrome/browser/ui/browser_tabrestore.cc
@@ -15,6 +15,7 @@
#include "chrome/browser/tab_contents/tab_util.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/tabs/tab_group_id.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/web_contents_sizer.h"
#include "components/sessions/content/content_serialized_navigation_builder.h"
@@ -100,6 +101,7 @@
int tab_index,
int selected_navigation,
const std::string& extension_app_id,
+ base::Optional<base::Token> raw_group_id,
bool select,
bool pin,
bool from_last_session,
@@ -118,9 +120,16 @@
tab_index, browser->tab_strip_model()->IndexOfFirstNonPinnedTab());
add_types |= TabStripModel::ADD_PINNED;
}
+
WebContents* raw_web_contents = web_contents.get();
- browser->tab_strip_model()->InsertWebContentsAt(
+ const int actual_index = browser->tab_strip_model()->InsertWebContentsAt(
tab_index, std::move(web_contents), add_types);
+
+ if (raw_group_id.has_value()) {
+ auto group_id = TabGroupId::FromRawToken(raw_group_id.value());
+ browser->tab_strip_model()->AddToGroupForRestore({actual_index}, group_id);
+ }
+
if (select) {
if (!browser->window()->IsMinimized())
browser->window()->Activate();
diff --git a/chrome/browser/ui/browser_tabrestore.h b/chrome/browser/ui/browser_tabrestore.h
index 451ead1..78fdbb6 100644
--- a/chrome/browser/ui/browser_tabrestore.h
+++ b/chrome/browser/ui/browser_tabrestore.h
@@ -8,6 +8,7 @@
#include <string>
#include <vector>
+#include "base/token.h"
#include "components/sessions/core/session_types.h"
class Browser;
@@ -28,11 +29,12 @@
// |tab_index| gives the index to insert the tab at. |selected_navigation| is
// the index of the SerializedNavigationEntry in |navigations| to select. If
// |extension_app_id| is non-empty the tab is an app tab and |extension_app_id|
-// is the id of the extension. If |pin| is true and |tab_index|/ is the last
-// pinned tab, then the newly created tab is pinned. If |from_last_session| is
-// true, |navigations| are from the previous session. |user_agent_override|
-// contains the string being used as the user agent for all of the tab's
-// navigations when the regular user agent is overridden. If
+// is the id of the extension. If |raw_group_id| has a value, it specifies the
+// token corresponding to the tab's group. If |pin| is true and |tab_index|/ is
+// the last pinned tab, then the newly created tab is pinned. If
+// |from_last_session| is true, |navigations| are from the previous session.
+// |user_agent_override| contains the string being used as the user agent for
+// all of the tab's navigations when the regular user agent is overridden. If
// |from_session_restore| is true, the restored tab is created by session
// restore. |last_active_time| is the value to use to indicate the last time the
// WebContents was made active, if this is left default initialized then the
@@ -43,6 +45,7 @@
int tab_index,
int selected_navigation,
const std::string& extension_app_id,
+ base::Optional<base::Token> raw_group_id,
bool select,
bool pin,
bool from_last_session,
diff --git a/chrome/browser/ui/tabs/tab_group_id.cc b/chrome/browser/ui/tabs/tab_group_id.cc
index fdb59328..80f07f2f 100644
--- a/chrome/browser/ui/tabs/tab_group_id.cc
+++ b/chrome/browser/ui/tabs/tab_group_id.cc
@@ -9,6 +9,11 @@
return TabGroupId(base::Token::CreateRandom());
}
+// static
+TabGroupId TabGroupId::FromRawToken(base::Token token) {
+ return TabGroupId(token);
+}
+
TabGroupId::TabGroupId(const TabGroupId& other) = default;
TabGroupId& TabGroupId::operator=(const TabGroupId& other) = default;
diff --git a/chrome/browser/ui/tabs/tab_group_id.h b/chrome/browser/ui/tabs/tab_group_id.h
index 04bc9e6b..d61a8fd 100644
--- a/chrome/browser/ui/tabs/tab_group_id.h
+++ b/chrome/browser/ui/tabs/tab_group_id.h
@@ -11,6 +11,10 @@
public:
static TabGroupId GenerateNew();
+ // This should only called with |token| returned from a previous |token()|
+ // call on a valid TabGroupId.
+ static TabGroupId FromRawToken(base::Token token);
+
TabGroupId(const TabGroupId& other);
TabGroupId& operator=(const TabGroupId& other);
diff --git a/chrome/browser/ui/tabs/tab_strip_model.cc b/chrome/browser/ui/tabs/tab_strip_model.cc
index 1a1916c..c5c87a4 100644
--- a/chrome/browser/ui/tabs/tab_strip_model.cc
+++ b/chrome/browser/ui/tabs/tab_strip_model.cc
@@ -769,7 +769,7 @@
}
bool TabStripModel::IsTabPinned(int index) const {
- DCHECK(ContainsIndex(index));
+ DCHECK(ContainsIndex(index)) << index;
return contents_data_[index]->pinned();
}
@@ -981,39 +981,17 @@
MoveWebContentsAt(active_index(), new_index, true);
}
-void TabStripModel::AddToNewGroup(const std::vector<int>& indices) {
+TabGroupId TabStripModel::AddToNewGroup(const std::vector<int>& indices) {
DCHECK(!reentrancy_guard_);
base::AutoReset<bool> resetter(&reentrancy_guard_, true);
+ // The odds of |new_group| colliding with an existing group are astronomically
+ // low. If there is a collision, a DCHECK will fail in |AddToNewGroupImpl()|,
+ // in which case there is probably something wrong with
+ // |TabGroupId::GenerateNew()|.
const TabGroupId new_group = TabGroupId::GenerateNew();
- // If the random generator is working correctly, the odds of this happening
- // are astronomically low. If this DCHECK fails, something is probably wrong
- // in |TabGroupId::GenerateNew()|.
- DCHECK(!std::any_of(
- contents_data_.cbegin(), contents_data_.cend(),
- [new_group](const auto& datum) { return datum->group() == new_group; }));
-
- group_data_[new_group] = std::make_unique<TabGroupData>();
-
- // Find a destination for the first tab that's not inside another group. We
- // will stack the rest of the tabs up to its right.
- int destination_index = -1;
- for (int i = indices[0]; i < count(); i++) {
- const int destination_candidate = i + 1;
- const bool end_of_strip = !ContainsIndex(destination_candidate);
- if (end_of_strip || !GetTabGroupForTab(destination_candidate).has_value() ||
- GetTabGroupForTab(destination_candidate) !=
- GetTabGroupForTab(indices[0])) {
- destination_index = destination_candidate;
- break;
- }
- }
-
- std::vector<int> new_indices = indices;
- if (IsTabPinned(new_indices[0]))
- new_indices = SetTabsPinned(new_indices, true);
-
- MoveTabsIntoGroup(new_indices, destination_index, new_group);
+ AddToNewGroupImpl(indices, new_group);
+ return new_group;
}
void TabStripModel::AddToExistingGroup(const std::vector<int>& indices,
@@ -1021,25 +999,19 @@
DCHECK(!reentrancy_guard_);
base::AutoReset<bool> resetter(&reentrancy_guard_, true);
- int destination_index = -1;
- bool pin = false;
- for (int i = contents_data_.size() - 1; i >= 0; i--) {
- if (contents_data_[i]->group() == group) {
- destination_index = i + 1;
- pin = IsTabPinned(i);
- break;
- }
- }
+ AddToExistingGroupImpl(indices, group);
+}
- // Ignore indices that are already in the group.
- std::vector<int> new_indices;
- for (int candidate_index : indices) {
- if (GetTabGroupForTab(candidate_index) != group)
- new_indices.push_back(candidate_index);
- }
- new_indices = SetTabsPinned(new_indices, pin);
+void TabStripModel::AddToGroupForRestore(const std::vector<int>& indices,
+ TabGroupId group) {
+ DCHECK(!reentrancy_guard_);
+ base::AutoReset<bool> resetter(&reentrancy_guard_, true);
- MoveTabsIntoGroup(new_indices, destination_index, group);
+ const bool group_exists = base::ContainsKey(group_data_, group);
+ if (group_exists)
+ AddToExistingGroupImpl(indices, group);
+ else
+ AddToNewGroupImpl(indices, group);
}
void TabStripModel::RemoveFromGroup(const std::vector<int>& indices) {
@@ -1767,6 +1739,58 @@
}
}
+void TabStripModel::AddToNewGroupImpl(const std::vector<int>& indices,
+ TabGroupId new_group) {
+ DCHECK(!std::any_of(
+ contents_data_.cbegin(), contents_data_.cend(),
+ [new_group](const auto& datum) { return datum->group() == new_group; }));
+
+ group_data_[new_group] = std::make_unique<TabGroupData>();
+
+ // Find a destination for the first tab that's not inside another group. We
+ // will stack the rest of the tabs up to its right.
+ int destination_index = -1;
+ for (int i = indices[0]; i < count(); i++) {
+ const int destination_candidate = i + 1;
+ const bool end_of_strip = !ContainsIndex(destination_candidate);
+ if (end_of_strip || !GetTabGroupForTab(destination_candidate).has_value() ||
+ GetTabGroupForTab(destination_candidate) !=
+ GetTabGroupForTab(indices[0])) {
+ destination_index = destination_candidate;
+ break;
+ }
+ }
+
+ std::vector<int> new_indices = indices;
+ if (IsTabPinned(new_indices[0]))
+ new_indices = SetTabsPinned(new_indices, true);
+
+ MoveTabsIntoGroup(new_indices, destination_index, new_group);
+}
+
+void TabStripModel::AddToExistingGroupImpl(const std::vector<int>& indices,
+ TabGroupId group) {
+ int destination_index = -1;
+ bool pin = false;
+ for (int i = contents_data_.size() - 1; i >= 0; i--) {
+ if (contents_data_[i]->group() == group) {
+ destination_index = i + 1;
+ pin = IsTabPinned(i);
+ break;
+ }
+ }
+
+ // Ignore indices that are already in the group.
+ std::vector<int> new_indices;
+ for (int candidate_index : indices) {
+ if (GetTabGroupForTab(candidate_index) != group)
+ new_indices.push_back(candidate_index);
+ }
+ new_indices = SetTabsPinned(new_indices, pin);
+
+ MoveTabsIntoGroup(new_indices, destination_index, group);
+}
+
void TabStripModel::MoveTabsIntoGroup(const std::vector<int>& indices,
int destination_index,
TabGroupId group) {
diff --git a/chrome/browser/ui/tabs/tab_strip_model.h b/chrome/browser/ui/tabs/tab_strip_model.h
index 5d9b6fa..3b22c4cb 100644
--- a/chrome/browser/ui/tabs/tab_strip_model.h
+++ b/chrome/browser/ui/tabs/tab_strip_model.h
@@ -392,10 +392,10 @@
// Create a new tab group and add the set of tabs pointed to be |indices| to
// it. Pins all of the tabs if any of them were pinned, and reorders the tabs
- // so they are contiguous and do not split an existing group in half.
- // |indices| must be sorted in ascending order. This feature is in development
- // and gated behind a feature flag. https://crbug.com/915956.
- void AddToNewGroup(const std::vector<int>& indices);
+ // so they are contiguous and do not split an existing group in half. Returns
+ // the new group. |indices| must be sorted in ascending order. This feature is
+ // in development and gated behind a feature flag. https://crbug.com/915956.
+ TabGroupId AddToNewGroup(const std::vector<int>& indices);
// Add the set of tabs pointed to by |indices| to the given tab group |group|.
// The tabs take on the pinnedness of the tabs already in the group, and are
@@ -404,6 +404,11 @@
// behind a feature flag (see https://crbug.com/915956).
void AddToExistingGroup(const std::vector<int>& indices, TabGroupId group);
+ // Similar to AddToExistingGroup(), but creates a group with id |group| if it
+ // doesn't exist. This is only intended to be called from session restore
+ // code.
+ void AddToGroupForRestore(const std::vector<int>& indices, TabGroupId group);
+
// Removes the set of tabs pointed to by |indices| from the the groups they
// are in, if any. The tabs are moved out of the group if necessary. |indices|
// must be sorted in ascending order. This feature is in development and gated
@@ -609,6 +614,15 @@
// starting at |start| to |index|. See MoveSelectedTabsTo for more details.
void MoveSelectedTabsToImpl(int index, size_t start, size_t length);
+ // Adds tabs to newly-allocated group id |new_group|. This group must be new
+ // and have no tabs in it.
+ void AddToNewGroupImpl(const std::vector<int>& indices, TabGroupId new_group);
+
+ // Adds tabs to existing group |group|. This group must have been initialized
+ // by a previous call to |AddToNewGroupImpl()|.
+ void AddToExistingGroupImpl(const std::vector<int>& indices,
+ TabGroupId group);
+
// Moves the set of tabs indicated by |indices| to precede the tab at index
// |destination_index|, maintaining their order and the order of tabs not
// being moved, and adds them to the tab group |group|.
diff --git a/components/sessions/core/session_service_commands.cc b/components/sessions/core/session_service_commands.cc
index 159accb..99a3301 100644
--- a/components/sessions/core/session_service_commands.cc
+++ b/components/sessions/core/session_service_commands.cc
@@ -12,6 +12,7 @@
#include "base/memory/ptr_util.h"
#include "base/pickle.h"
+#include "base/token.h"
#include "components/sessions/core/base_session_service_commands.h"
#include "components/sessions/core/base_session_service_delegate.h"
#include "components/sessions/core/session_command.h"
@@ -56,6 +57,7 @@
// static const SessionCommand::id_type kCommandSetWindowWorkspace = 22;
static const SessionCommand::id_type kCommandSetWindowWorkspace2 = 23;
static const SessionCommand::id_type kCommandTabNavigationPathPruned = 24;
+static const SessionCommand::id_type kCommandSetTabGroup = 25;
namespace {
@@ -110,6 +112,18 @@
int32_t count;
};
+struct SerializedTabGroupId {
+ // These fields correspond to the high and low fields of |base::Token|.
+ uint64_t id_high;
+ uint64_t id_low;
+};
+
+struct TabGroupPayload {
+ SessionID::id_type tab_id;
+ SerializedTabGroupId maybe_group;
+ bool has_group;
+};
+
struct PinnedStatePayload {
SessionID::id_type tab_id;
bool pinned_state;
@@ -544,6 +558,21 @@
break;
}
+ case kCommandSetTabGroup: {
+ TabGroupPayload payload;
+ if (!command->GetPayload(&payload, sizeof(payload))) {
+ DVLOG(1) << "Failed reading command " << command->id();
+ return true;
+ }
+ SessionTab* session_tab =
+ GetTab(SessionID::FromSerializedValue(payload.tab_id), tabs);
+ const base::Token token(payload.maybe_group.id_high,
+ payload.maybe_group.id_low);
+ session_tab->group =
+ payload.has_group ? base::make_optional(token) : base::nullopt;
+ break;
+ }
+
case kCommandSetPinnedState: {
PinnedStatePayload payload;
if (!command->GetPayload(&payload, sizeof(payload))) {
@@ -745,6 +774,20 @@
return CreateSessionCommandForPayload(kCommandSetWindowType, payload);
}
+std::unique_ptr<SessionCommand> CreateTabGroupCommand(
+ const SessionID& tab_id,
+ base::Optional<base::Token> group) {
+ TabGroupPayload payload = {0};
+ payload.tab_id = tab_id.id();
+ if (group.has_value()) {
+ DCHECK(!group.value().is_zero());
+ payload.maybe_group.id_high = group.value().high();
+ payload.maybe_group.id_low = group.value().low();
+ payload.has_group = true;
+ }
+ return CreateSessionCommandForPayload(kCommandSetTabGroup, payload);
+}
+
std::unique_ptr<SessionCommand> CreatePinnedStateCommand(
const SessionID& tab_id,
bool is_pinned) {
diff --git a/components/sessions/core/session_service_commands.h b/components/sessions/core/session_service_commands.h
index 0ed6fa72..ce0a0c6 100644
--- a/components/sessions/core/session_service_commands.h
+++ b/components/sessions/core/session_service_commands.h
@@ -11,7 +11,9 @@
#include "base/callback.h"
#include "base/memory/weak_ptr.h"
+#include "base/optional.h"
#include "base/task/cancelable_task_tracker.h"
+#include "base/token.h"
#include "components/sessions/core/base_session_service.h"
#include "components/sessions/core/session_types.h"
#include "components/sessions/core/sessions_export.h"
@@ -44,6 +46,9 @@
SESSIONS_EXPORT std::unique_ptr<SessionCommand> CreateSetWindowTypeCommand(
const SessionID& window_id,
SessionWindow::WindowType type);
+SESSIONS_EXPORT std::unique_ptr<SessionCommand> CreateTabGroupCommand(
+ const SessionID& tab_id,
+ base::Optional<base::Token> group);
SESSIONS_EXPORT std::unique_ptr<SessionCommand> CreatePinnedStateCommand(
const SessionID& tab_id,
bool is_pinned);
diff --git a/components/sessions/core/session_types.h b/components/sessions/core/session_types.h
index b161ca5..b160f2e9 100644
--- a/components/sessions/core/session_types.h
+++ b/components/sessions/core/session_types.h
@@ -11,8 +11,10 @@
#include <vector>
#include "base/macros.h"
+#include "base/optional.h"
#include "base/strings/string16.h"
#include "base/time/time.h"
+#include "base/token.h"
#include "components/sessions/core/serialized_navigation_entry.h"
#include "components/sessions/core/session_id.h"
#include "components/sessions/core/sessions_export.h"
@@ -65,6 +67,9 @@
// checking must be performed before indexing into |navigations|.
int current_navigation_index;
+ // The tab's group ID, if any.
+ base::Optional<base::Token> group;
+
// True if the tab is pinned.
bool pinned;
