Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(306)

Unified Diff: ios/shared/chrome/browser/tabs/web_state_list.mm

Issue 2710183003: Fix WebStateListObserver::WebStateDetachedAt() parameter lifetime. (Closed)
Patch Set: Fix typos. Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: ios/shared/chrome/browser/tabs/web_state_list.mm
diff --git a/ios/shared/chrome/browser/tabs/web_state_list.mm b/ios/shared/chrome/browser/tabs/web_state_list.mm
index 645cb680943574f2a4eb4668675613a7efa62d38..7320fa3f3f073b08149da921cb074669ba326e03 100644
--- a/ios/shared/chrome/browser/tabs/web_state_list.mm
+++ b/ios/shared/chrome/browser/tabs/web_state_list.mm
@@ -17,12 +17,10 @@
#error "This file requires ARC support."
#endif
-// Wrapper around a WebState stored in a WebStateList. May own the WebState
-// dependending on the WebStateList ownership setting (should always be true
-// once ownership of Tab is sane, see http://crbug.com/546222 for progress).
+// Wrapper around a WebState stored in a WebStateList.
class WebStateList::WebStateWrapper {
public:
- WebStateWrapper(web::WebState* web_state, bool assume_ownership);
+ explicit WebStateWrapper(web::WebState* web_state);
~WebStateWrapper();
web::WebState* web_state() const { return web_state_; }
@@ -47,21 +45,16 @@ class WebStateList::WebStateWrapper {
web::WebState* web_state_;
web::WebState* opener_ = nullptr;
int opener_last_committed_index_;
- const bool has_web_state_ownership_;
DISALLOW_COPY_AND_ASSIGN(WebStateWrapper);
};
-WebStateList::WebStateWrapper::WebStateWrapper(web::WebState* web_state,
- bool assume_ownership)
- : web_state_(web_state), has_web_state_ownership_(assume_ownership) {
+WebStateList::WebStateWrapper::WebStateWrapper(web::WebState* web_state)
+ : web_state_(web_state) {
DCHECK(web_state_);
}
-WebStateList::WebStateWrapper::~WebStateWrapper() {
- if (has_web_state_ownership_)
- delete web_state_;
-}
+WebStateList::WebStateWrapper::~WebStateWrapper() = default;
web::WebState* WebStateList::WebStateWrapper::ReplaceWebState(
web::WebState* web_state) {
@@ -97,7 +90,17 @@ WebStateList::WebStateList(WebStateOwnership ownership)
: web_state_ownership_(ownership),
order_controller_(base::MakeUnique<WebStateListOrderController>(this)) {}
-WebStateList::~WebStateList() = default;
+WebStateList::~WebStateList() {
+ // Once WebStateList owns the WebState and has a CloseWebStateAt() method,
+ // then change this to close all the WebState. See http://crbug.com/546222
+ // for progress.
+ if (web_state_ownership_ == WebStateOwned) {
+ for (auto& web_state_wrapper : web_state_wrappers_) {
+ web::WebState* web_state = web_state_wrapper->web_state();
+ delete web_state;
+ }
+ }
+}
bool WebStateList::ContainsIndex(int index) const {
return 0 <= index && index < count();
@@ -143,10 +146,8 @@ void WebStateList::InsertWebState(int index,
web::WebState* web_state,
web::WebState* opener) {
DCHECK(ContainsIndex(index) || index == count());
- web_state_wrappers_.insert(
- web_state_wrappers_.begin() + index,
- base::MakeUnique<WebStateWrapper>(web_state,
- web_state_ownership_ == WebStateOwned));
+ web_state_wrappers_.insert(web_state_wrappers_.begin() + index,
+ base::MakeUnique<WebStateWrapper>(web_state));
if (opener)
SetOpenerOfWebStateAt(index, opener);
@@ -200,15 +201,17 @@ web::WebState* WebStateList::ReplaceWebStateAt(int index,
return old_web_state;
}
-void WebStateList::DetachWebStateAt(int index) {
+web::WebState* WebStateList::DetachWebStateAt(int index) {
DCHECK(ContainsIndex(index));
ClearOpenersReferencing(index);
- web::WebState* web_state = web_state_wrappers_[index]->web_state();
+ web::WebState* old_web_state = web_state_wrappers_[index]->web_state();
web_state_wrappers_.erase(web_state_wrappers_.begin() + index);
for (auto& observer : observers_)
- observer.WebStateDetachedAt(this, web_state, index);
+ observer.WebStateDetachedAt(this, old_web_state, index);
+
+ return old_web_state;
}
void WebStateList::AddObserver(WebStateListObserver* observer) {
« no previous file with comments | « ios/shared/chrome/browser/tabs/web_state_list.h ('k') | ios/shared/chrome/browser/tabs/web_state_list_unittest.mm » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698