TabStripModelObserver: clean up on destruction
This change has TabStripModelObserver remember which TabStripModels it
is observing, and unregister from those if necessary on destruction. It
also removes instances of now-unnecessary cleanup code in subclass
destructors.
This change also introduces a new observable event on
TabStripModelObserver: OnTabStripModelDestroyed. This event
notifies observers that the model is being destroyed so
they can do any necessary cleanup. In the future most uses
of TabStripModelObserver::TabStripEmpty() should migrate to
OnTabStripModelDestroyed().
Bug: 991308
Change-Id: Ifed459ec4d88136db4a00e4386dc6c2095048b46
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1739552
Commit-Queue: Elly Fong-Jones <[email protected]>
Auto-Submit: Elly Fong-Jones <[email protected]>
Reviewed-by: Nico Weber <[email protected]>
Cr-Commit-Position: refs/heads/master@{#686075}
diff --git a/chrome/browser/apps/platform_apps/app_browsertest.cc b/chrome/browser/apps/platform_apps/app_browsertest.cc
index d96e2642..d3e0a73 100644
--- a/chrome/browser/apps/platform_apps/app_browsertest.cc
+++ b/chrome/browser/apps/platform_apps/app_browsertest.cc
@@ -101,7 +101,7 @@
public:
TabsAddedNotificationObserver(Browser* browser, size_t observations)
: observations_(observations) {
- tab_strip_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
~TabsAddedNotificationObserver() override = default;
@@ -129,8 +129,6 @@
base::RunLoop run_loop_;
size_t observations_;
std::vector<content::WebContents*> observed_tabs_;
- ScopedObserver<TabStripModel, TabStripModelObserver> tab_strip_observer_{
- this};
DISALLOW_COPY_AND_ASSIGN(TabsAddedNotificationObserver);
};
diff --git a/chrome/browser/captive_portal/captive_portal_browsertest.cc b/chrome/browser/captive_portal/captive_portal_browsertest.cc
index 86715b2..1e6e520d 100644
--- a/chrome/browser/captive_portal/captive_portal_browsertest.cc
+++ b/chrome/browser/captive_portal/captive_portal_browsertest.cc
@@ -515,8 +515,8 @@
class TabActivationWaiter : public TabStripModelObserver {
public:
explicit TabActivationWaiter(TabStripModel* tab_strip_model)
- : number_of_unconsumed_active_tab_changes_(0), scoped_observer_(this) {
- scoped_observer_.Add(tab_strip_model);
+ : number_of_unconsumed_active_tab_changes_(0) {
+ tab_strip_model->AddObserver(this);
}
void WaitForActiveTabChange() {
@@ -548,7 +548,6 @@
private:
scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
int number_of_unconsumed_active_tab_changes_;
- ScopedObserver<TabStripModel, TabActivationWaiter> scoped_observer_;
DISALLOW_COPY_AND_ASSIGN(TabActivationWaiter);
};
diff --git a/chrome/browser/download/download_frame_policy_browsertest.cc b/chrome/browser/download/download_frame_policy_browsertest.cc
index 33ef238..064d6ed 100644
--- a/chrome/browser/download/download_frame_policy_browsertest.cc
+++ b/chrome/browser/download/download_frame_policy_browsertest.cc
@@ -99,8 +99,8 @@
PopupPageLoadMetricsWaiterInitializer(
TabStripModel* tab_strip_model,
std::unique_ptr<page_load_metrics::PageLoadMetricsTestWaiter>* waiter)
- : waiter_(waiter), scoped_observer_(this) {
- scoped_observer_.Add(tab_strip_model);
+ : waiter_(waiter) {
+ tab_strip_model->AddObserver(this);
}
void OnTabStripModelChanged(
@@ -117,8 +117,6 @@
private:
std::unique_ptr<page_load_metrics::PageLoadMetricsTestWaiter>* waiter_;
- ScopedObserver<TabStripModel, PopupPageLoadMetricsWaiterInitializer>
- scoped_observer_;
DISALLOW_COPY_AND_ASSIGN(PopupPageLoadMetricsWaiterInitializer);
};
diff --git a/chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc b/chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc
index 2e1bab0..73e03b3 100644
--- a/chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc
+++ b/chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc
@@ -431,10 +431,6 @@
browser_->tab_strip_model()->AddObserver(this);
}
- ~IndicatorChangeObserver() override {
- browser_->tab_strip_model()->RemoveObserver(this);
- }
-
TabAlertState last_alert_state() const { return last_alert_state_; }
void TabChangedAt(content::WebContents* contents,
diff --git a/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc b/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
index ac34f48..2110bfc 100644
--- a/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
+++ b/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
@@ -76,7 +76,7 @@
delay_url_(delay_url),
until_url_suffix_(until_url_suffix),
script_(script) {
- tab_strip_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
~DelayLoadStartAndExecuteJavascript() override {}
@@ -91,7 +91,7 @@
content::WebContentsObserver::Observe(
change.GetInsert()->contents[0].contents);
- tab_strip_observer_.RemoveAll();
+ tab_strip_model->RemoveObserver(this);
}
// WebContentsObserver:
@@ -165,8 +165,6 @@
base::WeakPtr<WillStartRequestObserverThrottle> throttle_;
- ScopedObserver<TabStripModel, TabStripModelObserver> tab_strip_observer_{
- this};
GURL delay_url_;
std::string until_url_suffix_;
std::string script_;
diff --git a/chrome/browser/extensions/extension_unload_browsertest.cc b/chrome/browser/extensions/extension_unload_browsertest.cc
index 0de3b152..8fa6d34c 100644
--- a/chrome/browser/extensions/extension_unload_browsertest.cc
+++ b/chrome/browser/extensions/extension_unload_browsertest.cc
@@ -37,8 +37,8 @@
class TestTabStripModelObserver : public TabStripModelObserver {
public:
explicit TestTabStripModelObserver(TabStripModel* model)
- : model_(model), desired_count_(0), scoped_observer_(this) {
- scoped_observer_.Add(model);
+ : model_(model), desired_count_(0) {
+ model->AddObserver(this);
}
~TestTabStripModelObserver() override = default;
@@ -62,7 +62,6 @@
TabStripModel* model_;
int desired_count_;
base::RunLoop run_loop_;
- ScopedObserver<TabStripModel, TabStripModelObserver> scoped_observer_;
DISALLOW_COPY_AND_ASSIGN(TestTabStripModelObserver);
};
diff --git a/chrome/browser/external_protocol/external_protocol_handler_browsertest.cc b/chrome/browser/external_protocol/external_protocol_handler_browsertest.cc
index 5a6c854..91af2d5c 100644
--- a/chrome/browser/external_protocol/external_protocol_handler_browsertest.cc
+++ b/chrome/browser/external_protocol/external_protocol_handler_browsertest.cc
@@ -18,9 +18,8 @@
// Observe that the tab is created then automatically closed.
class TabAddedRemovedObserver : public TabStripModelObserver {
public:
- explicit TabAddedRemovedObserver(TabStripModel* tab_strip_model)
- : scoped_observer_(this) {
- scoped_observer_.Add(tab_strip_model);
+ explicit TabAddedRemovedObserver(TabStripModel* tab_strip_model) {
+ tab_strip_model->AddObserver(this);
}
void OnTabStripModelChanged(
@@ -50,7 +49,6 @@
bool inserted_ = false;
bool removed_ = false;
base::RunLoop loop_;
- ScopedObserver<TabStripModel, TabAddedRemovedObserver> scoped_observer_;
};
IN_PROC_BROWSER_TEST_F(ExternalProtocolHandlerBrowserTest,
diff --git a/chrome/browser/media/cast_mirroring_service_host_browsertest.cc b/chrome/browser/media/cast_mirroring_service_host_browsertest.cc
index 0f90d53..b8cb1b2 100644
--- a/chrome/browser/media/cast_mirroring_service_host_browsertest.cc
+++ b/chrome/browser/media/cast_mirroring_service_host_browsertest.cc
@@ -272,10 +272,6 @@
browser_->tab_strip_model()->AddObserver(this);
}
- ~IndicatorChangeObserver() override {
- browser_->tab_strip_model()->RemoveObserver(this);
- }
-
TabAlertState last_alert_state() const { return last_alert_state_; }
void TabChangedAt(content::WebContents* contents,
diff --git a/chrome/browser/metrics/tab_stats_tracker.cc b/chrome/browser/metrics/tab_stats_tracker.cc
index a4424f6..aa006fa4 100644
--- a/chrome/browser/metrics/tab_stats_tracker.cc
+++ b/chrome/browser/metrics/tab_stats_tracker.cc
@@ -213,11 +213,7 @@
TabStatsTracker::~TabStatsTracker() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- BrowserList* browser_list = BrowserList::GetInstance();
- for (Browser* browser : *browser_list)
- browser->tab_strip_model()->RemoveObserver(this);
-
- browser_list->RemoveObserver(this);
+ BrowserList::GetInstance()->RemoveObserver(this);
base::PowerMonitor::RemoveObserver(this);
}
diff --git a/chrome/browser/prerender/prerender_browsertest.cc b/chrome/browser/prerender/prerender_browsertest.cc
index 8824c094..4fc5122 100644
--- a/chrome/browser/prerender/prerender_browsertest.cc
+++ b/chrome/browser/prerender/prerender_browsertest.cc
@@ -451,7 +451,7 @@
NewTabNavigationOrSwapObserver() {
BrowserList::AddObserver(this);
for (const Browser* browser : *BrowserList::GetInstance())
- tab_strip_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
~NewTabNavigationOrSwapObserver() override {
@@ -481,12 +481,10 @@
// BrowserListObserver:
void OnBrowserAdded(Browser* browser) override {
- tab_strip_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
private:
- ScopedObserver<TabStripModel, TabStripModelObserver> tab_strip_observer_{
- this};
base::RunLoop new_tab_run_loop_;
std::unique_ptr<NavigationOrSwapObserver> swap_observer_;
diff --git a/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc b/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc
index a59019e..6e82c9c 100644
--- a/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc
@@ -150,7 +150,7 @@
public TabStripModelObserver {
public:
explicit TestNavigationObserverManager(Browser* browser) {
- observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
// TabStripModelObserver:
@@ -175,7 +175,6 @@
~TestNavigationObserverManager() override = default;
private:
- ScopedObserver<TabStripModel, TabStripModelObserver> observer_{this};
std::vector<std::unique_ptr<SafeBrowsingNavigationObserver>> observer_list_;
DISALLOW_COPY_AND_ASSIGN(TestNavigationObserverManager);
diff --git a/chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc b/chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc
index 1da0835..f878398 100644
--- a/chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc
+++ b/chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc
@@ -164,8 +164,6 @@
tab_strip_->AddObserver(this);
}
- ~TabClosingObserver() override { tab_strip_->RemoveObserver(this); }
-
void WaitForContentsClosing() {
if (!contents_)
return;
diff --git a/chrome/browser/sync/sessions/browser_list_router_helper.cc b/chrome/browser/sync/sessions/browser_list_router_helper.cc
index 19083cc..6ce0521 100644
--- a/chrome/browser/sync/sessions/browser_list_router_helper.cc
+++ b/chrome/browser/sync/sessions/browser_list_router_helper.cc
@@ -26,13 +26,6 @@
}
BrowserListRouterHelper::~BrowserListRouterHelper() {
- BrowserList* browser_list = BrowserList::GetInstance();
- for (Browser* browser : *browser_list) {
- if (browser->profile() == profile_) {
- browser->tab_strip_model()->RemoveObserver(this);
- }
- }
-
BrowserList::GetInstance()->RemoveObserver(this);
}
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 750609e..fb259dc9 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -526,7 +526,6 @@
// The tab strip should not have any tabs at this point.
DCHECK(tab_strip_model_->empty());
- tab_strip_model_->RemoveObserver(this);
bubble_manager_.reset();
// Destroy the BrowserCommandController before removing the browser, so that
diff --git a/chrome/browser/ui/search/instant_controller.cc b/chrome/browser/ui/search/instant_controller.cc
index e146612..b8fc888 100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -41,8 +41,8 @@
InstantController::InstantController(Profile* profile,
TabStripModel* tab_strip_model)
- : profile_(profile), tab_strip_observer_(this) {
- tab_strip_observer_.Add(tab_strip_model);
+ : profile_(profile) {
+ tab_strip_model->AddObserver(this);
}
InstantController::~InstantController() = default;
diff --git a/chrome/browser/ui/search/instant_controller.h b/chrome/browser/ui/search/instant_controller.h
index f6e5c786..aabfb16b 100644
--- a/chrome/browser/ui/search/instant_controller.h
+++ b/chrome/browser/ui/search/instant_controller.h
@@ -9,7 +9,6 @@
#include "base/gtest_prod_util.h"
#include "base/macros.h"
-#include "base/scoped_observer.h"
#include "build/build_config.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
@@ -54,7 +53,6 @@
void UpdateInfoForInstantTab();
Profile* const profile_;
- ScopedObserver<TabStripModel, TabStripModelObserver> tab_strip_observer_;
// Observes the currently active tab, and calls us back if it becomes an NTP.
std::unique_ptr<TabObserver> tab_observer_;
diff --git a/chrome/browser/ui/tabs/pinned_tab_service.cc b/chrome/browser/ui/tabs/pinned_tab_service.cc
index 29e862bb..cf103eb2 100644
--- a/chrome/browser/ui/tabs/pinned_tab_service.cc
+++ b/chrome/browser/ui/tabs/pinned_tab_service.cc
@@ -41,7 +41,7 @@
// over-writing the correct state.
// Saving is re-enabled if a new tab or window is opened.
DCHECK_EQ(type, chrome::NOTIFICATION_CLOSE_ALL_BROWSERS_REQUEST);
- if (observed_tab_strips_.IsObservingSources())
+ if (TabStripModelObserver::IsObservingAny(this))
WritePinnedTabsIfNecessary();
}
@@ -50,14 +50,14 @@
return;
need_to_write_pinned_tabs_ = true;
- observed_tab_strips_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
void PinnedTabService::OnBrowserClosing(Browser* browser) {
if (browser->profile() != profile_ || !browser->is_type_tabbed())
return;
- if (observed_tab_strips_.GetSourcesCount() == 1U)
+ if (TabStripModelObserver::CountObservedModels(this) == 1)
WritePinnedTabsIfNecessary();
}
@@ -65,14 +65,14 @@
if (browser->profile() != profile_ || !browser->is_type_tabbed())
return;
- observed_tab_strips_.Remove(browser->tab_strip_model());
+ browser->tab_strip_model()->RemoveObserver(this);
// This happens when user closes each tabs manually via the close button on
// them. In this case OnBrowserClosing() above is not called. This causes
// pinned tabs to repopen on the next startup. So we should call
// WritePinnedTab() to clear the data.
// http://crbug.com/71939
- if (!observed_tab_strips_.IsObservingSources())
+ if (!TabStripModelObserver::IsObservingAny(this))
WritePinnedTabsIfNecessary();
}
diff --git a/chrome/browser/ui/tabs/pinned_tab_service.h b/chrome/browser/ui/tabs/pinned_tab_service.h
index 2d624c0..11c482f 100644
--- a/chrome/browser/ui/tabs/pinned_tab_service.h
+++ b/chrome/browser/ui/tabs/pinned_tab_service.h
@@ -61,10 +61,6 @@
ScopedObserver<BrowserList, BrowserListObserver> browser_list_observer_{this};
- // |this| observes all tabbed browsers that match |profile_|.
- ScopedObserver<TabStripModel, TabStripModelObserver> observed_tab_strips_{
- this};
-
DISALLOW_COPY_AND_ASSIGN(PinnedTabService);
};
diff --git a/chrome/browser/ui/tabs/tab_strip_model.cc b/chrome/browser/ui/tabs/tab_strip_model.cc
index 00bb862..63eba3b 100644
--- a/chrome/browser/ui/tabs/tab_strip_model.cc
+++ b/chrome/browser/ui/tabs/tab_strip_model.cc
@@ -327,6 +327,10 @@
}
TabStripModel::~TabStripModel() {
+ std::vector<TabStripModelObserver*> observers;
+ for (auto& observer : observers_)
+ observer.ModelDestroyed(TabStripModelObserver::ModelPasskey(), this);
+
contents_data_.clear();
order_controller_.reset();
}
@@ -343,14 +347,17 @@
for (auto* new_observer : new_observers)
observers_.AddObserver(new_observer);
+ observer->StartedObserving(TabStripModelObserver::ModelPasskey(), this);
tab_strip_ui_was_set_ = true;
}
void TabStripModel::AddObserver(TabStripModelObserver* observer) {
observers_.AddObserver(observer);
+ observer->StartedObserving(TabStripModelObserver::ModelPasskey(), this);
}
void TabStripModel::RemoveObserver(TabStripModelObserver* observer) {
+ observer->StoppedObserving(TabStripModelObserver::ModelPasskey(), this);
observers_.RemoveObserver(observer);
}
diff --git a/chrome/browser/ui/tabs/tab_strip_model_observer.cc b/chrome/browser/ui/tabs/tab_strip_model_observer.cc
index 3f59037..3a71af9 100644
--- a/chrome/browser/ui/tabs/tab_strip_model_observer.cc
+++ b/chrome/browser/ui/tabs/tab_strip_model_observer.cc
@@ -7,6 +7,7 @@
#include <utility>
#include "base/logging.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
using content::WebContents;
@@ -114,7 +115,14 @@
////////////////////////////////////////////////////////////////////////////////
// TabStripModelObserver
//
-TabStripModelObserver::TabStripModelObserver() {
+TabStripModelObserver::TabStripModelObserver() {}
+
+TabStripModelObserver::~TabStripModelObserver() {
+ std::set<TabStripModel*> models(observed_models_.begin(),
+ observed_models_.end());
+ for (auto* model : models) {
+ model->RemoveObserver(this);
+ }
}
void TabStripModelObserver::OnTabStripModelChanged(
@@ -150,3 +158,48 @@
void TabStripModelObserver::CloseAllTabsStopped(TabStripModel* tab_strip_model,
CloseAllStoppedReason reason) {}
void TabStripModelObserver::SetTabNeedsAttentionAt(int index, bool attention) {}
+void TabStripModelObserver::OnTabStripModelDestroyed(TabStripModel* model) {}
+
+// static
+void TabStripModelObserver::StopObservingAll(TabStripModelObserver* observer) {
+ while (!observer->observed_models_.empty()) {
+ (*observer->observed_models_.begin())->RemoveObserver(observer);
+ }
+}
+
+// static
+bool TabStripModelObserver::IsObservingAny(TabStripModelObserver* observer) {
+ return !observer->observed_models_.empty();
+}
+
+// static
+int TabStripModelObserver::CountObservedModels(
+ TabStripModelObserver* observer) {
+ return observer->observed_models_.size();
+}
+
+void TabStripModelObserver::StartedObserving(
+ TabStripModelObserver::ModelPasskey,
+ TabStripModel* model) {
+ // TODO(https://crbug.com/991308): Add this DCHECK here. This DCHECK enforces
+ // that a given TabStripModelObserver only observes a given TabStripModel
+ // once.
+ // DCHECK_EQ(observed_models_.count(model), 0U);
+ observed_models_.insert(model);
+}
+
+void TabStripModelObserver::StoppedObserving(
+ TabStripModelObserver::ModelPasskey,
+ TabStripModel* model) {
+ // TODO(https://crbug.com/991308): Add this DCHECK here. This DCHECK enforces
+ // that a given TabStripModelObserver is only removed from a given
+ // TabStripModel once.
+ // DCHECK_EQ(observed_models_.count(model), 1U);
+ observed_models_.erase(model);
+}
+
+void TabStripModelObserver::ModelDestroyed(TabStripModelObserver::ModelPasskey,
+ TabStripModel* model) {
+ model->RemoveObserver(this);
+ OnTabStripModelDestroyed(model);
+}
diff --git a/chrome/browser/ui/tabs/tab_strip_model_observer.h b/chrome/browser/ui/tabs/tab_strip_model_observer.h
index e583769..bb9eac0 100644
--- a/chrome/browser/ui/tabs/tab_strip_model_observer.h
+++ b/chrome/browser/ui/tabs/tab_strip_model_observer.h
@@ -10,6 +10,7 @@
#include "base/macros.h"
#include "base/optional.h"
+#include "base/scoped_observer.h"
#include "chrome/browser/ui/tabs/tab_change_type.h"
#include "chrome/browser/ui/tabs/tab_group_id.h"
#include "ui/base/models/list_selection_model.h"
@@ -215,6 +216,12 @@
int reason = 0;
};
+// Forbid construction of ScopedObserver with TabStripModel:
+// TabStripModelObserver already implements ScopedObserver's functionality
+// natively.
+template <class U>
+class ScopedObserver<TabStripModel, U> {};
+
////////////////////////////////////////////////////////////////////////////////
//
// TabStripModelObserver
@@ -310,11 +317,39 @@
// |attention| is true.
virtual void SetTabNeedsAttentionAt(int index, bool attention);
+ // Called when an observed TabStripModel is beginning destruction.
+ virtual void OnTabStripModelDestroyed(TabStripModel* tab_strip_model);
+
+ static void StopObservingAll(TabStripModelObserver* observer);
+ static bool IsObservingAny(TabStripModelObserver* observer);
+ static int CountObservedModels(TabStripModelObserver* observer);
+
+ // A passkey for TabStripModel to access some methods on this class - see
+ // </docs/patterns/passkey.md>.
+ class ModelPasskey {
+ private:
+ friend class TabStripModel;
+ ModelPasskey() = default;
+ ~ModelPasskey() = default;
+ };
+
+ // These methods are used by TabStripModel to notify this class of lifecycle
+ // events on the TabStripModelObserver or the TabStripModel itself. The first
+ // two are used to allow TabStripModelObserver to track which models it is
+ // observing. The third is used to allow TabStripModelObserver to clean up
+ // when an observed TabStripModel is destroyed, and to send the
+ // OnTabStripModelDestroyed notification above.
+ void StartedObserving(ModelPasskey, TabStripModel* model);
+ void StoppedObserving(ModelPasskey, TabStripModel* model);
+ void ModelDestroyed(ModelPasskey, TabStripModel* model);
+
protected:
TabStripModelObserver();
- virtual ~TabStripModelObserver() {}
+ virtual ~TabStripModelObserver();
private:
+ std::set<TabStripModel*> observed_models_;
+
DISALLOW_COPY_AND_ASSIGN(TabStripModelObserver);
};
diff --git a/chrome/browser/ui/tabs/tab_strip_model_order_controller.cc b/chrome/browser/ui/tabs/tab_strip_model_order_controller.cc
index 1347f88..509bde9 100644
--- a/chrome/browser/ui/tabs/tab_strip_model_order_controller.cc
+++ b/chrome/browser/ui/tabs/tab_strip_model_order_controller.cc
@@ -16,9 +16,7 @@
tabstrip_->AddObserver(this);
}
-TabStripModelOrderController::~TabStripModelOrderController() {
- tabstrip_->RemoveObserver(this);
-}
+TabStripModelOrderController::~TabStripModelOrderController() {}
int TabStripModelOrderController::DetermineInsertionIndex(
ui::PageTransition transition,
diff --git a/chrome/browser/ui/tabs/tab_strip_model_unittest.cc b/chrome/browser/ui/tabs/tab_strip_model_unittest.cc
index 8f347bba..7243f064 100644
--- a/chrome/browser/ui/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/ui/tabs/tab_strip_model_unittest.cc
@@ -3439,3 +3439,13 @@
strip.CloseAllTabs();
}
+
+TEST_F(TabStripModelTest, ObserverCanBeDestroyedEarly) {
+ TestTabStripModelDelegate delegate;
+ TabStripModel strip(&delegate, profile());
+
+ {
+ MockTabStripModelObserver observer;
+ strip.AddObserver(&observer);
+ }
+}
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
index ed2241b..cd1329b4 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
@@ -109,15 +109,14 @@
should_check_extension_bubble_(!main_bar),
popped_out_action_(nullptr),
is_popped_out_sticky_(false),
- is_showing_bubble_(false),
- tab_strip_observer_(this) {
+ is_showing_bubble_(false) {
if (model_) // |model_| can be null in unittests.
model_observer_.Add(model_);
if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu))
DCHECK(!in_overflow_mode());
- tab_strip_observer_.Add(browser_->tab_strip_model());
+ browser_->tab_strip_model()->AddObserver(this);
}
ToolbarActionsBar::~ToolbarActionsBar() {
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.h b/chrome/browser/ui/toolbar/toolbar_actions_bar.h
index 0ab6ac5..22c593b 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar.h
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.h
@@ -361,8 +361,6 @@
// no drag is in progress.
base::Optional<size_t> index_of_dragged_item_;
- ScopedObserver<TabStripModel, TabStripModelObserver> tab_strip_observer_;
-
base::ObserverList<ToolbarActionsBarObserver>::Unchecked observers_;
base::WeakPtrFactory<ToolbarActionsBar> weak_ptr_factory_{this};
diff --git a/chrome/browser/ui/views/close_bubble_on_tab_activation_helper.cc b/chrome/browser/ui/views/close_bubble_on_tab_activation_helper.cc
index 1ecb6834..54295623 100644
--- a/chrome/browser/ui/views/close_bubble_on_tab_activation_helper.cc
+++ b/chrome/browser/ui/views/close_bubble_on_tab_activation_helper.cc
@@ -11,9 +11,9 @@
CloseBubbleOnTabActivationHelper::CloseBubbleOnTabActivationHelper(
views::BubbleDialogDelegateView* owner_bubble,
Browser* browser)
- : owner_bubble_(owner_bubble), tab_strip_observer_(this) {
+ : owner_bubble_(owner_bubble) {
DCHECK(owner_bubble_);
- tab_strip_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
CloseBubbleOnTabActivationHelper::~CloseBubbleOnTabActivationHelper() = default;
diff --git a/chrome/browser/ui/views/close_bubble_on_tab_activation_helper.h b/chrome/browser/ui/views/close_bubble_on_tab_activation_helper.h
index a7f4b7d..9e91de5 100644
--- a/chrome/browser/ui/views/close_bubble_on_tab_activation_helper.h
+++ b/chrome/browser/ui/views/close_bubble_on_tab_activation_helper.h
@@ -5,7 +5,6 @@
#ifndef CHROME_BROWSER_UI_VIEWS_CLOSE_BUBBLE_ON_TAB_ACTIVATION_HELPER_H_
#define CHROME_BROWSER_UI_VIEWS_CLOSE_BUBBLE_ON_TAB_ACTIVATION_HELPER_H_
-#include "base/scoped_observer.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
class Browser;
@@ -34,8 +33,6 @@
private:
views::BubbleDialogDelegateView* owner_bubble_; // weak, owns me.
- ScopedObserver<TabStripModel, CloseBubbleOnTabActivationHelper>
- tab_strip_observer_;
DISALLOW_COPY_AND_ASSIGN(CloseBubbleOnTabActivationHelper);
};
diff --git a/chrome/browser/ui/views/extensions/extension_popup.cc b/chrome/browser/ui/views/extensions/extension_popup.cc
index e4c23013..12b77ca 100644
--- a/chrome/browser/ui/views/extensions/extension_popup.cc
+++ b/chrome/browser/ui/views/extensions/extension_popup.cc
@@ -191,7 +191,7 @@
this, extensions::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE,
content::Source<content::BrowserContext>(host_->browser_context()));
content::DevToolsAgentHost::AddObserver(this);
- observer_.Add(GetExtensionView()->GetBrowser()->tab_strip_model());
+ GetExtensionView()->GetBrowser()->tab_strip_model()->AddObserver(this);
// If the host had somehow finished loading, then we'd miss the notification
// and not show. This seems to happen in single-process mode.
diff --git a/chrome/browser/ui/views/extensions/extension_popup.h b/chrome/browser/ui/views/extensions/extension_popup.h
index 4a3ea77..597cbfa 100644
--- a/chrome/browser/ui/views/extensions/extension_popup.h
+++ b/chrome/browser/ui/views/extensions/extension_popup.h
@@ -8,7 +8,6 @@
#include "base/callback.h"
#include "base/compiler_specific.h"
#include "base/macros.h"
-#include "base/scoped_observer.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "chrome/browser/ui/views/extensions/extension_view_views.h"
@@ -127,7 +126,6 @@
ShowAction show_action_;
content::NotificationRegistrar registrar_;
- ScopedObserver<TabStripModel, TabStripModelObserver> observer_{this};
DISALLOW_COPY_AND_ASSIGN(ExtensionPopup);
};
diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc
index 9f7c7d0..37e8033 100644
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -465,8 +465,6 @@
// Immersive mode may need to reparent views before they are removed/deleted.
immersive_mode_controller_.reset();
- browser_->tab_strip_model()->RemoveObserver(this);
-
extensions::ExtensionCommandsGlobalRegistry* global_registry =
extensions::ExtensionCommandsGlobalRegistry::Get(browser_->profile());
if (global_registry->registry_for_active_window() ==
diff --git a/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc b/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc
index 84ea02c..21ae6243 100644
--- a/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc
@@ -182,7 +182,6 @@
LocationBarView::Delegate* delegate)
: TabStripModelObserver(),
delegate_(delegate),
- tab_strip_model_observer_(this),
browser_(browser_view->browser()) {
set_context_menu_controller(this);
base::Optional<SkColor> optional_theme_color =
@@ -225,7 +224,7 @@
.SetCrossAxisAlignment(views::LayoutAlignment::kCenter)
.SetInteriorMargin(GetLayoutInsets(LayoutInset::TOOLBAR_INTERIOR_MARGIN));
- tab_strip_model_observer_.Add(browser_->tab_strip_model());
+ browser_->tab_strip_model()->AddObserver(this);
}
CustomTabBarView::~CustomTabBarView() {}
diff --git a/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h b/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
index 16fe0b1..0a23ebc6 100644
--- a/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
+++ b/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
@@ -7,7 +7,6 @@
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
-#include "base/scoped_observer.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
#include "chrome/browser/ui/views/location_bar/location_icon_view.h"
@@ -113,7 +112,6 @@
LocationBarView::Delegate* delegate_ = nullptr;
LocationIconView* location_icon_view_ = nullptr;
CustomTabBarTitleOriginView* title_origin_view_ = nullptr;
- ScopedObserver<TabStripModel, CustomTabBarView> tab_strip_model_observer_;
std::unique_ptr<ui::SimpleMenuModel> context_menu_model_;
std::unique_ptr<views::MenuRunner> context_menu_runner_;
Browser* browser_ = nullptr;
diff --git a/chrome/browser/ui/views/location_bar/custom_tab_bar_view_browsertest.cc b/chrome/browser/ui/views/location_bar/custom_tab_bar_view_browsertest.cc
index 29451f4..6a02447b 100644
--- a/chrome/browser/ui/views/location_bar/custom_tab_bar_view_browsertest.cc
+++ b/chrome/browser/ui/views/location_bar/custom_tab_bar_view_browsertest.cc
@@ -2,7 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include "base/scoped_observer.h"
#include "build/build_config.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
@@ -37,11 +36,9 @@
// Create a new TitleObserver for the browser of |contents|, waiting for
// |target_title|.
TestTitleObserver(content::WebContents* contents, base::string16 target_title)
- : contents_(contents),
- target_title_(target_title),
- tab_strip_model_observer_(this) {
+ : contents_(contents), target_title_(target_title) {
browser_ = chrome::FindBrowserWithWebContents(contents_);
- tab_strip_model_observer_.Add(browser_->tab_strip_model());
+ browser_->tab_strip_model()->AddObserver(this);
}
// Run a loop, blocking until a tab has the title |target_title|.
@@ -74,7 +71,6 @@
Browser* browser_;
base::string16 target_title_;
base::RunLoop awaiter_;
- ScopedObserver<TabStripModel, TestTitleObserver> tab_strip_model_observer_;
};
// Opens a new popup window from |web_contents| on |target_url| and returns
diff --git a/chrome/browser/ui/views/tab_sharing/tab_sharing_ui_views.cc b/chrome/browser/ui/views/tab_sharing/tab_sharing_ui_views.cc
index 73a59fb7..8cecf26 100644
--- a/chrome/browser/ui/views/tab_sharing/tab_sharing_ui_views.cc
+++ b/chrome/browser/ui/views/tab_sharing/tab_sharing_ui_views.cc
@@ -166,7 +166,7 @@
void TabSharingUIViews::OnBrowserAdded(Browser* browser) {
if (browser->profile()->GetOriginalProfile() == profile_)
- tab_strip_models_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
void TabSharingUIViews::OnBrowserRemoved(Browser* browser) {
@@ -174,9 +174,7 @@
if (browser_list->empty())
browser_list->RemoveObserver(this);
- TabStripModel* tab_strip_model = browser->tab_strip_model();
- if (tab_strip_models_observer_.IsObserving(tab_strip_model))
- tab_strip_models_observer_.Remove(tab_strip_model);
+ browser->tab_strip_model()->RemoveObserver(this);
}
void TabSharingUIViews::OnTabStripModelChanged(
@@ -233,7 +231,7 @@
void TabSharingUIViews::RemoveInfobarsForAllTabs() {
BrowserList::GetInstance()->RemoveObserver(this);
- tab_strip_models_observer_.RemoveAll();
+ TabStripModelObserver::StopObservingAll(this);
for (const auto& infobars_entry : infobars_)
infobars_entry.second->RemoveSelf();
diff --git a/chrome/browser/ui/views/tab_sharing/tab_sharing_ui_views.h b/chrome/browser/ui/views/tab_sharing/tab_sharing_ui_views.h
index bf846250..68210f2 100644
--- a/chrome/browser/ui/views/tab_sharing/tab_sharing_ui_views.h
+++ b/chrome/browser/ui/views/tab_sharing/tab_sharing_ui_views.h
@@ -10,7 +10,6 @@
#include "base/callback.h"
#include "base/macros.h"
-#include "base/scoped_observer.h"
#include "base/strings/string16.h"
#include "chrome/browser/media/webrtc/media_stream_capture_indicator.h"
#include "chrome/browser/ui/browser_list_observer.h"
@@ -69,8 +68,6 @@
void RemoveInfobarsForAllTabs();
std::map<content::WebContents*, infobars::InfoBar*> infobars_;
- ScopedObserver<TabStripModel, TabStripModelObserver>
- tab_strip_models_observer_{this};
const base::string16 app_name_;
content::WebContents* shared_tab_;
base::string16 shared_tab_name_;
diff --git a/chrome/browser/ui/views/tabs/tab_drag_controller.cc b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
index b3663a6..e2e7b93 100644
--- a/chrome/browser/ui/views/tabs/tab_drag_controller.cc
+++ b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
@@ -242,27 +242,26 @@
public:
explicit SourceTabStripEmptinessTracker(TabStripModel* tabstrip,
TabDragController* parent)
- : tab_strip_(tabstrip), parent_(parent), observer_(this) {
- observer_.Add(tab_strip_);
+ : tab_strip_(tabstrip), parent_(parent) {
+ tab_strip_->AddObserver(this);
}
private:
void TabStripEmpty() override {
- observer_.Remove(tab_strip_);
+ tab_strip_->RemoveObserver(this);
parent_->OnSourceTabStripEmpty();
}
TabStripModel* const tab_strip_;
TabDragController* const parent_;
- ScopedObserver<TabStripModel, TabStripModelObserver> observer_;
};
class TabDragController::DraggedTabsClosedTracker
: public TabStripModelObserver {
public:
DraggedTabsClosedTracker(TabStripModel* tabstrip, TabDragController* parent)
- : parent_(parent), observer_(this) {
- observer_.Add(tabstrip);
+ : parent_(parent) {
+ tabstrip->AddObserver(this);
}
void OnTabStripModelChanged(
@@ -277,7 +276,6 @@
private:
TabDragController* const parent_;
- ScopedObserver<TabStripModel, TabStripModelObserver> observer_;
};
TabDragController::TabDragData::TabDragData()
diff --git a/chrome/test/base/ui_test_utils.cc b/chrome/test/base/ui_test_utils.cc
index 8934e079e..78fcb8fe1 100644
--- a/chrome/test/base/ui_test_utils.cc
+++ b/chrome/test/base/ui_test_utils.cc
@@ -620,11 +620,9 @@
}
TabAddedWaiter::TabAddedWaiter(Browser* browser) {
- scoped_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
-TabAddedWaiter::~TabAddedWaiter() = default;
-
void TabAddedWaiter::Wait() {
run_loop_.Run();
}
@@ -640,7 +638,7 @@
AllBrowserTabAddedWaiter::AllBrowserTabAddedWaiter() {
BrowserList::AddObserver(this);
for (const Browser* browser : *BrowserList::GetInstance())
- tab_strip_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
AllBrowserTabAddedWaiter::~AllBrowserTabAddedWaiter() {
@@ -664,7 +662,7 @@
}
void AllBrowserTabAddedWaiter::OnBrowserAdded(Browser* browser) {
- tab_strip_observer_.Add(browser->tab_strip_model());
+ browser->tab_strip_model()->AddObserver(this);
}
} // namespace ui_test_utils
diff --git a/chrome/test/base/ui_test_utils.h b/chrome/test/base/ui_test_utils.h
index 79c9d9e..c7407ba 100644
--- a/chrome/test/base/ui_test_utils.h
+++ b/chrome/test/base/ui_test_utils.h
@@ -281,7 +281,7 @@
class TabAddedWaiter : public TabStripModelObserver {
public:
explicit TabAddedWaiter(Browser* browser);
- ~TabAddedWaiter() override;
+ ~TabAddedWaiter() override = default;
void Wait();
@@ -293,7 +293,6 @@
private:
base::RunLoop run_loop_;
- ScopedObserver<TabStripModel, TabStripModelObserver> scoped_observer_{this};
DISALLOW_COPY_AND_ASSIGN(TabAddedWaiter);
};
@@ -318,8 +317,6 @@
void OnBrowserAdded(Browser* browser) override;
private:
- ScopedObserver<TabStripModel, TabStripModelObserver> tab_strip_observer_{
- this};
base::RunLoop run_loop_;
// The last tab that was added.
diff --git a/docs/patterns/passkey.md b/docs/patterns/passkey.md
new file mode 100644
index 0000000..b45c76e
--- /dev/null
+++ b/docs/patterns/passkey.md
@@ -0,0 +1,47 @@
+# The Passkey Pattern
+
+The Passkey pattern is used when you need to expose a subset of a class's
+methods to another class in a more granular way than simply friending the other
+class. In essence, it involves creating a "passkey" class that can only be
+constructed by specific other classes, and requiring an instance of that passkey
+class to be passed in when calling methods you wish to restrict the use of. It
+is used like this:
+
+```
+class Foo {
+ public:
+ Foo();
+ ~Foo();
+
+ void NormalPublicMethod();
+ bool AnotherNormalPublicMethod(int a, int b);
+
+ class BarPasskey {
+ private:
+ friend class Bar;
+ BarPasskey() = default;
+ ~BarPasskey() = default;
+ };
+
+ void HelpBarOut(BarPasskey, ...);
+};
+
+...
+
+void Bar::DoStuff() {
+ foo->HelpBarOut(Foo::BarPasskey(), ...);
+}
+```
+
+The private constructor on `Foo::BarPasskey` prevents any class other than `Bar`
+from constructing a `Foo::BarPasskey`, which means that:
+
+* Only `Bar` can call those methods
+* `Bar` can delegate the ability to call those methods to other
+ classes/functions by passing them a `Foo::BarPasskey` instance
+
+This method is effectively free at runtime - a few extra bytes of argument space
+are used to pass in the Passkey object.
+
+It is encouraged to leave the `BarPasskey` parameter unnamed to reinforce that it
+carries no semantic information and is not actually used for anything.
