[ TabRestore ] Remove duplicate group data from Window object
Consolidates the tab group mappings into one structure in tab_restore::Window. This prevents the duplication of group data. Change-Id: I34fcb4028b409b954d7e2296ceb2ea503cc8c456 Bug: 333425400, 333425400 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5502733 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Darryl James <dljames@chromium.org> Reviewed-by: Eshwar Stalin <estalin@chromium.org> Cr-Commit-Position: refs/heads/main@{#1295842}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
38a052d1ea
commit
30ca657612
chrome/browser
android
sessions
ui
components/sessions/core
@ -113,7 +113,7 @@ void AddBulkEventToEntries(
|
||||
group_titles.reserve(group_count);
|
||||
for (const auto& tab_group : window.tab_groups) {
|
||||
group_ids.push_back(tab_group.first.token());
|
||||
group_titles.push_back(&tab_group.second.title());
|
||||
group_titles.push_back(&tab_group.second->visual_data.title());
|
||||
}
|
||||
|
||||
Java_RecentlyClosedBridge_addBulkEventToEntries(
|
||||
|
@ -1991,7 +1991,7 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreEntireGroupInWindow) {
|
||||
|
||||
// Restore the double entry group.
|
||||
const auto& double_entry_group =
|
||||
*window_entry->groups.at(double_entry_group_id).get();
|
||||
*window_entry->tab_groups.at(double_entry_group_id).get();
|
||||
service->RestoreEntryById(second_browser->live_tab_context(),
|
||||
double_entry_group.id,
|
||||
WindowOpenDisposition::NEW_FOREGROUND_TAB);
|
||||
@ -2001,7 +2001,7 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreEntireGroupInWindow) {
|
||||
|
||||
// Restore one of the tabs in the double entry group.
|
||||
const auto& single_entry_group =
|
||||
*window_entry->groups.at(single_entry_group_id).get();
|
||||
*window_entry->tab_groups.at(single_entry_group_id).get();
|
||||
service->RestoreEntryById(second_browser->live_tab_context(),
|
||||
single_entry_group.id,
|
||||
WindowOpenDisposition::NEW_FOREGROUND_TAB);
|
||||
|
@ -1301,7 +1301,7 @@ TEST_F(TabRestoreServiceImplTest, TabGroupsRestoredFromSessionData) {
|
||||
auto* window = static_cast<sessions::tab_restore::Window*>(entry);
|
||||
ASSERT_EQ(1u, window->tabs.size());
|
||||
EXPECT_EQ(group, window->tabs[0]->group);
|
||||
EXPECT_EQ(group_visual_data, window->tab_groups[group]);
|
||||
EXPECT_EQ(group_visual_data, window->tab_groups[group]->visual_data);
|
||||
}
|
||||
|
||||
// Ensures tab extra data is restored from previous session.
|
||||
|
@ -589,9 +589,9 @@ RecentTabsSubMenuModel::CreateWindowSubMenuModel(
|
||||
*current_group);
|
||||
}
|
||||
|
||||
CHECK(window.groups.contains(tab->group.value()));
|
||||
CHECK(window.tab_groups.contains(tab->group.value()));
|
||||
seen_groups.emplace(tab->group.value());
|
||||
current_group = window.groups.at(tab->group.value()).get();
|
||||
current_group = window.tab_groups.at(tab->group.value()).get();
|
||||
current_group_model = CreateGroupSubMenuModel(*current_group);
|
||||
}
|
||||
|
||||
|
@ -221,15 +221,15 @@ void TabRestoreServiceHelper::BrowserClosing(LiveTabContext* context) {
|
||||
if (tab->navigations.empty()) {
|
||||
continue;
|
||||
}
|
||||
tab->browser_id = context->GetSessionID().id();
|
||||
|
||||
if (tab->group.has_value() &&
|
||||
!window->tab_groups.contains(tab->group.value())) {
|
||||
// Add new groups to the mapping if we haven't already.
|
||||
window->tab_groups.emplace(tab->group.value(),
|
||||
tab->group_visual_data.value());
|
||||
Group::FromTab(*tab.get()));
|
||||
}
|
||||
|
||||
tab->browser_id = context->GetSessionID().id();
|
||||
window->tabs.push_back(std::move(tab));
|
||||
}
|
||||
|
||||
@ -484,7 +484,6 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTabOrGroupFromWindow(
|
||||
});
|
||||
|
||||
if (other_tabs_in_group == window.tabs.end()) {
|
||||
window.groups.erase(group_id.value());
|
||||
window.tab_groups.erase(group_id.value());
|
||||
}
|
||||
}
|
||||
@ -501,7 +500,7 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTabOrGroupFromWindow(
|
||||
|
||||
// Check the groups for the id if we didn't find the tab.
|
||||
if (!found_tab_to_delete) {
|
||||
for (auto& group_pair : window.groups) {
|
||||
for (auto& group_pair : window.tab_groups) {
|
||||
auto& group = group_pair.second;
|
||||
if (group->id != id && group->original_id != id) {
|
||||
continue;
|
||||
@ -533,9 +532,8 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTabOrGroupFromWindow(
|
||||
window.tabs.erase(window.tabs.begin() + tab_i);
|
||||
}
|
||||
|
||||
// Clear groups from mappings.
|
||||
// Clear group from mapping.
|
||||
window.tab_groups.erase(group_id);
|
||||
window.groups.erase(group_id);
|
||||
|
||||
if (!window.tabs.empty()) {
|
||||
// Adjust |selected_tab index| to keep the window in a valid state.
|
||||
@ -735,25 +733,6 @@ void TabRestoreServiceHelper::AddEntry(std::unique_ptr<Entry> entry,
|
||||
return;
|
||||
}
|
||||
|
||||
if (entry->type == sessions::tab_restore::WINDOW) {
|
||||
auto& window = static_cast<Window&>(*entry.get());
|
||||
if (window.groups.empty()) {
|
||||
for (auto& tab : window.tabs) {
|
||||
if (tab->group.has_value() &&
|
||||
!window.groups.contains(tab->group.value())) {
|
||||
// Create an empty group that we can use later to find its tabs when
|
||||
// we need to restore an entire group from a window. This is done to
|
||||
// prevent duplicating of tab data.
|
||||
//
|
||||
// Creating the mapping here covers the cases where we close a browser
|
||||
// window and when restoring the last session on browser startup.
|
||||
auto group = Group::FromTab(*tab);
|
||||
window.groups.emplace(group->group_id, std::move(group));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (to_front) {
|
||||
entries_.push_front(std::move(entry));
|
||||
} else {
|
||||
@ -802,7 +781,7 @@ TabRestoreServiceHelper::GetEntryIteratorById(SessionID id) {
|
||||
}
|
||||
|
||||
// Or group in this window.
|
||||
for (const auto& group_pair : window.groups) {
|
||||
for (const auto& group_pair : window.tab_groups) {
|
||||
const std::unique_ptr<sessions::tab_restore::Group>& group =
|
||||
group_pair.second;
|
||||
if (group->id == id || group->original_id == id) {
|
||||
@ -1088,7 +1067,7 @@ void TabRestoreServiceHelper::UpdateTabBrowserIDs(SessionID::id_type old_id,
|
||||
tab->browser_id = new_id.id();
|
||||
}
|
||||
|
||||
for (auto& group_pair : window.groups) {
|
||||
for (auto& group_pair : window.tab_groups) {
|
||||
Group& group = *group_pair.second.get();
|
||||
group.browser_id = new_id.id();
|
||||
}
|
||||
|
@ -9,6 +9,8 @@
|
||||
#include <string.h>
|
||||
|
||||
#include <map>
|
||||
#include <memory>
|
||||
#include <optional>
|
||||
#include <utility>
|
||||
#include <vector>
|
||||
|
||||
@ -32,6 +34,7 @@
|
||||
#include "components/sessions/core/session_command.h"
|
||||
#include "components/sessions/core/session_constants.h"
|
||||
#include "components/sessions/core/session_id.h"
|
||||
#include "components/sessions/core/tab_restore_types.h"
|
||||
#include "components/tab_groups/tab_group_color.h"
|
||||
#include "components/tab_groups/tab_group_id.h"
|
||||
#include "components/tab_groups/tab_group_visual_data.h"
|
||||
@ -1418,13 +1421,22 @@ bool TabRestoreServiceImpl::PersistenceDelegate::ConvertSessionWindowToWindow(
|
||||
tab_restore::Window* window) {
|
||||
window->type = session_window->type;
|
||||
|
||||
// The group visual datas must be stored in both |window| and each
|
||||
// grouped tab.
|
||||
std::map<tab_groups::TabGroupId, tab_groups::TabGroupVisualData>
|
||||
group_visual_datas;
|
||||
// The groups in ` window`. The group visual data must also be explicitly set
|
||||
// on grouped tabs.
|
||||
std::map<tab_groups::TabGroupId, std::unique_ptr<tab_restore::Group>> groups;
|
||||
for (auto& tab_group : session_window->tab_groups) {
|
||||
auto group_id = tab_group->id;
|
||||
group_visual_datas[group_id] = tab_group->visual_data;
|
||||
auto group = std::make_unique<sessions::tab_restore::Group>();
|
||||
|
||||
group->group_id = tab_group->id;
|
||||
if (tab_group->saved_guid.has_value()) {
|
||||
group->saved_group_id =
|
||||
base::Uuid::ParseLowercase(tab_group->saved_guid.value());
|
||||
}
|
||||
group->visual_data = tab_group->visual_data;
|
||||
group->browser_id = session_window->window_id.id();
|
||||
group->timestamp = base::Time::Now();
|
||||
groups[group_id] = std::move(group);
|
||||
}
|
||||
|
||||
for (auto& i : session_window->tabs) {
|
||||
@ -1437,7 +1449,7 @@ bool TabRestoreServiceImpl::PersistenceDelegate::ConvertSessionWindowToWindow(
|
||||
auto group_id = i->group;
|
||||
if (group_id.has_value()) {
|
||||
tab.group = group_id;
|
||||
tab.group_visual_data = group_visual_datas[group_id.value()];
|
||||
tab.group_visual_data = groups[group_id.value()]->visual_data;
|
||||
}
|
||||
|
||||
tab.pinned = i->pinned;
|
||||
@ -1451,7 +1463,7 @@ bool TabRestoreServiceImpl::PersistenceDelegate::ConvertSessionWindowToWindow(
|
||||
if (window->tabs.empty()) {
|
||||
return false;
|
||||
}
|
||||
window->tab_groups = std::move(group_visual_datas);
|
||||
window->tab_groups = std::move(groups);
|
||||
window->selected_tab_index =
|
||||
std::min(session_window->selected_tab_index,
|
||||
static_cast<int>(window->tabs.size() - 1));
|
||||
|
@ -174,18 +174,13 @@ struct SESSIONS_EXPORT Window : public Entry {
|
||||
// Type of window.
|
||||
sessions::SessionWindow::WindowType type;
|
||||
|
||||
// TODO(crbug.com/333425400): `tabs`, `groups`, and `tab_groups` contain
|
||||
// duplicated data. To prevent duplication of data consider changing `tabs`
|
||||
// to std::vector<std::unique_ptr<Entries>> so all data can be stored
|
||||
// together in one object. This should prevent data duplication.
|
||||
// The tabs that comprised the window, in order.
|
||||
std::vector<std::unique_ptr<Tab>> tabs;
|
||||
|
||||
// Tab groups in this window including their tabs.
|
||||
std::map<tab_groups::TabGroupId, std::unique_ptr<Group>> groups;
|
||||
|
||||
// Tab group data.
|
||||
std::map<tab_groups::TabGroupId, tab_groups::TabGroupVisualData> tab_groups;
|
||||
// The tab groups in the window. These are only used to query properties about
|
||||
// a group such as visual data, collapsed state, and saved state. As such,
|
||||
// groups in this structure should NOT contain any tabs.
|
||||
std::map<tab_groups::TabGroupId, std::unique_ptr<Group>> tab_groups;
|
||||
|
||||
// Index of the selected tab.
|
||||
int selected_tab_index = -1;
|
||||
|
Reference in New Issue
Block a user