Support batch WebStateList mutations in BreadcrumbManagerBrowserAgent

WebStateList supports batch operation (f.e. used in Close All Tabs
and Undo Close All Tabs actions). These batch operations may call
the same WebStateListObserver callback multiple times which creates
a lot of noise in breadcrumb logs.

This CL adds support for Insert and Close batch operations and only
logs the number of inserted or closed WebState objects.

Additional changes:
 - Remove WebStateDetachedAt because it's always coupled with
   WillCloseWebStateAt and does not add anything on its own
 - Remove _ from "Browser_<id>" to be consistent with logging
   "Tab<id>"

Bug: 1046231
Change-Id: I11429a281bf8d7c343132c02e4b92f463aa8a7ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2082484
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Auto-Submit: Eugene But <eugenebut@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746289}

ios/chrome/browser/crash_report/breadcrumbs/breadcrumb_manager_browser_agent.h

@@ -34,6 +34,8 @@ class BreadcrumbManagerBrowserAgent
   bool IsLoggingEnabled();
   void SetLoggingEnabled(bool enabled);
 
+  ~BreadcrumbManagerBrowserAgent() override;
+
  private:
   explicit BreadcrumbManagerBrowserAgent(Browser* browser);
   friend class BrowserUserData<BreadcrumbManagerBrowserAgent>;
@@ -64,9 +66,6 @@ class BreadcrumbManagerBrowserAgent
                           web::WebState* old_web_state,
                           web::WebState* new_web_state,
                           int index) override;
-  void WebStateDetachedAt(WebStateList* web_state_list,
-                          web::WebState* web_state,
-                          int index) override;
   void WillCloseWebStateAt(WebStateList* web_state_list,
                            web::WebState* web_state,
                            int index,
@@ -76,6 +75,8 @@ class BreadcrumbManagerBrowserAgent
                            web::WebState* new_web_state,
                            int active_index,
                            int reason) override;
+  void WillBeginBatchOperation(WebStateList* web_state_list) override;
+  void BatchOperationEnded(WebStateList* web_state_list) override;
 
   // Unique (across this application run only) identifier for logs associated
   // with |browser_| instance. Used to differentiate logs associated with the
@@ -84,6 +85,19 @@ class BreadcrumbManagerBrowserAgent
   // Whether or not events will be logged.
   bool logging_enabled_ = true;
   Browser* browser_ = nullptr;
+
+  // Keeps track of WebState mutation count to avoid logging every event.
+  // Created in WillBeginBatchOperation and destroyed in BatchOperationEnded.
+  // Final mutation count is logged in BatchOperationEnded.
+  struct BatchOperation {
+    // Number of WebState objects inserted between WillBeginBatchOperation and
+    // BatchOperationEnded callbacks.
+    int insertion_count = 0;
+    // Number of WebState objects closed between WillBeginBatchOperation and
+    // BatchOperationEnded callbacks.
+    int close_count = 0;
+  };
+  std::unique_ptr<BatchOperation> batch_operation_;
 };
 
 #endif  // IOS_CHROME_BROWSER_CRASH_REPORT_BREADCRUMBS_BREADCRUMB_MANAGER_BROWSER_AGENT_H_

ios/chrome/browser/crash_report/breadcrumbs/breadcrumb_manager_browser_agent.mm

@@ -28,6 +28,8 @@ BreadcrumbManagerBrowserAgent::BreadcrumbManagerBrowserAgent(Browser* browser)
   browser_->GetWebStateList()->AddObserver(this);
 }
 
+BreadcrumbManagerBrowserAgent::~BreadcrumbManagerBrowserAgent() = default;
+
 bool BreadcrumbManagerBrowserAgent::IsLoggingEnabled() {
   return logging_enabled_;
 }
