[Mac] Reduce jank in the cookie manager by batching updates from the model.

Subclass TreeModelObserver specifically for the CookiesTreeModel so that we can
send TreeModelBatchBegin() and BatchEnd() notifications before and after local
storage, databases, and appcache entries load, respectively.

This also rewrites CookiesTreeModelObserverBridge::FindCocoaNode() to do
breadth-first search, rather than a pre-order traversal as we most often search
for origin nodes, which are at depth 1.

BUG=35134
TEST=Get a profile with 1K local storage entries. Chromium-->Preferences-->Under the Hood-->Content Settings-->Cookies should not be slow/janky.

Review URL: http://codereview.chromium.org/660251

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@40444 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 136 insertions, 24 deletions

diff --git a/chrome/browser/cocoa/cookies_window_controller.h b/chrome/browser/cocoa/cookies_window_controller.h
index e0d31f3..8f85285 100644
--- a/chrome/browser/cocoa/cookies_window_controller.h
+++ b/chrome/browser/cocoa/cookies_window_controller.h
@@ -21,7 +21,7 @@ class CookiesWindowControllerTest;
 
 // Thin bridge to the window controller that performs model update actions
 // directly on the treeController_.
-class CookiesTreeModelObserverBridge : public TreeModelObserver {
+class CookiesTreeModelObserverBridge : public CookiesTreeModel::Observer {
  public:
   explicit CookiesTreeModelObserverBridge(CookiesWindowController* controller);
 
@@ -45,6 +45,9 @@ class CookiesTreeModelObserverBridge : public TreeModelObserver {
   // Notification that the contents of a node has changed.
   virtual void TreeNodeChanged(TreeModel* model, TreeModelNode* node);
 
+  virtual void TreeModelBeginBatch(CookiesTreeModel* model);
+  virtual void TreeModelEndBatch(CookiesTreeModel* model);
+
   // Invalidates the Cocoa model. This is used to tear down the Cocoa model
   // when we're about to entirely rebuild it.
   void InvalidateCocoaModel();
@@ -66,6 +69,11 @@ class CookiesTreeModelObserverBridge : public TreeModelObserver {
   bool HasCocoaModel();
 
   CookiesWindowController* window_controller_;  // weak, owns us.
+
+  // If this is true, then the Model has informed us that it is batching
+  // updates. Rather than updating the Cocoa side of the model, we ignore those
+  // small changes and rebuild once at the end.
+  bool batch_update_;
 };
 
 // Controller for the cookies manager. This class stores an internal copy of
diff --git a/chrome/browser/cocoa/cookies_window_controller.mm b/chrome/browser/cocoa/cookies_window_controller.mm
index b9fbc59..e2f53c5 100644
--- a/chrome/browser/cocoa/cookies_window_controller.mm
+++ b/chrome/browser/cocoa/cookies_window_controller.mm
@@ -4,6 +4,7 @@
 
 #import "chrome/browser/cocoa/cookies_window_controller.h"
 
+#include <queue>
 #include <vector>
 
 #include "app/l10n_util_mac.h"
@@ -25,7 +26,8 @@ static NSString* const kCocoaTreeModel = @"cocoaTreeModel";
 
 CookiesTreeModelObserverBridge::CookiesTreeModelObserverBridge(
     CookiesWindowController* controller)
-    : window_controller_(controller) {
+    : window_controller_(controller),
+      batch_update_(false) {
 }
 
 // Notification that nodes were added to the specified parent.
@@ -34,7 +36,7 @@ void CookiesTreeModelObserverBridge::TreeNodesAdded(TreeModel* model,
                                                     int start,
                                                     int count) {
   // We're in for a major rebuild. Ignore this request.
-  if (!HasCocoaModel())
+  if (batch_update_ || !HasCocoaModel())
     return;
 
   CocoaCookieTreeNode* cocoa_parent = FindCocoaNode(parent, nil);
@@ -56,7 +58,7 @@ void CookiesTreeModelObserverBridge::TreeNodesRemoved(TreeModel* model,
                                                       int start,
                                                       int count) {
   // We're in for a major rebuild. Ignore this request.
-  if (!HasCocoaModel())
+  if (batch_update_ || !HasCocoaModel())
     return;
 
   CocoaCookieTreeNode* cocoa_parent = FindCocoaNode(parent, nil);
@@ -73,7 +75,7 @@ void CookiesTreeModelObserverBridge::TreeNodesRemoved(TreeModel* model,
 void CookiesTreeModelObserverBridge::TreeNodeChildrenReordered(TreeModel* model,
     TreeModelNode* parent) {
   // We're in for a major rebuild. Ignore this request.
-  if (!HasCocoaModel())
+  if (batch_update_ || !HasCocoaModel())
     return;
 
   CocoaCookieTreeNode* cocoa_parent = FindCocoaNode(parent, nil);
@@ -101,7 +103,7 @@ void CookiesTreeModelObserverBridge::TreeNodeChildrenReordered(TreeModel* model,
 void CookiesTreeModelObserverBridge::TreeNodeChanged(TreeModel* model,
                                                      TreeModelNode* node) {
   // If we don't have a Cocoa model, only let the root node change.
-  if (!HasCocoaModel() && model->GetRoot() != node)
+  if (batch_update_ || (!HasCocoaModel() && model->GetRoot() != node))
     return;
 
   if (HasCocoaModel()) {
@@ -116,6 +118,19 @@ void CookiesTreeModelObserverBridge::TreeNodeChanged(TreeModel* model,
   }
 }
 
+void CookiesTreeModelObserverBridge::TreeModelBeginBatch(
+    CookiesTreeModel* model) {
+  batch_update_ = true;
+}
+
+void CookiesTreeModelObserverBridge::TreeModelEndBatch(
+    CookiesTreeModel* model) {
+  DCHECK(batch_update_);
+  CocoaCookieTreeNode* root = CocoaNodeFromTreeNode(model->GetRoot());
+  [window_controller_ setCocoaTreeModel:root];
+  batch_update_ = false;
+}
+
 void CookiesTreeModelObserverBridge::InvalidateCocoaModel() {
   [[[window_controller_ cocoaTreeModel] mutableChildren] removeAllObjects];
 }
@@ -126,27 +141,41 @@ CocoaCookieTreeNode* CookiesTreeModelObserverBridge::CocoaNodeFromTreeNode(
   return [[[CocoaCookieTreeNode alloc] initWithNode:cookie_node] autorelease];
 }
 
-// Does a pre-order traversal on the tree to find |node|.
+// Does breadth-first search on the tree to find |node|. This method is most
+// commonly used to find origin/folder nodes, which are at the first level off
+// the root (hence breadth-first search).
 CocoaCookieTreeNode* CookiesTreeModelObserverBridge::FindCocoaNode(
-    TreeModelNode* node, CocoaCookieTreeNode* start) {
+    TreeModelNode* target, CocoaCookieTreeNode* start) {
   if (!start) {
     start = [window_controller_ cocoaTreeModel];
   }
-  if ([start treeNode] == node) {
+  if ([start treeNode] == target) {
     return start;
   }
 
-  NSArray* children = [start children];
-  for (CocoaCookieTreeNode* child in children) {
-    if ([child treeNode] == node) {
-      return child;
+  // Enqueue the root node of the search (sub-)tree.
+  std::queue<CocoaCookieTreeNode*> horizon;
+  horizon.push(start);
+
+  // Loop until we've looked at every node or we found the target.
+  while (!horizon.empty()) {
+    // Dequeue the item at the front.
+    CocoaCookieTreeNode* node = horizon.front();
+    horizon.pop();
+
+    // If this is the droid we're looking for, report it.
+    if ([node treeNode] == target)
+      return node;
+
+    // "Move along, move along." by adding all child nodes to the queue.
+    if (![node isLeaf]) {
+      NSArray* children = [node children];
+      for (CocoaCookieTreeNode* child in children) {
+        horizon.push(child);
+      }
     }
-
-    // Search the children. Return the result if we find one.
-    CocoaCookieTreeNode* recurse = FindCocoaNode(node, child);
-    if (recurse)
-      return recurse;
   }
+
   return nil;  // We couldn't find the node.
 }
 
diff --git a/chrome/browser/cocoa/cookies_window_controller_unittest.mm b/chrome/browser/cocoa/cookies_window_controller_unittest.mm
index 69757cf..b58c584 100644
--- a/chrome/browser/cocoa/cookies_window_controller_unittest.mm
+++ b/chrome/browser/cocoa/cookies_window_controller_unittest.mm
@@ -345,7 +345,7 @@ TEST_F(CookiesWindowControllerTest, TreeNodeChanged) {
   EXPECT_TRUE([@"Silly Change" isEqualToString:[cocoa_node title]]);
 }
 
-TEST_F(CookiesWindowControllerTest, TestDeleteCookie) {
+TEST_F(CookiesWindowControllerTest, DeleteCookie) {
   const GURL url = GURL("http://foo.com");
   TestingProfile* profile = browser_helper_.profile();
   net::CookieMonster* cm = profile->GetCookieMonster();
@@ -379,7 +379,7 @@ TEST_F(CookiesWindowControllerTest, TestDeleteCookie) {
   [controller closeSheet:nil];
 }
 
-TEST_F(CookiesWindowControllerTest, TestDidExpandItem) {
+TEST_F(CookiesWindowControllerTest, DidExpandItem) {
   const GURL url = GURL("http://foo.com");
   TestingProfile* profile = browser_helper_.profile();
   net::CookieMonster* cm = profile->GetCookieMonster();
diff --git a/chrome/browser/cookies_tree_model.cc b/chrome/browser/cookies_tree_model.cc
index 8e3f611..d884b11 100644
--- a/chrome/browser/cookies_tree_model.cc
+++ b/chrome/browser/cookies_tree_model.cc
@@ -301,7 +301,8 @@ CookiesTreeModel::CookiesTreeModel(
       profile_(profile),
       appcache_helper_(appcache_helper),
       database_helper_(database_helper),
-      local_storage_helper_(local_storage_helper) {
+      local_storage_helper_(local_storage_helper),
+      batch_update_(0) {
   LoadCookies();
   DCHECK(database_helper_);
   database_helper_->StartFetching(NewCallback(
@@ -395,12 +396,14 @@ void CookiesTreeModel::LoadCookiesWithFilter(const std::wstring& filter) {
 }
 
 void CookiesTreeModel::DeleteAllStoredObjects() {
+  NotifyObserverBeginBatch();
   CookieTreeNode* root = GetRoot();
   root->DeleteStoredObjects();
   int num_children = root->GetChildCount();
   for (int i = num_children - 1; i >= 0; --i)
     delete Remove(root, i);
   NotifyObserverTreeNodeChanged(root);
+  NotifyObserverEndBatch();
 }
 
 void CookiesTreeModel::DeleteCookieNode(CookieTreeNode* cookie_node) {
@@ -414,6 +417,7 @@ void CookiesTreeModel::DeleteCookieNode(CookieTreeNode* cookie_node) {
 void CookiesTreeModel::UpdateSearchResults(const std::wstring& filter) {
   CookieTreeNode* root = GetRoot();
   int num_children = root->GetChildCount();
+  NotifyObserverBeginBatch();
   for (int i = num_children - 1; i >= 0; --i)
     delete Remove(root, i);
   LoadCookiesWithFilter(filter);
@@ -421,6 +425,19 @@ void CookiesTreeModel::UpdateSearchResults(const std::wstring& filter) {
   PopulateLocalStorageInfoWithFilter(filter);
   PopulateAppCacheInfoWithFilter(filter);
   NotifyObserverTreeNodeChanged(root);
+  NotifyObserverEndBatch();
+}
+
+void CookiesTreeModel::AddObserver(Observer* observer) {
+  cookies_observer_list_.AddObserver(observer);
+  // Call super so that TreeNodeModel can notify, too.
+  TreeNodeModel<CookieTreeNode>::AddObserver(observer);
+}
+
+void CookiesTreeModel::RemoveObserver(Observer* observer) {
+  cookies_observer_list_.RemoveObserver(observer);
+  // Call super so that TreeNodeModel doesn't have dead pointers.
+  TreeNodeModel<CookieTreeNode>::RemoveObserver(observer);
 }
 
 void CookiesTreeModel::OnAppCacheModelInfoLoaded() {
@@ -432,6 +449,7 @@ void CookiesTreeModel::PopulateAppCacheInfoWithFilter(
   if (!appcache_helper_ || appcache_helper_->info_list().empty())
     return;
   CookieTreeRootNode* root = static_cast<CookieTreeRootNode*>(GetRoot());
+  NotifyObserverBeginBatch();
   for (AppCacheInfoList::const_iterator info =
            appcache_helper_->info_list().begin();
        info != appcache_helper_->info_list().end(); ++info) {
@@ -447,6 +465,7 @@ void CookiesTreeModel::PopulateAppCacheInfoWithFilter(
     }
   }
   NotifyObserverTreeNodeChanged(root);
+  NotifyObserverEndBatch();
 }
 
 void CookiesTreeModel::OnDatabaseModelInfoLoaded(
@@ -460,6 +479,7 @@ void CookiesTreeModel::PopulateDatabaseInfoWithFilter(
   if (database_info_list_.empty())
     return;
   CookieTreeRootNode* root = static_cast<CookieTreeRootNode*>(GetRoot());
+  NotifyObserverBeginBatch();
   for (DatabaseInfoList::iterator database_info = database_info_list_.begin();
        database_info != database_info_list_.end();
        ++database_info) {
@@ -483,6 +503,7 @@ void CookiesTreeModel::PopulateDatabaseInfoWithFilter(
     }
   }
   NotifyObserverTreeNodeChanged(root);
+  NotifyObserverEndBatch();
 }
 
 void CookiesTreeModel::OnStorageModelInfoLoaded(
@@ -496,6 +517,7 @@ void CookiesTreeModel::PopulateLocalStorageInfoWithFilter(
   if (local_storage_info_list_.empty())
     return;
   CookieTreeRootNode* root = static_cast<CookieTreeRootNode*>(GetRoot());
+  NotifyObserverBeginBatch();
   for (LocalStorageInfoList::iterator local_storage_info =
        local_storage_info_list_.begin();
        local_storage_info != local_storage_info_list_.end();
@@ -520,6 +542,26 @@ void CookiesTreeModel::PopulateLocalStorageInfoWithFilter(
     }
   }
   NotifyObserverTreeNodeChanged(root);
+  NotifyObserverEndBatch();
+}
+
+void CookiesTreeModel::NotifyObserverBeginBatch() {
+  // Only notify the model once if we're batching in a nested manner.
+  if (batch_update_++ == 0) {
+    FOR_EACH_OBSERVER(Observer,
+                      cookies_observer_list_,
+                      TreeModelBeginBatch(this));
+  }
+}
+
+void CookiesTreeModel::NotifyObserverEndBatch() {
+  // Only notify the observers if this is the outermost call to EndBatch() if
+  // called in a nested manner.
+  if (--batch_update_ == 0) {
+    FOR_EACH_OBSERVER(Observer,
+                      cookies_observer_list_,
+                      TreeModelEndBatch(this));
+  }
 }
 
 std::wstring CookiesTreeModel::FormExtensionNodeName(
diff --git a/chrome/browser/cookies_tree_model.h b/chrome/browser/cookies_tree_model.h
index 3fabbaf..056fe5f 100644
--- a/chrome/browser/cookies_tree_model.h
+++ b/chrome/browser/cookies_tree_model.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "app/tree_node_model.h"
+#include "base/observer_list.h"
 #include "base/scoped_ptr.h"
 #include "chrome/browser/browsing_data_appcache_helper.h"
 #include "chrome/browser/browsing_data_database_helper.h"
@@ -344,6 +345,17 @@ class CookieTreeLocalStoragesNode : public CookieTreeNode {
 
 class CookiesTreeModel : public TreeNodeModel<CookieTreeNode> {
  public:
+  // Because non-cookie nodes are fetched in a background thread, they are not
+  // present at the time the Model is created. The Model then notifies its
+  // observers for every item added from databases, local storage, and
+  // appcache. We extend the Observer interface to add notifications before and
+  // after these batch inserts.
+  class Observer : public TreeModelObserver {
+   public:
+    virtual void TreeModelBeginBatch(CookiesTreeModel* model) {}
+    virtual void TreeModelEndBatch(CookiesTreeModel* model) {}
+  };
+
   CookiesTreeModel(
       Profile* profile,
       BrowsingDataDatabaseHelper* database_helper,
@@ -368,6 +380,16 @@ class CookiesTreeModel : public TreeNodeModel<CookieTreeNode> {
   // Filter the origins to only display matched results.
   void UpdateSearchResults(const std::wstring& filter);
 
+  // Overload the Add/Remove observer methods so we can notify about
+  // CookiesTreeModel-specific things. Note that this is NOT overriding the
+  // method by the same name in TreeNodeModel because the argument type is
+  // different. Therefore, if this AddObserver(TreeModelObserver*) is called,
+  // the observer will NOT be notified about batching. This is also why we
+  // maintain a separate list of observers that are specifically Observer*
+  // objects.
+  virtual void AddObserver(Observer* observer);
+  virtual void RemoveObserver(Observer* observer);
+
  private:
   enum CookieIconIndex {
     ORIGIN = 0,
@@ -396,6 +418,9 @@ class CookiesTreeModel : public TreeNodeModel<CookieTreeNode> {
 
   std::wstring FormExtensionNodeName(const std::string& extension_id);
 
+  void NotifyObserverBeginBatch();
+  void NotifyObserverEndBatch();
+
   // The profile from which this model sources cookies.
   Profile* profile_;
   CookieList all_cookies_;
@@ -407,6 +432,15 @@ class CookiesTreeModel : public TreeNodeModel<CookieTreeNode> {
   scoped_refptr<BrowsingDataLocalStorageHelper> local_storage_helper_;
   LocalStorageInfoList local_storage_info_list_;
 
+  // The CookiesTreeModel maintains a separate list of observers that are
+  // specifically of the type CookiesTreeModel::Observer.
+  ObserverList<Observer> cookies_observer_list_;
+
+  // If this is non-zero, then this model is batching updates (there's a lot of
+  // notifications coming down the pipe). This is an integer is used to balance
+  // calls to Begin/EndBatch() if they're called in a nested manner.
+  int batch_update_;
+
   friend class CookieTreeAppCacheNode;
   friend class CookieTreeCookieNode;
   friend class CookieTreeDatabaseNode;
diff --git a/chrome/browser/views/options/cookies_view.cc b/chrome/browser/views/options/cookies_view.cc
index 584cf85..2271536 100644
--- a/chrome/browser/views/options/cookies_view.cc
+++ b/chrome/browser/views/options/cookies_view.cc
@@ -12,7 +12,6 @@
 #include "base/i18n/time_formatting.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
-#include "chrome/browser/cookies_tree_model.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/views/appcache_info_view.h"
 #include "chrome/browser/views/cookie_info_view.h"
diff --git a/chrome/browser/views/options/cookies_view.h b/chrome/browser/views/options/cookies_view.h
index ee2c314..64ca05d 100644
--- a/chrome/browser/views/options/cookies_view.h
+++ b/chrome/browser/views/options/cookies_view.h
@@ -9,6 +9,7 @@
 
 #include "app/tree_model.h"
 #include "base/task.h"
+#include "chrome/browser/cookies_tree_model.h"
 #include "net/base/cookie_monster.h"
 #include "views/controls/button/button.h"
 #include "views/controls/tree/tree_view.h"
@@ -27,7 +28,6 @@ class NativeButton;
 
 class AppCacheInfoView;
 class CookieInfoView;
-class CookiesTreeModel;
 class CookiesTreeView;
 class DatabaseInfoView;
 class LocalStorageInfoView;
@@ -35,7 +35,7 @@ class Profile;
 class Timer;
 
 
-class CookiesView : public TreeModelObserver,
+class CookiesView : public CookiesTreeModel::Observer,
                     public views::View,
                     public views::DialogDelegate,
                     public views::ButtonListener,