SessionRestorationBrowserAgent to handle active webState changes

SessionRestorationBrowserAgent now observers WebStateList to monitor
the changes of the active webState, once the active webState is changed,
unless the new webstate is loading the session is saved.

This was handled previously by two observers:
- TabModelClosingWebStateObserver for changes that happen when the
  active webstate is closed.
- TabModelSelectedWebStateObserver for changes that happen when the
  active webstate is changed from one to another.
But it seems that we don't need two observers, we only need to observe
didChangeActiveWebState as it's also called when the active webState
detached. and the webStateList is locked after that happens so saving
the session at that point is the same as saving it on WillCloseWebState.

With that in place, TabModelSelectedWebStateObserver is not needed
anymore so it's deleted.

Also Changed TabModelClosingWebStateObserver to be
ClosingWebStateObserver as it has nothing to do with tabmodel. Updated
its initializer and usages. And in a later iteration i'll try to find
a place to move the functionalities that it does to other places.
(right now it handles tabRestoration and snapshot removal).

Bug: 1045142, 1010164
Change-Id: I27ef8bf027ebb641230b56aecdc599acf807c1ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2018147
Commit-Queue: Mohammad Refaat <mrefaat@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735524}

ios/chrome/browser/sessions/session_restoration_browser_agent.h

@@ -11,7 +11,9 @@
 
 #include "base/macros.h"
 #include "base/observer_list.h"
+#include "ios/chrome/browser/main/browser_observer.h"
 #include "ios/chrome/browser/main/browser_user_data.h"
+#include "ios/chrome/browser/web_state_list/web_state_list_observer.h"
 
 class ChromeBrowserState;
 @class SessionWindowIOS;
@@ -22,9 +24,12 @@ class WebStateList;
 
 // This class is responsible for handling requests of session restoration. It
 // can be observed via SeassonRestorationObserver which it uses to notify
-// observers of session restoration events.
+// observers of session restoration events. This class also automatically
+// save sessions when the active webState changes.
 class SessionRestorationBrowserAgent
-    : public BrowserUserData<SessionRestorationBrowserAgent> {
+    : BrowserObserver,
+      public BrowserUserData<SessionRestorationBrowserAgent>,
+      WebStateListObserver {
  public:
   // Creates an SessionRestorationBrowserAgent scoped to |browser|.
   static void CreateForBrowser(Browser* browser,
@@ -61,6 +66,16 @@ class SessionRestorationBrowserAgent
   // Returns true if the current session can be saved.
   bool CanSaveSession();
 
+  // BrowserObserver methods
+  void BrowserDestroyed(Browser* browser) override;
+
+  // WebStateListObserver methods
+  void WebStateActivatedAt(WebStateList* web_state_list,
+                           web::WebState* old_web_state,
+                           web::WebState* new_web_state,
+                           int active_index,
+                           int reason) override;
+
   // The service object which handles the actual saving of sessions.
   SessionServiceIOS* session_service_;
 
@@ -74,7 +89,7 @@ class SessionRestorationBrowserAgent
   // Session Factory used to create session data for saving.
   SessionIOSFactory* session_ios_factory_;
 
-  // true when session restoration is in progress.
+  // True when session restoration is in progress.
   bool restoring_session_ = false;
 };
 

ios/chrome/browser/sessions/session_restoration_browser_agent.mm

@@ -50,7 +50,10 @@ SessionRestorationBrowserAgent::SessionRestorationBrowserAgent(
       web_state_list_(browser->GetWebStateList()),
       browser_state_(browser->GetBrowserState()),
       session_ios_factory_(
-          [[SessionIOSFactory alloc] initWithWebStateList:web_state_list_]) {}
+          [[SessionIOSFactory alloc] initWithWebStateList:web_state_list_]) {
+  browser->AddObserver(this);
+  web_state_list_->AddObserver(this);
+}
 
 SessionRestorationBrowserAgent::~SessionRestorationBrowserAgent() {
   // Disconnect the session factory object as it's not granteed that it will be
@@ -180,3 +183,24 @@ bool SessionRestorationBrowserAgent::CanSaveSession() {
 
   return YES;
 }
+
+// Browser Observer methods:
+void SessionRestorationBrowserAgent::BrowserDestroyed(Browser* browser) {
+  DCHECK_EQ(browser->GetWebStateList(), web_state_list_);
+  browser->GetWebStateList()->RemoveObserver(this);
+  browser->RemoveObserver(this);
+}
+
+// WebStateList Observer methods:
+void SessionRestorationBrowserAgent::WebStateActivatedAt(
+    WebStateList* web_state_list,
+    web::WebState* old_web_state,
+    web::WebState* new_web_state,
+    int active_index,
+    int reason) {
+  if (new_web_state && new_web_state->IsLoading())
+    return;
+
+  // Persist the session state if the new web state is not loading.
+  SaveSession(/*immediately=*/false);
+}

ios/chrome/browser/sessions/session_restoration_browser_agent_unittest.mm

@@ -258,4 +258,41 @@ TEST_F(SessionRestorationBrowserAgentTest, ObserverCalledWithRestore) {
   session_restoration_agent_->RemoveObserver(&observer);
 }
 
+// Tests that SessionRestorationAgent saves session when the active webState
+// changes.
+TEST_F(SessionRestorationBrowserAgentTest,
+       SaveSessionWithActiveWebStateChange) {
+  InsertNewWebState(GURL(kURL1), /*parent=*/nullptr, /*index=*/0,
+                    /*background=*/true);
+  InsertNewWebState(GURL(kURL2), /*parent=*/nullptr, /*index=*/1,
+                    /*background=*/true);
+  EXPECT_EQ(test_session_service_.saveSessionCallsCount, 0);
+
+  // Inserting new active webState.
+  InsertNewWebState(GURL(kURL2), /*parent=*/nullptr, /*index=*/2,
+                    /*background=*/false);
+  EXPECT_EQ(test_session_service_.saveSessionCallsCount, 1);
+
+  // Removing the active webstate.
+  web_state_list_->CloseWebStateAt(/*index=*/2,
+                                   WebStateList::CLOSE_USER_ACTION);
+  EXPECT_EQ(test_session_service_.saveSessionCallsCount, 2);
+
+  EXPECT_EQ(web_state_list_->active_index(), 1);
+
+  // Activating another webState without removing or inserting.
+  web_state_list_->ActivateWebStateAt(/*index=*/0);
+  EXPECT_EQ(test_session_service_.saveSessionCallsCount, 3);
+
+  // Removing a non active webState.
+  web_state_list_->CloseWebStateAt(/*index=*/1,
+                                   WebStateList::CLOSE_USER_ACTION);
+  EXPECT_EQ(test_session_service_.saveSessionCallsCount, 3);
+
+  // Removing the last active webState.
+  web_state_list_->CloseWebStateAt(/*index=*/0,
+                                   WebStateList::CLOSE_USER_ACTION);
+  EXPECT_EQ(test_session_service_.saveSessionCallsCount, 4);
+}
+
 }  // anonymous namespace

ios/chrome/browser/sessions/test_session_service.h

@@ -11,12 +11,15 @@
 
 // Testing subclass of SessionService that immediately consumes session windows
 // passed to -saveSessionWindow:sessionPath:immediately: is consumed immediately
-// but only saved to disk if |performIO| is set to YES..
+// but only saved to disk if |performIO| is set to YES. Also it keeps track of
+// how many calls to saveSessionWindow have been done.
 @interface TestSessionService : SessionServiceIOS
 
 // If YES, then sessions are saved to disk, otherwise, data is discarded.
 @property(nonatomic, assign) BOOL performIO;
 
+@property(nonatomic, readonly) int saveSessionCallsCount;
+
 @end
 
 #endif  // IOS_CHROME_BROWSER_SESSIONS_TEST_SESSION_SERVICE_H_

ios/chrome/browser/sessions/test_session_service.mm

@@ -14,8 +14,6 @@
 
 @implementation TestSessionService
 
-@synthesize performIO = _performIO;
-
 - (instancetype)init {
   return [super initWithTaskRunner:base::ThreadTaskRunnerHandle::Get()];
 }
@@ -28,9 +26,9 @@
       [NSKeyedArchiver archivedDataWithRootObject:[factory sessionForSaving]
                             requiringSecureCoding:NO
                                             error:nil];
-  if (self.performIO) {
+  if (self.performIO)
     [self performSaveSessionData:data sessionPath:sessionPath];
-  }
+  _saveSessionCallsCount++;
 }
 
 @end

ios/chrome/browser/tabs/BUILD.gn

@@ -30,13 +30,11 @@ source_set("tabs") {
 
 source_set("tabs_internal") {
   sources = [
+    "closing_web_state_observer.h",
+    "closing_web_state_observer.mm",
     "tab_helper_util.mm",
     "tab_model.mm",
-    "tab_model_closing_web_state_observer.h",
-    "tab_model_closing_web_state_observer.mm",
     "tab_model_list.mm",
-    "tab_model_selected_tab_observer.h",
-    "tab_model_selected_tab_observer.mm",
     "tab_model_synced_window_delegate.mm",
     "tab_model_synced_window_delegate_getter.mm",
     "tab_parenting_observer.h",

ios/chrome/browser/tabs/tab_model_closing_web_state_observer.h → ios/chrome/browser/tabs/closing_web_state_observer.h

@@ -2,27 +2,27 @@
 // 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_TAB_MODEL_CLOSING_WEB_STATE_OBSERVER_H_
-#define IOS_CHROME_BROWSER_TABS_TAB_MODEL_CLOSING_WEB_STATE_OBSERVER_H_
+#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"
 
-@class TabModel;
 namespace sessions {
 class TabRestoreService;
 }
 
 // Observes WebStateList events about closing WebState.
-@interface TabModelClosingWebStateObserver : NSObject<WebStateListObserving>
+// TODO(crbug.com/1045142): Find a suitable location for the functionalities
+// handled by this class and then remove it.
+@interface ClosingWebStateObserver : NSObject <WebStateListObserving>
 
-- (instancetype)initWithTabModel:(TabModel*)tabModel
-                  restoreService:(sessions::TabRestoreService*)restoreService
-    NS_DESIGNATED_INITIALIZER;
+- (instancetype)initWithRestoreService:
+    (sessions::TabRestoreService*)restoreService NS_DESIGNATED_INITIALIZER;
 
 - (instancetype)init NS_UNAVAILABLE;
 
 @end
 
-#endif  // IOS_CHROME_BROWSER_TABS_TAB_MODEL_CLOSING_WEB_STATE_OBSERVER_H_
+#endif  // IOS_CHROME_BROWSER_TABS_CLOSING_WEB_STATE_OBSERVER_H_

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

@@ -2,7 +2,7 @@
 // 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/tab_model_closing_web_state_observer.h"
+#import "ios/chrome/browser/tabs/closing_web_state_observer.h"
 
 #include "base/logging.h"
 #include "base/strings/string_piece.h"
@@ -11,7 +11,6 @@
 #include "components/sessions/ios/ios_restore_live_tab.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
 #import "ios/chrome/browser/snapshots/snapshot_tab_helper.h"
-#import "ios/chrome/browser/tabs/tab_model.h"
 #import "ios/chrome/browser/web_state_list/web_state_list.h"
 #import "ios/web/public/navigation/navigation_item.h"
 #import "ios/web/public/navigation/navigation_manager.h"
@@ -22,23 +21,15 @@
 #error "This file requires ARC support."
 #endif
 
-@implementation TabModelClosingWebStateObserver {
-  __weak TabModel* _tabModel;
+@implementation ClosingWebStateObserver {
   sessions::TabRestoreService* _restoreService;
-
-  // Records whether the last detached WebState was the active WebState.
-  // If YES, then the session is saved if the WebState is closed.
-  BOOL _lastDetachedWebStateWasActive;
 }
 
-- (instancetype)initWithTabModel:(TabModel*)tabModel
-                  restoreService:(sessions::TabRestoreService*)restoreService {
-  DCHECK(tabModel);
+- (instancetype)initWithRestoreService:
+    (sessions::TabRestoreService*)restoreService {
   self = [super init];
   if (self) {
-    _tabModel = tabModel;
     _restoreService = restoreService;
-    _lastDetachedWebStateWasActive = NO;
   }
   return self;
 }
@@ -55,7 +46,6 @@
 - (void)webStateList:(WebStateList*)webStateList
     willDetachWebState:(web::WebState*)webState
                atIndex:(int)atIndex {
-  _lastDetachedWebStateWasActive = webStateList->active_index() == atIndex;
   [self recordHistoryForWebState:webState atIndex:atIndex];
 }
 
@@ -63,10 +53,6 @@
     willCloseWebState:(web::WebState*)webState
               atIndex:(int)atIndex
            userAction:(BOOL)userAction {
-  if (_lastDetachedWebStateWasActive) {
-    _lastDetachedWebStateWasActive = NO;
-    [_tabModel saveSessionImmediately:NO];
-  }
   if (userAction) {
     SnapshotTabHelper::FromWebState(webState)->RemoveSnapshot();
   }

ios/chrome/browser/tabs/tab_model.mm

@@ -37,9 +37,8 @@
 #import "ios/chrome/browser/sessions/session_window_ios.h"
 #import "ios/chrome/browser/snapshots/snapshot_cache.h"
 #import "ios/chrome/browser/snapshots/snapshot_cache_factory.h"
-#import "ios/chrome/browser/tabs/tab_model_closing_web_state_observer.h"
+#import "ios/chrome/browser/tabs/closing_web_state_observer.h"
 #import "ios/chrome/browser/tabs/tab_model_list.h"
-#import "ios/chrome/browser/tabs/tab_model_selected_tab_observer.h"
 #import "ios/chrome/browser/tabs/tab_model_synced_window_delegate.h"
 #import "ios/chrome/browser/tabs/tab_parenting_observer.h"
 #import "ios/chrome/browser/web/tab_id_tab_helper.h"
@@ -311,29 +310,20 @@ void RecordMainFrameNavigationMetric(web::WebState* web_state) {
     NSMutableArray<id<WebStateListObserving>>* retainedWebStateListObservers =
         [[NSMutableArray alloc] init];
 
-    TabModelClosingWebStateObserver* tabModelClosingWebStateObserver =
-        [[TabModelClosingWebStateObserver alloc]
-            initWithTabModel:self
-              restoreService:IOSChromeTabRestoreServiceFactory::
-                                 GetForBrowserState(_browserState)];
-    [retainedWebStateListObservers addObject:tabModelClosingWebStateObserver];
+    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>(
-            tabModelClosingWebStateObserver));
+        std::make_unique<WebStateListObserverBridge>(closingWebStateObserver));
 
     _webStateListObservers.push_back(std::make_unique<TabParentingObserver>());
 
-    TabModelSelectedTabObserver* tabModelSelectedTabObserver =
-        [[TabModelSelectedTabObserver alloc] initWithTabModel:self];
-    [retainedWebStateListObservers addObject:tabModelSelectedTabObserver];
-    _webStateListObservers.push_back(
-        std::make_unique<WebStateListObserverBridge>(
-            tabModelSelectedTabObserver));
-
     auto webStateListMetricsObserver =
         std::make_unique<WebStateListMetricsObserver>();
     _webStateListMetricsObserver = webStateListMetricsObserver.get();

ios/chrome/browser/tabs/tab_model_selected_tab_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_TAB_MODEL_SELECTED_TAB_OBSERVER_H_
-#define IOS_CHROME_BROWSER_TABS_TAB_MODEL_SELECTED_TAB_OBSERVER_H_
-
-#import <Foundation/Foundation.h>
-
-#import "ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h"
-
-@class TabModel;
-
-// Listen to WebStateList active WebState change events and then save the
-// old Tab state, fire notification and save the session if necessary.
-@interface TabModelSelectedTabObserver : NSObject<WebStateListObserving>
-
-- (instancetype)initWithTabModel:(TabModel*)tabModel NS_DESIGNATED_INITIALIZER;
-
-- (instancetype)init NS_UNAVAILABLE;
-
-@end
-
-#endif  // IOS_CHROME_BROWSER_TABS_TAB_MODEL_SELECTED_TAB_OBSERVER_H_

ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm

-// 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.
-
-#import "ios/chrome/browser/tabs/tab_model_selected_tab_observer.h"
-
-#include "base/logging.h"
-#import "ios/chrome/browser/tabs/tab_model.h"
-#import "ios/web/public/web_state.h"
-
-#if !defined(__has_feature) || !__has_feature(objc_arc)
-#error "This file requires ARC support."
-#endif
-
-@implementation TabModelSelectedTabObserver {
-  __weak TabModel* _tabModel;
-}
-
-- (instancetype)initWithTabModel:(TabModel*)tabModel {
-  DCHECK(tabModel);
-  if ((self = [super init])) {
-    _tabModel = tabModel;
-  }
-  return self;
-}
-
-#pragma mark WebStateListObserving
-
-- (void)webStateList:(WebStateList*)webStateList
-    didChangeActiveWebState:(web::WebState*)newWebState
-                oldWebState:(web::WebState*)oldWebState
-                    atIndex:(int)atIndex
-                     reason:(int)reason {
-
-  if (newWebState && !newWebState->IsLoading()) {
-    // Persist the session state.
-    [_tabModel saveSessionImmediately:NO];
-  }
-}
-
-@end

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

@@ -19,8 +19,7 @@
 #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_tab_helper.h"
-#import "ios/chrome/browser/tabs/tab_model.h"
-#import "ios/chrome/browser/tabs/tab_model_closing_web_state_observer.h"
+#import "ios/chrome/browser/tabs/closing_web_state_observer.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"
@@ -220,20 +219,15 @@ class TabGridMediatorTest : public PlatformTest {
     web_state_list_ =
         std::make_unique<WebStateList>(web_state_list_delegate_.get());
     tab_restore_service_ = std::make_unique<FakeTabRestoreService>();
-    tab_model_ = OCMClassMock([TabModel class]);
-    OCMStub([tab_model_ webStateList]).andReturn(web_state_list_.get());
-    OCMStub([tab_model_ browserState]).andReturn(browser_state_.get());
-    tab_model_closing_web_state_observer_ =
-        [[TabModelClosingWebStateObserver alloc]
-            initWithTabModel:tab_model_
-              restoreService:tab_restore_service_.get()];
-    tab_model_closing_web_state_observer_bridge_ =
+    closing_web_state_observer_ = [[ClosingWebStateObserver alloc]
+        initWithRestoreService:tab_restore_service_.get()];
+    closing_web_state_observer_bridge_ =
         std::make_unique<WebStateListObserverBridge>(
-            tab_model_closing_web_state_observer_);
-    web_state_list_->AddObserver(
-        tab_model_closing_web_state_observer_bridge_.get());
+            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(), tab_model_);
+    browser_ = std::make_unique<TestBrowser>(browser_state_.get(),
+                                             web_state_list_.get());
 
     // Insert some web states.
     for (int i = 0; i < 3; i++) {
@@ -276,8 +270,7 @@ class TabGridMediatorTest : public PlatformTest {
   }
 
   void TearDown() override {
-    web_state_list_->RemoveObserver(
-        tab_model_closing_web_state_observer_bridge_.get());
+    web_state_list_->RemoveObserver(closing_web_state_observer_bridge_.get());
     PlatformTest::TearDown();
   }
 
@@ -301,9 +294,9 @@ class TabGridMediatorTest : public PlatformTest {
   NSSet<NSString*>* original_identifiers_;
   NSString* original_selected_identifier_;
   std::unique_ptr<Browser> browser_;
-  TabModelClosingWebStateObserver* tab_model_closing_web_state_observer_;
+  ClosingWebStateObserver* closing_web_state_observer_;
   std::unique_ptr<WebStateListObserverBridge>
-      tab_model_closing_web_state_observer_bridge_;
+      closing_web_state_observer_bridge_;
 };
 
 #pragma mark - Consumer tests