@@ -50,7 +52,7 @@ void BreadcrumbManagerBrowserAgent::LogEvent(const std::string& event) {
       BreadcrumbManagerKeyedServiceFactory::GetInstance()->GetForBrowserState(
           browser_->GetBrowserState());
   breadcrumb_manager->AddEvent(
-      base::StringPrintf("Browser_%d %s", unique_id_, event.c_str()));
+      base::StringPrintf("Browser%d %s", unique_id_, event.c_str()));
 }
 
 void BreadcrumbManagerBrowserAgent::WebStateInsertedAt(
@@ -58,6 +60,11 @@ void BreadcrumbManagerBrowserAgent::WebStateInsertedAt(
     web::WebState* web_state,
     int index,
     bool activating) {
+  if (batch_operation_) {
+    ++batch_operation_->insertion_count;
+    return;
+  }
+
   int web_state_id =
       BreadcrumbManagerTabHelper::FromWebState(web_state)->GetUniqueId();
   const char* activating_string = activating ? "active" : "inactive";
@@ -85,19 +92,16 @@ void BreadcrumbManagerBrowserAgent::WebStateReplacedAt(
   LogEvent(base::StringPrintf("Replaced Tab%d with Tab%d at %d",
                               old_web_state_id, new_web_state_id, index));
 }
-void BreadcrumbManagerBrowserAgent::WebStateDetachedAt(
-    WebStateList* web_state_list,
-    web::WebState* web_state,
-    int index) {
-  int web_state_id =
-      BreadcrumbManagerTabHelper::FromWebState(web_state)->GetUniqueId();
-  LogEvent(base::StringPrintf("Tab%d detached at %d", web_state_id, index));
-}
 void BreadcrumbManagerBrowserAgent::WillCloseWebStateAt(
     WebStateList* web_state_list,
     web::WebState* web_state,
     int index,
     bool user_action) {
+  if (batch_operation_) {
+    ++batch_operation_->close_count;
+    return;
+  }
+
   int web_state_id =
       BreadcrumbManagerTabHelper::FromWebState(web_state)->GetUniqueId();
   const char* user_action_string = user_action ? " by user action" : "";
@@ -137,3 +141,23 @@ void BreadcrumbManagerBrowserAgent::WebStateActivatedAt(
                               old_web_state_id, change_reason_string,
                               new_web_state_id, active_index));
 }
+
+void BreadcrumbManagerBrowserAgent::WillBeginBatchOperation(
+    WebStateList* web_state_list) {
+  batch_operation_ = std::make_unique<BatchOperation>();
+}
+
+void BreadcrumbManagerBrowserAgent::BatchOperationEnded(
+    WebStateList* web_state_list) {
+  if (batch_operation_) {
+    if (batch_operation_->insertion_count > 0) {
+      LogEvent(base::StringPrintf("Inserted %d tabs",
+                                  batch_operation_->insertion_count));
+    }
+    if (batch_operation_->close_count > 0) {
+      LogEvent(
+          base::StringPrintf("Closed %d tabs", batch_operation_->close_count));
+    }
+  }
+  batch_operation_.reset();
+}

ios/chrome/browser/crash_report/breadcrumbs/breadcrumb_manager_browser_agent_unittest.mm

@@ -4,6 +4,7 @@
 
 #import "ios/chrome/browser/crash_report/breadcrumbs/breadcrumb_manager_browser_agent.h"
 
+#include "base/bind.h"
 #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
 #include "ios/chrome/browser/browser_state/test_chrome_browser_state_manager.h"
 #include "ios/chrome/browser/crash_report/breadcrumbs/breadcrumb_manager_keyed_service.h"
@@ -123,3 +124,40 @@ TEST_F(BreadcrumbManagerBrowserAgentTest, MultipleBrowsers) {
       browser2_split_pos, events.back().length() - browser2_split_pos);
   EXPECT_STRNE(browser1_end.c_str(), browser2_end.c_str());
 }
+
+// Tests WebStateList's batch insertion and closing.
+TEST_F(BreadcrumbManagerBrowserAgentTest, BatchOperations) {
+  WebStateList web_state_list(&web_state_list_delegate_);
+  std::unique_ptr<Browser> browser =
+      std::make_unique<TestBrowser>(browser_state_.get(), &web_state_list);
+  BreadcrumbManagerBrowserAgent::CreateForBrowser(browser.get());
+
+  // Insert multiple WebStates.
+  web_state_list.PerformBatchOperation(base::BindOnce(^(WebStateList* list) {
+    web::WebState::CreateParams create_params(browser_state_.get());
+    list->InsertWebState(
+        /*index=*/0, web::WebState::Create(create_params),
+        WebStateList::InsertionFlags::INSERT_NO_FLAGS, WebStateOpener());
+    list->InsertWebState(
+        /*index=*/1, web::WebState::Create(create_params),
+        WebStateList::InsertionFlags::INSERT_NO_FLAGS, WebStateOpener());
+  }));
+
+  std::list<std::string> events = breadcrumb_service_->GetEvents(0);
+  ASSERT_EQ(1ul, events.size());
+  EXPECT_NE(std::string::npos, events.front().find("Inserted 2 tabs"))
+      << events.front();
+
+  // Close multiple WebStates.
+  web_state_list.PerformBatchOperation(base::BindOnce(^(WebStateList* list) {
+    list->CloseWebStateAt(
+        /*index=*/0, WebStateList::ClosingFlags::CLOSE_NO_FLAGS);
+    list->CloseWebStateAt(
+        /*index=*/0, WebStateList::ClosingFlags::CLOSE_NO_FLAGS);
+  }));
+
+  events = breadcrumb_service_->GetEvents(0);
+  ASSERT_EQ(2ul, events.size());
+  EXPECT_NE(std::string::npos, events.back().find("Closed 2 tabs"))
+      << events.back();
+}