Skip to content

GitLab

T
tangled
Commit 2c262046 authored by Dana Fried's avatar Dana Fried Committed by Commit Bot
Browse files
Options

Inserting a tab with a group assigned triggers group change event. 

This allows proper UI update (color change) on tabs that have been added
to a group.

Had to remove the unit test ensuring contiguity across all update
notifications, since it's no longer a valid assumption.

Bug: 976502
Change-Id: I260eb4fb5128079068f7750b882dcb1d749eceed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1670281
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: default avatarBret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671489}
parent fa82758e
Hide whitespace changes
Inline Side-by-side
Showing with 27 additions and 212 deletions
chrome/browser/ui/tabs/tab_strip_model.cc
View file @ 2c262046
...@@ -1472,6 +1472,8 @@ int TabStripModel::InsertWebContentsAtImpl( ...@@ -1472,6 +1472,8 @@ int TabStripModel::InsertWebContentsAtImpl(
} }
data->set_opener(active_contents); data->set_opener(active_contents);
} }
if (group.has_value())
data->set_group(group);
// TODO(gbillock): Ask the modal dialog manager whether the WebContents should // TODO(gbillock): Ask the modal dialog manager whether the WebContents should
// be blocked, or just let the modal dialog manager make the blocking call // be blocked, or just let the modal dialog manager make the blocking call
...@@ -1495,14 +1497,13 @@ int TabStripModel::InsertWebContentsAtImpl( ...@@ -1495,14 +1497,13 @@ int TabStripModel::InsertWebContentsAtImpl(
/*triggered_by_other_operation=*/true); /*triggered_by_other_operation=*/true);
} }
if (group.has_value())
contents_data_[index]->set_group(group);
TabStripModelChange::Insert insert; TabStripModelChange::Insert insert;
insert.contents.push_back({raw_contents, index}); insert.contents.push_back({raw_contents, index});
TabStripModelChange change(std::move(insert)); TabStripModelChange change(std::move(insert));
for (auto& observer : observers_) for (auto& observer : observers_)
observer.OnTabStripModelChanged(this, change, selection); observer.OnTabStripModelChanged(this, change, selection);
if (group.has_value())
NotifyGroupChange(index, base::nullopt, group);
return index; return index;
} }
......
chrome/browser/ui/tabs/tab_strip_model_unittest.cc
View file @ 2c262046
...@@ -3230,6 +3230,29 @@ TEST_F(TabStripModelTest, CloseTabNotifiesObserversOfGroupChange) { ...@@ -3230,6 +3230,29 @@ TEST_F(TabStripModelTest, CloseTabNotifiesObserversOfGroupChange) {
EXPECT_EQ(num_group_changed_notifications, 1); EXPECT_EQ(num_group_changed_notifications, 1);
} }
TEST_F(TabStripModelTest, InsertWebContentsAtWithGroupNotifiesObservers) {
TestTabStripModelDelegate delegate;
MockTabStripModelObserver observer;
TabStripModel strip(&delegate, profile());
strip.AddObserver(&observer);
strip.AppendWebContents(CreateWebContents(), true);
strip.AppendWebContents(CreateWebContents(), false);
auto group_id = strip.AddToNewGroup({0, 1});
observer.ClearStates();
strip.InsertWebContentsAt(1, CreateWebContents(), TabStripModel::ADD_NONE,
group_id);
EXPECT_EQ(observer.GetStateCount(), 2);
EXPECT_EQ(MockTabStripModelObserver::GROUP_CHANGED,
observer.GetStateAt(1).action);
EXPECT_TRUE(observer.StateEquals(
1, ExpectedGroupChangeState(strip, 1, base::nullopt, group_id)));
strip.CloseAllTabs();
}
// When inserting a WebContents, if a group is not specified, the new tab // When inserting a WebContents, if a group is not specified, the new tab
// should be left ungrouped. // should be left ungrouped.
TEST_F(TabStripModelTest, InsertWebContentsAtDoesNotGroupByDefault) { TEST_F(TabStripModelTest, InsertWebContentsAtDoesNotGroupByDefault) {
...@@ -3289,212 +3312,3 @@ TEST_F(TabStripModelTest, InsertWebContentsAtWithPinnedGroupPins) { ...@@ -3289,212 +3312,3 @@ TEST_F(TabStripModelTest, InsertWebContentsAtWithPinnedGroupPins) {
strip.CloseAllTabs(); strip.CloseAllTabs();
} }
namespace {
// Checks whether a sequence of tabs have contiguous groups. |group_per_tab|
// defines the group ID for each tab in the sequence.
bool GroupsAreContiguous(
base::span<const base::Optional<TabGroupId>> group_per_tab) {
base::flat_set<TabGroupId> seen_groups;
base::Optional<TabGroupId> last_seen_group;
for (int tab_index = 0; tab_index < static_cast<int>(group_per_tab.size());
++tab_index) {
// If the current tab is grouped and its group ID is not the same as the
// last seen tab, we have found a new or discontiguous group. Checking
// |seen_groups| tells us which.
if (group_per_tab[tab_index].has_value() &&
group_per_tab[tab_index] != last_seen_group) {
if (base::Contains(seen_groups, group_per_tab[tab_index].value()))
return false;
seen_groups.insert(group_per_tab[tab_index].value());
}
last_seen_group = group_per_tab[tab_index];
}
return true;
}
// Printing a base::Optional<TabGroupId> gives absolutely unreadable
// results. For more readable output, this assigns successive integers to each
// seen group, starting at 1. It prints those to the string, substituting "*"
// for non-grouped tabs. Sample output: "* 1 1 * * 2 2 2"
std::string PrintGroupsToString(
base::span<const base::Optional<TabGroupId>> group_per_tab) {
if (group_per_tab.empty())
return "";
int next_group = 1;
base::flat_map<TabGroupId, int> group_to_output;
std::string result;
for (const base::Optional<TabGroupId>& cur : group_per_tab) {
if (!cur.has_value()) {
result += "* ";
continue;
}
if (!base::Contains(group_to_output, cur.value()))
group_to_output[cur.value()] = next_group++;
result += base::NumberToString(group_to_output[cur.value()]) + " ";
}
result.pop_back();
return result;
}
class TabStripGroupContiguityChecker : public TabStripModelObserver {
public:
explicit TabStripGroupContiguityChecker(TabStripModel* model)
: model_(model) {
// This must be used on a TabStripModel with no tabs so we can accurately
// track the group of each tab at each observer update.
EXPECT_EQ(0, model_->count());
model_->AddObserver(this);
}
~TabStripGroupContiguityChecker() override { model_->RemoveObserver(this); }
void OnTabStripModelChanged(
TabStripModel* model,
const TabStripModelChange& change,
const TabStripSelectionChange& selection) override {
ASSERT_EQ(model_, model);
// Update our list of tabs' groups, |group_of_tab_|, according to the
// observer notification.
switch (change.type()) {
case TabStripModelChange::kInserted: {
const TabStripModelChange::Insert* insert = change.GetInsert();
for (const TabStripModelChange::ContentsWithIndex& data :
insert->contents) {
ASSERT_LE(data.index, static_cast<int>(group_of_tab_.size()));
group_of_tab_.insert(group_of_tab_.begin() + data.index,
model_->GetTabGroupForTab(data.index));
}
break;
}
case TabStripModelChange::kRemoved: {
const TabStripModelChange::Remove* remove = change.GetRemove();
for (const TabStripModelChange::ContentsWithIndex& data :
remove->contents) {
ASSERT_LT(data.index, static_cast<int>(group_of_tab_.size()));
group_of_tab_.erase(group_of_tab_.begin() + data.index);
}
break;
}
case TabStripModelChange::kMoved: {
const TabStripModelChange::Move* move = change.GetMove();
ASSERT_LT(move->from_index, static_cast<int>(group_of_tab_.size()));
ASSERT_LT(move->to_index, static_cast<int>(group_of_tab_.size()));
auto from_iter = group_of_tab_.begin() + move->from_index;
auto to_iter = group_of_tab_.begin() + move->to_index;
if (from_iter <= to_iter) {
std::rotate(from_iter, from_iter + 1, to_iter + 1);
} else {
std::rotate(to_iter, from_iter, from_iter + 1);
}
break;
}
case TabStripModelChange::kGroupChanged: {
const TabStripModelChange::GroupChange* group_change =
change.GetGroupChange();
ASSERT_LT(group_change->index, static_cast<int>(group_of_tab_.size()));
EXPECT_EQ(group_change->old_group, group_of_tab_[group_change->index]);
group_of_tab_[group_change->index] = group_change->new_group;
break;
}
case TabStripModelChange::kSelectionOnly:
break;
case TabStripModelChange::kReplaced:
break;
default:
NOTREACHED();
break;
}
EXPECT_TRUE(GroupsAreContiguous(group_of_tab_))
<< "Groups: " << PrintGroupsToString(group_of_tab_);
}
private:
TabStripModel* const model_;
std::vector<base::Optional<TabGroupId>> group_of_tab_;
};
} // namespace
// Group contiguity is an invariant of the TabStripModel. It should also send
// observer updates in a way that preserves this invariant. Otherwise,
// reconstructing the TabStripModel state from a sequence of observer
// notifications could result in an invalid state. This is important for
// sessions code.
//
// TODO(crbug.com/971491): this doesn't test all cases because some cases do
// result in an inconsistent state. For example, removing a tab in the middle of
// a group sends the ungroup update before the remove update. Once this is
// fixed, this test should be updated for full coverage.
TEST_F(TabStripModelTest, GroupsAreAlwaysContiguousForObservers) {
TestTabStripModelDelegate delegate;
TabStripModel strip(&delegate, profile());
// This observers |strip|. On each update it receives, it updates its internal
// model of the tab strip and checks that all groups are contiguous in it.
TabStripGroupContiguityChecker checker(&strip);
// Add a few tabs to start out.
for (int i = 0; i < 6; ++i)
strip.AppendWebContents(CreateWebContents(), true);
// Since failures will happen from inside |checker|, we use |SCOPED_TRACE()|
// to see where they come from.
base::Optional<TabGroupId> group1, group2;
{
SCOPED_TRACE("Creating initial groups");
// Do some grouping, getting some tab reordering along the way.
group1 = strip.AddToNewGroup({1, 2});
group2 = strip.AddToNewGroup({4, 5});
strip.AddToExistingGroup({3}, group1.value());
strip.AddToExistingGroup({0}, group2.value());
}
// Groups: 1 1 1 2 2 2
{
ASSERT_EQ(6, strip.count());
SCOPED_TRACE("Remove tab at group beginning");
strip.DetachWebContentsAt(3);
}
// Groups: 1 1 1 2 2
{
ASSERT_EQ(5, strip.count());
SCOPED_TRACE("Insert ungrouped tab between groups");
strip.InsertWebContentsAt(3, CreateWebContents(), TabStripModel::ADD_NONE);
}
// Groups: 1 1 1 * 2 2
{
ASSERT_EQ(6, strip.count());
SCOPED_TRACE("Move ungrouped tab from between groups to end of group");
strip.MoveWebContentsAt(3, 5, false);
}
// Groups: 1 1 1 2 2 *
{
ASSERT_EQ(6, strip.count());
SCOPED_TRACE("Insert grouped tab into middle of group");
strip.InsertWebContentsAt(4, CreateWebContents(), TabStripModel::ADD_NONE,
group2);
}
// Groups: 1 1 1 2 2 2 *
// The following test currently fails, but at some point should pass:
//
// {
// ASSERT_EQ(7, strip.count());
// SCOPED_TRACE("Remove tab in middle of group");
// strip.DetachWebContentsAt(1);
// }
// // Groups: 1 1 2 2 2 *
strip.CloseAllTabs();
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment