Add support for batched operations to WebStateList

Some observers of WebStateList are interested in grouping multiple
mutations of the WebStateList as a single operation (e.g. "closing
all tabs" or "restoring a session").

Add WillBeginBatchOperation() and BatchOperationEnded() methods to
WebStateListObserver that can be used to detect those operations
(and to avoid expensives UI events during the batch).

Sending the notifications is managed by PerformBatchOperation that
executes a callback surrounded by invoking the two methonds on the
observers.

Bug: 1014526
Change-Id: Ieb3ffa5ab349159b333dc651314aa594713ce94e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865161
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Auto-Submit: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706881}

ios/chrome/browser/web_state_list/web_state_list.h

@@ -159,6 +159,12 @@ class WebStateList {
   // Removes an observer from the model.
   void RemoveObserver(WebStateListObserver* observer);
 
+  // Performs mutating operations on the WebStateList as batched operation.
+  // The observers will be notified by WillBeginBatchOperation() before the
+  // |operation| callback is executed and by BatchOperationEnded() after it
+  // has completed.
+  void PerformBatchOperation(base::OnceCallback<void(WebStateList*)> operation);
+
   // Invalid index.
   static const int kInvalidIndex = -1;
 

ios/chrome/browser/web_state_list/web_state_list.mm

@@ -333,13 +333,13 @@ void WebStateList::CloseWebStateAt(int index, int close_flags) {
 }
 
 void WebStateList::CloseAllWebStates(int close_flags) {
-  const bool user_action = IsClosingFlagSet(close_flags, CLOSE_USER_ACTION);
-  for (auto& observer : observers_)
-    observer.WillCloseAllWebStates(this, user_action);
-  while (!empty())
-    CloseWebStateAt(count() - 1, close_flags);
-  for (auto& observer : observers_)
-    observer.DidCloseAllWebStates(this, user_action);
+  PerformBatchOperation(base::BindOnce(
+      [](int close_flags, WebStateList* web_state_list) {
+        while (!web_state_list->empty())
+          web_state_list->CloseWebStateAt(web_state_list->count() - 1,
+                                          close_flags);
+      },
+      close_flags));
 }
 
 void WebStateList::ActivateWebStateAt(int index) {
@@ -358,6 +358,16 @@ void WebStateList::RemoveObserver(WebStateListObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void WebStateList::PerformBatchOperation(
+    base::OnceCallback<void(WebStateList*)> operation) {
+  for (auto& observer : observers_)
+    observer.WillBeginBatchOperation(this);
+  if (!operation.is_null())
+    std::move(operation).Run(this);
+  for (auto& observer : observers_)
+    observer.BatchOperationEnded(this);
+}
+
 void WebStateList::ClearOpenersReferencing(int index) {
   web::WebState* old_web_state = web_state_wrappers_[index]->web_state();
   for (auto& web_state_wrapper : web_state_wrappers_) {

ios/chrome/browser/web_state_list/web_state_list_observer.h

@@ -85,15 +85,17 @@ class WebStateListObserver {
                                    int active_index,
                                    int reason);
 
-  // Invoked before all WebStates are closed in the WebStateList. If the
-  // WebState is closed due to user action, |user_action| will be true.
-  virtual void WillCloseAllWebStates(WebStateList* web_state_list,
-                                     bool user_action);
-
-  // Invoked after all WebStates are closed in the WebStateList. If the WebState
-  // is closed due to user action, |user_action| will be true.
-  virtual void DidCloseAllWebStates(WebStateList* web_state_list,
-                                    bool user_action);
+  // Invoked before a batched operations begins. The observer can use this
+  // notification if it is interested in considering all those individual
+  // operations as a single mutation of the WebStateList (e.g. considering
+  // insertion of multiple tabs as a restoration operation).
+  virtual void WillBeginBatchOperation(WebStateList* web_state_list);
+
+  // Invoked after the completion of batched operations. The observer can
+  // investigate the state of the WebStateList to detect any changes that
+  // were performed on it during the batch (e.g. detect that all tabs were
+  // closed at once).
+  virtual void BatchOperationEnded(WebStateList* web_state_list);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(WebStateListObserver);

ios/chrome/browser/web_state_list/web_state_list_observer.mm

@@ -46,8 +46,7 @@ void WebStateListObserver::WebStateActivatedAt(WebStateList* web_state_list,
                                                int active_index,
                                                int reason) {}
 
-void WebStateListObserver::WillCloseAllWebStates(WebStateList* web_state_list,
-                                                 bool user_action) {}
+void WebStateListObserver::WillBeginBatchOperation(
+    WebStateList* web_state_list) {}
 
-void WebStateListObserver::DidCloseAllWebStates(WebStateList* web_state_list,
-                                                bool user_action) {}
+void WebStateListObserver::BatchOperationEnded(WebStateList* web_state_list) {}

ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h

@@ -68,58 +68,58 @@
                     atIndex:(int)atIndex
                      reason:(int)reason;
 
-// Invoked before all the WebStates are closed in the WebStateList. If the
-// WebState is closed due to user action, |userAction| will be true.
-- (void)webStateList:(WebStateList*)webStateList
-    willCloseAllWebStatesUserAction:(BOOL)userAction;
-
-// Invoked after all the WebStates are closed in the WebStateList. If the
-// WebState is closed due to user action, |userAction| will be true.
-- (void)webStateList:(WebStateList*)webStateList
-    didCloseAllWebStatesUserAction:(BOOL)userAction;
+// Invoked before a batched operations begins. The observer can use this
+// notification if it is interested in considering all those individual
+// operations as a single mutation of the WebStateList (e.g. considering
+// insertion of multiple tabs as a restoration operation).
+- (void)webStateListWillBeginBatchOperation:(WebStateList*)webStateList;
+
+// Invoked after the completion of batched operations. The observer can
+// investigate the state of the WebStateList to detect any changes that
+// were performed on it during the batch (e.g. detect that all tabs were
+// closed at once).
+- (void)webStateListBatchOperationEnded:(WebStateList*)webStateList;
 
 @end
 
 // Observer that bridges WebStateList events to an Objective-C observer that
 // implements the WebStateListObserver protocol (the observer is *not* owned).
-class WebStateListObserverBridge : public WebStateListObserver {
+class WebStateListObserverBridge final : public WebStateListObserver {
  public:
   explicit WebStateListObserverBridge(id<WebStateListObserving> observer);
-  ~WebStateListObserverBridge() override;
+  ~WebStateListObserverBridge() final;
 
  private:
   // WebStateListObserver implementation.
   void WebStateInsertedAt(WebStateList* web_state_list,
                           web::WebState* web_state,
                           int index,
-                          bool activating) override;
+                          bool activating) final;
   void WebStateMoved(WebStateList* web_state_list,
                      web::WebState* web_state,
                      int from_index,
-                     int to_index) override;
+                     int to_index) final;
   void WebStateReplacedAt(WebStateList* web_state_list,
                           web::WebState* old_web_state,
                           web::WebState* new_web_state,
-                          int index) override;
+                          int index) final;
   void WillDetachWebStateAt(WebStateList* web_state_list,
                             web::WebState* web_state,
-                            int index) override;
+                            int index) final;
   void WebStateDetachedAt(WebStateList* web_state_list,
                           web::WebState* web_state,
-                          int index) override;
+                          int index) final;
   void WillCloseWebStateAt(WebStateList* web_state_list,
                            web::WebState* web_state,
                            int index,
-                           bool user_action) override;
+                           bool user_action) final;
   void WebStateActivatedAt(WebStateList* web_state_list,
                            web::WebState* old_web_state,
                            web::WebState* new_web_state,
                            int active_index,
-                           int reason) override;
-  void WillCloseAllWebStates(WebStateList* web_state_list,
-                             bool user_action) override;
-  void DidCloseAllWebStates(WebStateList* web_state_list,
-                            bool user_action) override;
+                           int reason) final;
+  void WillBeginBatchOperation(WebStateList* web_state_list) final;
+  void BatchOperationEnded(WebStateList* web_state_list) final;
 
   __weak id<WebStateListObserving> observer_ = nil;
 

ios/chrome/browser/web_state_list/web_state_list_observer_bridge.mm

@@ -121,24 +121,20 @@ void WebStateListObserverBridge::WebStateActivatedAt(
                        reason:reason];
 }
 
-void WebStateListObserverBridge::WillCloseAllWebStates(
-    WebStateList* web_state_list,
-    bool user_action) {
-  const SEL selector = @selector(webStateList:willCloseAllWebStatesUserAction:);
+void WebStateListObserverBridge::WillBeginBatchOperation(
+    WebStateList* web_state_list) {
+  const SEL selector = @selector(webStateListWillBeginBatchOperation:);
   if (![observer_ respondsToSelector:selector])
     return;
 
-  [observer_ webStateList:web_state_list
-      willCloseAllWebStatesUserAction:(user_action ? YES : NO)];
+  [observer_ webStateListWillBeginBatchOperation:web_state_list];
 }
 
-void WebStateListObserverBridge::DidCloseAllWebStates(
-    WebStateList* web_state_list,
-    bool user_action) {
-  const SEL selector = @selector(webStateList:didCloseAllWebStatesUserAction:);
+void WebStateListObserverBridge::BatchOperationEnded(
+    WebStateList* web_state_list) {
+  const SEL selector = @selector(webStateListBatchOperationEnded:);
   if (![observer_ respondsToSelector:selector])
     return;
 
-  [observer_ webStateList:web_state_list
-      didCloseAllWebStatesUserAction:(user_action ? YES : NO)];
+  [observer_ webStateListBatchOperationEnded:web_state_list];
 }

ios/chrome/browser/web_state_list/web_state_list_serialization.mm

@@ -27,47 +27,12 @@ namespace {
 // the WebStates stored in the WebStateList.
 NSString* const kOpenerIndexKey = @"OpenerIndex";
 NSString* const kOpenerNavigationIndexKey = @"OpenerNavigationIndex";
-}  // namespace
-
-SessionWindowIOS* SerializeWebStateList(WebStateList* web_state_list) {
-  NSMutableArray<CRWSessionStorage*>* serialized_session =
-      [NSMutableArray arrayWithCapacity:web_state_list->count()];
 
-  for (int index = 0; index < web_state_list->count(); ++index) {
-    web::WebState* web_state = web_state_list->GetWebStateAt(index);
-    WebStateOpener opener = web_state_list->GetOpenerOfWebStateAt(index);
-
-    web::SerializableUserDataManager* user_data_manager =
-        web::SerializableUserDataManager::FromWebState(web_state);
-
-    int opener_index = WebStateList::kInvalidIndex;
-    if (opener.opener) {
-      opener_index = web_state_list->GetIndexOfWebState(opener.opener);
-      DCHECK_NE(opener_index, WebStateList::kInvalidIndex);
-      user_data_manager->AddSerializableData(@(opener_index), kOpenerIndexKey);
-      user_data_manager->AddSerializableData(@(opener.navigation_index),
-                                             kOpenerNavigationIndexKey);
-    } else {
-      user_data_manager->AddSerializableData([NSNull null], kOpenerIndexKey);
-      user_data_manager->AddSerializableData([NSNull null],
-                                             kOpenerNavigationIndexKey);
-    }
-
-    [serialized_session addObject:web_state->BuildSessionStorage()];
-  }
-
-  NSUInteger selectedIndex =
-      web_state_list->active_index() != WebStateList::kInvalidIndex
-          ? static_cast<NSUInteger>(web_state_list->active_index())
-          : static_cast<NSUInteger>(NSNotFound);
-
-  return [[SessionWindowIOS alloc] initWithSessions:[serialized_session copy]
-                                      selectedIndex:selectedIndex];
-}
-
-void DeserializeWebStateList(WebStateList* web_state_list,
-                             SessionWindowIOS* session_window,
-                             const WebStateFactory& web_state_factory) {
+// Helper for DeserializeWebStateList allowing the mutation to appears as a
+// single batched operation.
+void DeserializeWebStateListHelper(SessionWindowIOS* session_window,
+                                   const WebStateFactory& web_state_factory,
+                                   WebStateList* web_state_list) {
   const int old_count = web_state_list->count();
   for (CRWSessionStorage* session in session_window.sessions) {
     std::unique_ptr<web::WebState> web_state = web_state_factory.Run(session);
@@ -110,3 +75,48 @@ void DeserializeWebStateList(WebStateList* web_state_list,
         old_count + static_cast<int>(session_window.selectedIndex));
   }
 }
+}  // namespace
+
+SessionWindowIOS* SerializeWebStateList(WebStateList* web_state_list) {
+  NSMutableArray<CRWSessionStorage*>* serialized_session =
+      [NSMutableArray arrayWithCapacity:web_state_list->count()];
+
+  for (int index = 0; index < web_state_list->count(); ++index) {
+    web::WebState* web_state = web_state_list->GetWebStateAt(index);
+    WebStateOpener opener = web_state_list->GetOpenerOfWebStateAt(index);
+
+    web::SerializableUserDataManager* user_data_manager =
+        web::SerializableUserDataManager::FromWebState(web_state);
+
+    int opener_index = WebStateList::kInvalidIndex;
+    if (opener.opener) {
+      opener_index = web_state_list->GetIndexOfWebState(opener.opener);
+      DCHECK_NE(opener_index, WebStateList::kInvalidIndex);
+      user_data_manager->AddSerializableData(@(opener_index), kOpenerIndexKey);
+      user_data_manager->AddSerializableData(@(opener.navigation_index),
+                                             kOpenerNavigationIndexKey);
+    } else {
+      user_data_manager->AddSerializableData([NSNull null], kOpenerIndexKey);
+      user_data_manager->AddSerializableData([NSNull null],
+                                             kOpenerNavigationIndexKey);
+    }
+
+    [serialized_session addObject:web_state->BuildSessionStorage()];
+  }
+
+  NSUInteger selectedIndex =
+      web_state_list->active_index() != WebStateList::kInvalidIndex
+          ? static_cast<NSUInteger>(web_state_list->active_index())
+          : static_cast<NSUInteger>(NSNotFound);
+
+  return [[SessionWindowIOS alloc] initWithSessions:[serialized_session copy]
+                                      selectedIndex:selectedIndex];
+}
+
+void DeserializeWebStateList(WebStateList* web_state_list,
+                             SessionWindowIOS* session_window,
+                             const WebStateFactory& web_state_factory) {
+  web_state_list->PerformBatchOperation(
+      base::BindOnce(&DeserializeWebStateListHelper,
+                     base::Unretained(session_window), web_state_factory));
+}

ios/chrome/browser/web_state_list/web_state_list_unittest.mm

@@ -38,8 +38,8 @@ class WebStateListTestObserver : public WebStateListObserver {
     web_state_replaced_called_ = false;
     web_state_detached_called_ = false;
     web_state_activated_called_ = false;
-    will_close_all_webstates_called_ = false;
-    did_close_all_webstates_called_ = false;
+    batch_operation_started_ = false;
+    batch_operation_ended_ = false;
   }
 
   // Returns whether WebStateInsertedAt was invoked.
@@ -59,15 +59,11 @@ class WebStateListTestObserver : public WebStateListObserver {
     return web_state_activated_called_;
   }
 
-  // Returns whether WillCloseAllWebStates was invoked.
-  bool will_close_all_webstates_called() const {
-    return will_close_all_webstates_called_;
-  }
+  // Returns whether WillBeginBatchOperation was invoked.
+  bool batch_operation_started() const { return batch_operation_started_; }
 
-  // Returns whether DidCloseAllWebStates was invoked.
-  bool did_close_all_webstates_called() const {
-    return did_close_all_webstates_called_;
-  }
+  // Returns whether BatchOperationEnded was invoked.
+  bool batch_operation_ended() const { return batch_operation_ended_; }
 
   // WebStateListObserver implementation.
   void WebStateInsertedAt(WebStateList* web_state_list,
@@ -108,14 +104,12 @@ class WebStateListTestObserver : public WebStateListObserver {
     web_state_activated_called_ = true;
   }
 
-  void WillCloseAllWebStates(WebStateList* web_state_list,
-                             bool user_action) override {
-    will_close_all_webstates_called_ = true;
+  void WillBeginBatchOperation(WebStateList* web_state_list) override {
+    batch_operation_started_ = true;
   }
 
-  void DidCloseAllWebStates(WebStateList* web_state_list,
-                            bool user_action) override {
-    did_close_all_webstates_called_ = true;
+  void BatchOperationEnded(WebStateList* web_state_list) override {
+    batch_operation_ended_ = true;
   }
 
  private:
@@ -124,8 +118,8 @@ class WebStateListTestObserver : public WebStateListObserver {
   bool web_state_replaced_called_ = false;
   bool web_state_detached_called_ = false;
   bool web_state_activated_called_ = false;
-  bool will_close_all_webstates_called_ = false;
-  bool did_close_all_webstates_called_ = false;
+  bool batch_operation_started_ = false;
+  bool batch_operation_ended_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(WebStateListTestObserver);
 };
@@ -733,8 +727,8 @@ TEST_F(WebStateListTest, CloseAllWebStates) {
   EXPECT_EQ(0, web_state_list_.count());
 
   EXPECT_TRUE(observer_.web_state_detached_called());
-  EXPECT_TRUE(observer_.will_close_all_webstates_called());
-  EXPECT_TRUE(observer_.did_close_all_webstates_called());
+  EXPECT_TRUE(observer_.batch_operation_started());
+  EXPECT_TRUE(observer_.batch_operation_ended());
 }
 
 // Test closing one webstate.
@@ -751,6 +745,28 @@ TEST_F(WebStateListTest, CloseWebState) {
 
   EXPECT_EQ(2, web_state_list_.count());
   EXPECT_TRUE(observer_.web_state_detached_called());
-  EXPECT_FALSE(observer_.will_close_all_webstates_called());
-  EXPECT_FALSE(observer_.did_close_all_webstates_called());
+  EXPECT_FALSE(observer_.batch_operation_started());
+  EXPECT_FALSE(observer_.batch_operation_ended());
+}
+
+// Test that batch operation can be empty.
+TEST_F(WebStateListTest, PerformBatchOperation_EmptyCallback) {
+  observer_.ResetStatistics();
+
+  web_state_list_.PerformBatchOperation({});
+
+  EXPECT_TRUE(observer_.batch_operation_started());
+  EXPECT_TRUE(observer_.batch_operation_ended());
+}
+
+// Test that batch operation WebStateList is the correct one.
+TEST_F(WebStateListTest, PerformBatchOperation_CorrectWebStateList) {
+  WebStateList* captured_web_state_list = nullptr;
+  web_state_list_.PerformBatchOperation(base::BindOnce(
+      [](WebStateList** captured_web_state_list, WebStateList* web_state_list) {
+        *captured_web_state_list = web_state_list;
+      },
+      &captured_web_state_list));
+
+  EXPECT_EQ(captured_web_state_list, &web_state_list_);
 }