[iOS][tabmodel] Factor ClosingWebStateObserver into a BrowserAgent.

As a tabmodel needs to be deprecated, this CL extracts
ClosingWebStateObserver into a browser agent to remove its usage in
TabModel.

Bug: 1050111,1045575,1045142
Change-Id: Ic6c5b65ff55bd211d38752b274c4dcbb9d854bcc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2287434
Commit-Queue: Nazerke Kalidolda <nazerke@google.com>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800977}

ios/chrome/browser/main/browser_agent_util.mm

@@ -17,6 +17,7 @@
 #import "ios/chrome/browser/sessions/session_restoration_browser_agent.h"
 #import "ios/chrome/browser/sessions/session_service_ios.h"
 #import "ios/chrome/browser/snapshots/snapshot_browser_agent.h"
+#import "ios/chrome/browser/tabs/closing_web_state_observer_browser_agent.h"
 #include "ios/chrome/browser/tabs/synced_window_delegate_browser_agent.h"
 #import "ios/chrome/browser/url_loading/url_loading_browser_agent.h"
 #import "ios/chrome/browser/url_loading/url_loading_notifier_browser_agent.h"
@@ -42,6 +43,7 @@ void AttachBrowserAgents(Browser* browser) {
   UrlLoadingNotifierBrowserAgent::CreateForBrowser(browser);
   AppLauncherBrowserAgent::CreateForBrowser(browser);
   WebStateListMetricsBrowserAgent::CreateForBrowser(browser);
+  ClosingWebStateObserverBrowserAgent::CreateForBrowser(browser);
   SnapshotBrowserAgent::CreateForBrowser(browser);
 
   // Send Tab To Self is non-OTR only.

ios/chrome/browser/tabs/BUILD.gn

@@ -4,6 +4,7 @@
 
 source_set("tabs") {
   sources = [
+    "closing_web_state_observer_browser_agent.h",
     "ios_synced_window_delegate_getter.h",
     "synced_window_delegate_browser_agent.h",
     "tab_helper_delegate_installer.h",
@@ -29,8 +30,7 @@ source_set("tabs") {
 
 source_set("tabs_internal") {
   sources = [
-    "closing_web_state_observer.h",
-    "closing_web_state_observer.mm",
+    "closing_web_state_observer_browser_agent.mm",
     "ios_synced_window_delegate_getter.mm",
     "synced_window_delegate_browser_agent.mm",
     "tab_helper_util.mm",

ios/chrome/browser/tabs/closing_web_state_observer.h

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef IOS_CHROME_BROWSER_TABS_CLOSING_WEB_STATE_OBSERVER_H_
-#define IOS_CHROME_BROWSER_TABS_CLOSING_WEB_STATE_OBSERVER_H_
-
-#import <Foundation/Foundation.h>
-
-#import "ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h"
-
-namespace sessions {
-class TabRestoreService;
-}
-
-// Observes WebStateList events about closing WebState.
-// TODO(crbug.com/1045142): Find a suitable location for the functionalities
-// handled by this class and then remove it.
-@interface ClosingWebStateObserver : NSObject <WebStateListObserving>
-
-- (instancetype)initWithRestoreService:
-    (sessions::TabRestoreService*)restoreService NS_DESIGNATED_INITIALIZER;
-
-- (instancetype)init NS_UNAVAILABLE;
-
-@end
-
-#endif  // IOS_CHROME_BROWSER_TABS_CLOSING_WEB_STATE_OBSERVER_H_

ios/chrome/browser/tabs/closing_web_state_observer_browser_agent.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_CHROME_BROWSER_TABS_CLOSING_WEB_STATE_OBSERVER_BROWSER_AGENT_H_
+#define IOS_CHROME_BROWSER_TABS_CLOSING_WEB_STATE_OBSERVER_BROWSER_AGENT_H_
+
+#import "ios/chrome/browser/main/browser_observer.h"
+#import "ios/chrome/browser/main/browser_user_data.h"
+#import "ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h"
+
+namespace sessions {
+class TabRestoreService;
+}
+
+// TODO(crbug.com/1121120): more cleanly separate the responsibilities of this
+// class: There should be a different object to be responsible for cleaning up
+// snapshots.
+// A browser agent that is responsible for handling WebStateList
+// events about closing WebState, like requesting deletion of the current page
+// snapshot from disk and memory. This class also records of history for
+// non-incognito Browser's WebStates.
+class ClosingWebStateObserverBrowserAgent
+    : BrowserObserver,
+      public BrowserUserData<ClosingWebStateObserverBrowserAgent>,
+      WebStateListObserver {
+ public:
+  ClosingWebStateObserverBrowserAgent();
+  ~ClosingWebStateObserverBrowserAgent() override;
+
+ private:
+  explicit ClosingWebStateObserverBrowserAgent(Browser* browser);
+  friend class BrowserUserData<ClosingWebStateObserverBrowserAgent>;
+  BROWSER_USER_DATA_KEY_DECL();
+
+  // Records history for a given non-incognito WebState and does not record
+  // history if the tab has no navigation or has only presented the NTP or the
+  // bookmark UI.
+  void RecordHistoryForWebStateAtIndex(web::WebState* web_state, int index);
+
+  // BrowserObserver methods.
+  void BrowserDestroyed(Browser* browser) override;
+
+  // WebStateListObserver implementation.
+  void WebStateReplacedAt(WebStateList* web_state_list,
+                          web::WebState* old_web_state,
+                          web::WebState* new_web_state,
+                          int index) override;
+
+  void WillDetachWebStateAt(WebStateList* web_state_list,
+                            web::WebState* web_state,
+                            int index) override;
+
+  void WillCloseWebStateAt(WebStateList* web_state_list,
+                           web::WebState* web_state,
+                           int index,
+                           bool user_action) override;
+
+  sessions::TabRestoreService* restore_service_ = nullptr;
+};
+#endif  // IOS_CHROME_BROWSER_TABS_CLOSING_WEB_STATE_OBSERVER_BROWSER_AGENT_H_

ios/chrome/browser/tabs/closing_web_state_observer.mm → ios/chrome/browser/tabs/closing_web_state_observer_browser_agent.mm

@@ -2,13 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#import "ios/chrome/browser/tabs/closing_web_state_observer.h"
+#import "ios/chrome/browser/tabs/closing_web_state_observer_browser_agent.h"
 
 #include "base/strings/string_piece.h"
 #include "components/sessions/core/tab_restore_service.h"
 #include "components/sessions/ios/ios_restore_live_tab.h"
 #include "components/sessions/ios/ios_webstate_live_tab.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
+#include "ios/chrome/browser/sessions/ios_chrome_tab_restore_service_factory.h"
 #import "ios/chrome/browser/snapshots/snapshot_tab_helper.h"
 #import "ios/chrome/browser/web_state_list/web_state_list.h"
 #import "ios/web/public/navigation/navigation_item.h"
@@ -20,66 +21,39 @@
 #error "This file requires ARC support."
 #endif
 
-@implementation ClosingWebStateObserver {
-  sessions::TabRestoreService* _restoreService;
-}
+BROWSER_USER_DATA_KEY_IMPL(ClosingWebStateObserverBrowserAgent)
 
-- (instancetype)initWithRestoreService:
-    (sessions::TabRestoreService*)restoreService {
-  self = [super init];
-  if (self) {
-    _restoreService = restoreService;
-  }
-  return self;
+ClosingWebStateObserverBrowserAgent::ClosingWebStateObserverBrowserAgent(
+    Browser* browser)
+    : restore_service_(IOSChromeTabRestoreServiceFactory::GetForBrowserState(
+          browser->GetBrowserState())) {
+  browser->AddObserver(this);
+  browser->GetWebStateList()->AddObserver(this);
 }
 
-#pragma mark - WebStateListObserving
-
-- (void)webStateList:(WebStateList*)webStateList
-    didReplaceWebState:(web::WebState*)oldWebState
-          withWebState:(web::WebState*)newWebState
-               atIndex:(int)atIndex {
-  SnapshotTabHelper::FromWebState(oldWebState)->RemoveSnapshot();
-}
+ClosingWebStateObserverBrowserAgent::~ClosingWebStateObserverBrowserAgent() {}
 
-- (void)webStateList:(WebStateList*)webStateList
-    willDetachWebState:(web::WebState*)webState
-               atIndex:(int)atIndex {
-  [self recordHistoryForWebState:webState atIndex:atIndex];
-}
+#pragma mark - Private methods
 
-- (void)webStateList:(WebStateList*)webStateList
-    willCloseWebState:(web::WebState*)webState
-              atIndex:(int)atIndex
-           userAction:(BOOL)userAction {
-  if (userAction) {
-    SnapshotTabHelper::FromWebState(webState)->RemoveSnapshot();
-  }
-}
-
-#pragma mark - Private
-
-- (void)recordHistoryForWebState:(web::WebState*)webState atIndex:(int)atIndex {
+void ClosingWebStateObserverBrowserAgent::RecordHistoryForWebStateAtIndex(
+    web::WebState* web_state,
+    int index) {
   // The RestoreService will be null if navigation is off the record.
-  if (!_restoreService)
+  if (!restore_service_)
     return;
 
-  web::NavigationManager* navigationManager = webState->GetNavigationManager();
-  if (navigationManager->IsRestoreSessionInProgress()) {
-    // TODO(crbug.com/1070852): RestoreIOSLiveTab is not compiled with ARC and
-    // therefore is not properly retaining the CRWSessionStorage* which it is
-    // passed. Temporarily work around this bug by keeping a strong pointer to
-    // the CRWSessionStorage* here, until RestoreIOSLiveTab can be fixed
-    // properly.
-    CRWSessionStorage* storage = webState->BuildSessionStorage();
+  web::NavigationManager* navigation_manager =
+      web_state->GetNavigationManager();
+  if (navigation_manager->IsRestoreSessionInProgress()) {
+    CRWSessionStorage* storage = web_state->BuildSessionStorage();
     auto live_tab = std::make_unique<sessions::RestoreIOSLiveTab>(storage);
-    _restoreService->CreateHistoricalTab(live_tab.get(), atIndex);
+    restore_service_->CreateHistoricalTab(live_tab.get(), index);
     return;
   }
   // No need to record history if the tab has no navigation or has only
   // presented the NTP or the bookmark UI.
-  if (navigationManager->GetItemCount() <= 1) {
-    web::NavigationItem* item = navigationManager->GetLastCommittedItem();
+  if (navigation_manager->GetItemCount() <= 1) {
+    web::NavigationItem* item = navigation_manager->GetLastCommittedItem();
     if (!item)
       return;
 
@@ -88,8 +62,40 @@
       return;
   }
 
-  _restoreService->CreateHistoricalTab(
-      sessions::IOSWebStateLiveTab::GetForWebState(webState), atIndex);
+  restore_service_->CreateHistoricalTab(
+      sessions::IOSWebStateLiveTab::GetForWebState(web_state), index);
 }
 
-@end
+#pragma mark - BrowserObserver
+
+void ClosingWebStateObserverBrowserAgent::BrowserDestroyed(Browser* browser) {
+  browser->RemoveObserver(this);
+  browser->GetWebStateList()->RemoveObserver(this);
+}
+
+#pragma mark - WebStateListObserving
+
+void ClosingWebStateObserverBrowserAgent::WebStateReplacedAt(
+    WebStateList* web_state_list,
+    web::WebState* old_web_state,
+    web::WebState* new_web_state,
+    int index) {
+  SnapshotTabHelper::FromWebState(old_web_state)->RemoveSnapshot();
+}
+
+void ClosingWebStateObserverBrowserAgent::WillDetachWebStateAt(
+    WebStateList* web_state_list,
+    web::WebState* web_state,
+    int index) {
+  RecordHistoryForWebStateAtIndex(web_state, index);
+}
+
+void ClosingWebStateObserverBrowserAgent::WillCloseWebStateAt(
+    WebStateList* web_state_list,
+    web::WebState* web_state,
+    int index,
+    bool user_action) {
+  if (user_action) {
+    SnapshotTabHelper::FromWebState(web_state)->RemoveSnapshot();
+  }
+}

ios/chrome/browser/tabs/tab_model.mm

@@ -37,7 +37,6 @@
 #import "ios/chrome/browser/sessions/session_window_ios.h"
 #import "ios/chrome/browser/snapshots/snapshot_browser_agent.h"
 #import "ios/chrome/browser/snapshots/snapshot_cache.h"
-#import "ios/chrome/browser/tabs/closing_web_state_observer.h"
 #import "ios/chrome/browser/tabs/tab_parenting_observer.h"
 #import "ios/chrome/browser/web/tab_id_tab_helper.h"
 #import "ios/chrome/browser/web_state_list/web_state_list.h"
@@ -120,7 +119,7 @@ void RecordInterfaceOrientationMetric() {
 
 }  // anonymous namespace
 
-@interface TabModel ()<CRWWebStateObserver, WebStateListObserving> {
+@interface TabModel () <CRWWebStateObserver> {
   // Weak reference to the underlying shared model implementation.
   WebStateList* _webStateList;
 
@@ -131,10 +130,6 @@ void RecordInterfaceOrientationMetric() {
   // notification, translate and forward events, update metrics, ...).
   std::vector<std::unique_ptr<WebStateListObserver>> _webStateListObservers;
 
-  // Strong references to id<WebStateListObserving> wrapped by non-owning
-  // WebStateListObserverBridges.
-  NSArray<id<WebStateListObserving>>* _retainedWebStateListObservers;
-
   // Weak reference to the session restoration agent.
   SessionRestorationBrowserAgent* _sessionRestorationBrowserAgent;
 
@@ -187,26 +182,10 @@ void RecordInterfaceOrientationMetric() {
 
     _snapshotBrowserAgent = SnapshotBrowserAgent::FromBrowser(browser);
 
-    NSMutableArray<id<WebStateListObserving>>* retainedWebStateListObservers =
-        [[NSMutableArray alloc] init];
-
-    ClosingWebStateObserver* closingWebStateObserver =
-        [[ClosingWebStateObserver alloc]
-            initWithRestoreService:IOSChromeTabRestoreServiceFactory::
-                                       GetForBrowserState(_browserState)];
-    [retainedWebStateListObservers addObject:closingWebStateObserver];
-
-    _webStateListObservers.push_back(
-        std::make_unique<WebStateListObserverBridge>(self));
-
-    _webStateListObservers.push_back(
-        std::make_unique<WebStateListObserverBridge>(closingWebStateObserver));
-
     _webStateListObservers.push_back(std::make_unique<TabParentingObserver>());
 
     for (const auto& webStateListObserver : _webStateListObservers)
       _webStateList->AddObserver(webStateListObserver.get());
-    _retainedWebStateListObservers = [retainedWebStateListObservers copy];
 
     // Register for resign active notification.
     [[NSNotificationCenter defaultCenter]
@@ -248,7 +227,6 @@ void RecordInterfaceOrientationMetric() {
   for (const auto& webStateListObserver : _webStateListObservers)
     _webStateList->RemoveObserver(webStateListObserver.get());
   _webStateListObservers.clear();
-  _retainedWebStateListObservers = nil;
   _webStateList = nullptr;
 
   _clearPoliciesTaskTracker.TryCancelAll();
@@ -376,31 +354,4 @@ void RecordInterfaceOrientationMetric() {
   NOTREACHED();
 }
 
-#pragma mark - WebStateListObserving
-
-- (void)webStateList:(WebStateList*)webStateList
-    didInsertWebState:(web::WebState*)webState
-              atIndex:(int)index
-           activating:(BOOL)activating {
-  DCHECK(webState);
-  webState->AddObserver(_webStateObserver.get());
-}
-
-- (void)webStateList:(WebStateList*)webStateList
-    didReplaceWebState:(web::WebState*)oldWebState
-          withWebState:(web::WebState*)newWebState
-               atIndex:(int)atIndex {
-  DCHECK(oldWebState);
-  DCHECK(newWebState);
-  newWebState->AddObserver(_webStateObserver.get());
-  oldWebState->RemoveObserver(_webStateObserver.get());
-}
-
-- (void)webStateList:(WebStateList*)webStateList
-    didDetachWebState:(web::WebState*)webState
-              atIndex:(int)atIndex {
-  DCHECK(webState);
-  webState->RemoveObserver(_webStateObserver.get());
-}
-
 @end

ios/chrome/browser/ui/tab_grid/BUILD.gn

@@ -129,6 +129,7 @@ source_set("unit_tests") {
     "//ios/chrome/browser/browser_state:test_support",
     "//ios/chrome/browser/main:test_support",
     "//ios/chrome/browser/ntp",
+    "//ios/chrome/browser/sessions",
     "//ios/chrome/browser/sessions:restoration_agent",
     "//ios/chrome/browser/sessions:test_support",
     "//ios/chrome/browser/snapshots",

ios/chrome/browser/ui/tab_grid/tab_grid_mediator_unittest.mm

@@ -17,11 +17,12 @@
 #import "ios/chrome/browser/main/test_browser.h"
 #import "ios/chrome/browser/ntp/new_tab_page_tab_helper.h"
 #import "ios/chrome/browser/ntp/new_tab_page_tab_helper_delegate.h"
+#include "ios/chrome/browser/sessions/ios_chrome_tab_restore_service_factory.h"
 #include "ios/chrome/browser/sessions/session_restoration_browser_agent.h"
 #import "ios/chrome/browser/sessions/test_session_service.h"
 #import "ios/chrome/browser/snapshots/snapshot_browser_agent.h"
 #import "ios/chrome/browser/snapshots/snapshot_tab_helper.h"
-#import "ios/chrome/browser/tabs/closing_web_state_observer.h"
+#import "ios/chrome/browser/tabs/closing_web_state_observer_browser_agent.h"
 #import "ios/chrome/browser/ui/tab_grid/grid/grid_commands.h"
 #import "ios/chrome/browser/ui/tab_grid/grid/grid_consumer.h"
 #import "ios/chrome/browser/ui/tab_grid/grid/grid_item.h"
@@ -142,6 +143,11 @@ class FakeTabRestoreService : public sessions::TabRestoreService {
   }
   Entries entries_;
 };
+
+std::unique_ptr<KeyedService> BuildFakeTabRestoreService(
+    web::BrowserState* browser_state) {
+  return std::make_unique<FakeTabRestoreService>();
+}
 }  // namespace
 
 // Test object that conforms to GridConsumer and exposes inner state for test
@@ -219,22 +225,23 @@ class TabGridMediatorTest : public PlatformTest {
 
   void SetUp() override {
     PlatformTest::SetUp();
-    browser_state_ = TestChromeBrowserState::Builder().Build();
+    TestChromeBrowserState::Builder builder;
+    builder.AddTestingFactory(IOSChromeTabRestoreServiceFactory::GetInstance(),
+                              base::BindRepeating(BuildFakeTabRestoreService));
+
+    browser_state_ = builder.Build();
+    tab_restore_service_ =
+        IOSChromeTabRestoreServiceFactory::GetForBrowserState(
+            browser_state_.get());
     web_state_list_delegate_ =
         std::make_unique<TabHelperFakeWebStateListDelegate>();
     web_state_list_ =
         std::make_unique<WebStateList>(web_state_list_delegate_.get());
-    tab_restore_service_ = std::make_unique<FakeTabRestoreService>();
-    closing_web_state_observer_ = [[ClosingWebStateObserver alloc]
-        initWithRestoreService:tab_restore_service_.get()];
-    closing_web_state_observer_bridge_ =
-        std::make_unique<WebStateListObserverBridge>(
-            closing_web_state_observer_);
-    web_state_list_->AddObserver(closing_web_state_observer_bridge_.get());
     NSMutableSet<NSString*>* identifiers = [[NSMutableSet alloc] init];
     browser_ = std::make_unique<TestBrowser>(browser_state_.get(),
                                              web_state_list_.get());
     WebUsageEnablerBrowserAgent::CreateForBrowser(browser_.get());
+    ClosingWebStateObserverBrowserAgent::CreateForBrowser(browser_.get());
     SnapshotBrowserAgent::CreateForBrowser(browser_.get());
     SnapshotBrowserAgent::FromBrowser(browser_.get())
         ->SetSessionID(base::SysNSStringToUTF8([[NSUUID UUID] UUIDString]));
@@ -259,7 +266,7 @@ class TabGridMediatorTest : public PlatformTest {
     consumer_ = [[FakeConsumer alloc] init];
     mediator_ = [[TabGridMediator alloc] initWithConsumer:consumer_];
     mediator_.browser = browser_.get();
-    mediator_.tabRestoreService = tab_restore_service_.get();
+    mediator_.tabRestoreService = tab_restore_service_;
   }
 
   // Creates a TestWebState with a navigation history containing exactly only
@@ -280,7 +287,6 @@ class TabGridMediatorTest : public PlatformTest {
   }
 
   void TearDown() override {
-    web_state_list_->RemoveObserver(closing_web_state_observer_bridge_.get());
     PlatformTest::TearDown();
   }
 
@@ -297,16 +303,13 @@ class TabGridMediatorTest : public PlatformTest {
   std::unique_ptr<ChromeBrowserState> browser_state_;
   std::unique_ptr<TabHelperFakeWebStateListDelegate> web_state_list_delegate_;
   std::unique_ptr<WebStateList> web_state_list_;
-  std::unique_ptr<FakeTabRestoreService> tab_restore_service_;
+  sessions::TabRestoreService* tab_restore_service_;
   id tab_model_;
   FakeConsumer* consumer_;
   TabGridMediator* mediator_;
   NSSet<NSString*>* original_identifiers_;
   NSString* original_selected_identifier_;
   std::unique_ptr<Browser> browser_;
-  ClosingWebStateObserver* closing_web_state_observer_;
-  std::unique_ptr<WebStateListObserverBridge>
-      closing_web_state_observer_bridge_;
 };
 
 #pragma mark - Consumer tests