0

Introduce remove_reason enum in TabStripModelChange

In order to consider cases for ClosedTabCache feature separately,
turn bool 'will_be_deleted' into enum 'remove_reason', rename
TabStripModelChange::ContentsWithIndexAndWillBeDeleted to
TabStripModelChange::RemovedTab and use 'remove_reason' in it.

The following values have been introduced in enum RemoveReason:
- kDeleted to indicate that the WebContents will be deleted.
- kCached to indicate that WebContents will be stored in ClosedTabCache
  for some amount of time.
- kInsertedIntoOtherTabStrip to indicate that WebContents got detached
  from a TabStrip and inserted into another TabStrip.

Bug: 1149549
Change-Id: I715f61a8d1dbb4100d4a4cecdf91fba2ae0375c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2782442
Reviewed-by: Sreeja Kamishetty <sreejakshetty@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Sreeja Kamishetty <sreejakshetty@chromium.org>
Cr-Commit-Position: refs/heads/master@{#891184}
This commit is contained in:
Tobias Soppa
2021-06-10 11:55:30 +00:00
committed by Chromium LUCI CQ
parent fc5fd4f09a
commit c96e396ae3
14 changed files with 53 additions and 33 deletions

@ -1088,6 +1088,7 @@ Timo Gurr <timo.gurr@gmail.com>
Timo Reimann <ttr314@googlemail.com>
Timo Witte <timo.witte@gmail.com>
Ting Shao <ting.shao@intel.com>
Tobias Soppa <tobias@soppa.me>
Tom Callaway <tcallawa@redhat.com>
Tom Harwood <tfh@skip.org>
Tomas Popela <tomas.popela@gmail.com>

@ -205,7 +205,8 @@ void TabsEventRouter::OnTabStripModelChanged(
}
case TabStripModelChange::kRemoved: {
for (const auto& contents : change.GetRemove()->contents) {
if (contents.will_be_deleted) {
if (contents.remove_reason ==
TabStripModelChange::RemoveReason::kDeleted) {
DispatchTabClosingAt(tab_strip_model, contents.contents,
contents.index);
}

@ -38,7 +38,7 @@ void AudibleContentsTracker::OnTabStripModelChanged(
const TabStripSelectionChange& selection) {
if (change.type() == TabStripModelChange::kRemoved) {
for (const auto& contents : change.GetRemove()->contents) {
if (contents.will_be_deleted)
if (contents.remove_reason == TabStripModelChange::RemoveReason::kDeleted)
RemoveAudibleWebContents(contents.contents);
}
} else if (change.type() == TabStripModelChange::kReplaced) {

@ -176,7 +176,9 @@ class TabClosingObserver : public TabStripModelObserver {
auto* remove = change.GetRemove();
for (const auto& contents : remove->contents) {
if (contents_ == contents.contents && contents.will_be_deleted) {
if (contents_ == contents.contents &&
contents.remove_reason ==
TabStripModelChange::RemoveReason::kDeleted) {
if (run_loop_.running())
run_loop_.Quit();
contents_ = nullptr;

@ -234,7 +234,7 @@ void BrowserStatusMonitor::OnTabStripModelChanged(
} else if (change.type() == TabStripModelChange::kRemoved) {
auto* remove = change.GetRemove();
for (const auto& contents : remove->contents) {
if (contents.will_be_deleted)
if (contents.remove_reason == TabStripModelChange::RemoveReason::kDeleted)
OnTabClosing(contents.contents);
}
} else if (change.type() == TabStripModelChange::kReplaced) {

@ -1150,7 +1150,8 @@ void Browser::OnTabStripModelChanged(TabStripModel* tab_strip_model,
}
case TabStripModelChange::kRemoved: {
for (const auto& contents : change.GetRemove()->contents) {
if (contents.will_be_deleted)
if (contents.remove_reason ==
TabStripModelChange::RemoveReason::kDeleted)
OnTabClosing(contents.contents);
OnTabDetached(contents.contents,
contents.contents == selection.old_contents);

@ -207,7 +207,7 @@ class TabClosingObserver : public TabStripModelObserver {
auto* remove = change.GetRemove();
for (const auto& contents : remove->contents) {
if (contents.will_be_deleted)
if (contents.remove_reason == TabStripModelChange::RemoveReason::kDeleted)
closing_count_ += 1;
}
}

@ -262,16 +262,16 @@ void TabStripModel::WebContentsData::WebContentsDestroyed() {
}
// Holds state for a WebContents that has been detached from the tab strip. Will
// also handle WebContents deletion if |will_delete| is true.
// also handle WebContents deletion if |remove_reason| is kDeleted.
struct TabStripModel::DetachedWebContents {
DetachedWebContents(int index_before_any_removals,
int index_at_time_of_removal,
std::unique_ptr<WebContents> contents,
bool will_delete)
TabStripModelChange::RemoveReason remove_reason)
: contents(std::move(contents)),
index_before_any_removals(index_before_any_removals),
index_at_time_of_removal(index_at_time_of_removal),
will_delete(will_delete) {}
remove_reason(remove_reason) {}
DetachedWebContents(const DetachedWebContents&) = delete;
DetachedWebContents& operator=(const DetachedWebContents&) = delete;
~DetachedWebContents() = default;
@ -289,8 +289,7 @@ struct TabStripModel::DetachedWebContents {
// removed tabs in this batch.
const int index_at_time_of_removal;
// Whether to delete the WebContents after sending notifications.
const bool will_delete;
const TabStripModelChange::RemoveReason remove_reason;
};
// Holds all state necessary to send notifications for detached tabs.
@ -447,7 +446,9 @@ std::unique_ptr<content::WebContents> TabStripModel::DetachWebContentsAt(
std::make_unique<DetachedWebContents>(
index, index,
DetachWebContentsImpl(index, /*create_historical_tab=*/false),
/*will_delete=*/false);
// TODO(https://crbug.com/2564529): Add case for kCached once the
// hooking logic is implemented.
TabStripModelChange::RemoveReason::kInsertedIntoOtherTabStrip);
notifications.detached_web_contents.push_back(std::move(dwc));
SendDetachWebContentsNotifications(&notifications);
return std::move(notifications.detached_web_contents[0]->contents);
@ -519,7 +520,7 @@ void TabStripModel::SendDetachWebContentsNotifications(
for (auto& dwc : notifications->detached_web_contents) {
remove.contents.push_back({dwc->contents.get(),
dwc->index_before_any_removals,
dwc->will_delete});
dwc->remove_reason});
}
TabStripModelChange change(std::move(remove));
@ -544,7 +545,7 @@ void TabStripModel::SendDetachWebContentsNotifications(
}
for (auto& dwc : notifications->detached_web_contents) {
if (dwc->will_delete) {
if (dwc->remove_reason == TabStripModelChange::RemoveReason::kDeleted) {
// This destroys the WebContents, which will also send
// WebContentsDestroyed notifications.
dwc->contents.reset();
@ -1877,7 +1878,7 @@ bool TabStripModel::CloseWebContentses(
original_indices[i], current_index,
DetachWebContentsImpl(current_index,
close_types & CLOSE_CREATE_HISTORICAL_TAB),
/*will_delete=*/true);
TabStripModelChange::RemoveReason::kDeleted);
notifications->detached_web_contents.push_back(std::move(dwc));
}

@ -72,12 +72,12 @@ TabStripModelChange::TabStripModelChange(Type type,
std::unique_ptr<Delta> delta)
: type_(type), delta_(std::move(delta)) {}
void TabStripModelChange::ContentsWithIndexAndWillBeDeleted::WriteIntoTrace(
void TabStripModelChange::RemovedTab::WriteIntoTrace(
perfetto::TracedValue context) const {
auto dict = std::move(context).WriteDictionary();
dict.Add("contents", contents);
dict.Add("index", index);
dict.Add("will_be_deleted", will_be_deleted);
dict.Add("remove_reason", remove_reason);
}
void TabStripModelChange::ContentsWithIndex::WriteIntoTrace(

@ -38,6 +38,21 @@ class TabStripModelChange {
public:
enum Type { kSelectionOnly, kInserted, kRemoved, kMoved, kReplaced };
// Used to specify what will happen with the WebContents after it is removed.
enum class RemoveReason {
// WebContents will be deleted.
kDeleted,
// WebContents will be stored in ClosedTabCache. After some amount of time,
// the WebContents will either be deleted, or inserted back into another
// TabStripModel.
kCached,
// WebContents got detached from a TabStrip and inserted into another
// TabStrip.
kInsertedIntoOtherTabStrip
};
// Base class for all changes.
// TODO(dfried): would love to change this whole thing into a std::variant,
// but C++17 features are not yet approved for use in chromium.
@ -47,16 +62,12 @@ class TabStripModelChange {
virtual void WriteIntoTrace(perfetto::TracedValue context) const = 0;
};
struct ContentsWithIndexAndWillBeDeleted {
struct RemovedTab {
void WriteIntoTrace(perfetto::TracedValue context) const;
content::WebContents* contents;
int index;
// The specified WebContents are being closed (and eventually destroyed).
// TODO(https://crbug.com/1149549): Make will_be_deleted into enum to
// consider the case for ClosedTabCache feature separtely.
bool will_be_deleted;
void WriteIntoTrace(perfetto::TracedValue context) const;
RemoveReason remove_reason;
};
struct ContentsWithIndex {
@ -110,7 +121,7 @@ class TabStripModelChange {
Remove& operator=(Remove&& other);
// Contains the list of web contents removed with their indexes at
// the time of removal along with flag |will_be_deleted| that indicates if
// the time of removal along with flag |remove_reason| that indicates if
// the web contents will be deleted or not after removing. For example, if
// we removed elements:
//
@ -133,7 +144,7 @@ class TabStripModelChange {
// them in the order the web contents appear in |contents|. Observers should
// not do index-based queries based on their own internally-stored indices
// until after processing all of |contents|.
std::vector<ContentsWithIndexAndWillBeDeleted> contents;
std::vector<RemovedTab> contents;
void WriteIntoTrace(perfetto::TracedValue context) const override;
};

@ -146,7 +146,7 @@ void TabStripModelStatsRecorder::OnTabStripModelChanged(
const TabStripSelectionChange& selection) {
if (change.type() == TabStripModelChange::kRemoved) {
for (const auto& contents : change.GetRemove()->contents) {
if (contents.will_be_deleted)
if (contents.remove_reason == TabStripModelChange::RemoveReason::kDeleted)
OnTabClosing(contents.contents);
}
} else if (change.type() == TabStripModelChange::kReplaced) {

@ -251,7 +251,8 @@ class MockTabStripModelObserver : public TabStripModelObserver {
}
case TabStripModelChange::kRemoved: {
for (const auto& contents : change.GetRemove()->contents) {
if (contents.will_be_deleted)
if (contents.remove_reason ==
TabStripModelChange::RemoveReason::kDeleted)
PushCloseState(contents.contents, contents.index);
PushDetachState(contents.contents, contents.index,
selection.old_contents == contents.contents);

@ -50,7 +50,8 @@ void ActiveTabTracker::OnTabStripModelChanged(
// Ignore if the tab isn't being closed (this would happen if it were
// dragged to a different tab strip).
for (const auto& contents : remove->contents) {
if (contents.will_be_deleted &&
if (contents.remove_reason ==
TabStripModelChange::RemoveReason::kDeleted &&
contents.index == prev_active_tab_index) {
active_tab_closed_callback_.Run(
model, clock_->NowTicks() - active_tab_changed_times_[model]);

@ -203,9 +203,10 @@ void WebAppMetrics::OnTabStripModelChanged(
}
if (change.type() == TabStripModelChange::kRemoved) {
for (const TabStripModelChange::ContentsWithIndexAndWillBeDeleted&
contents : change.GetRemove()->contents) {
if (contents.will_be_deleted) {
for (const TabStripModelChange::RemovedTab& contents :
change.GetRemove()->contents) {
if (contents.remove_reason ==
TabStripModelChange::RemoveReason::kDeleted) {
auto* tab_helper =
WebAppTabHelperBase::FromWebContents(contents.contents);
if (tab_helper && !tab_helper->GetAppId().empty())