Refactoring CookiesTreeModel to support multiple data sources.

chrome/browser/cookies_tree_model.h

Index: chrome/browser/cookies_tree_model.h
diff --git a/chrome/browser/cookies_tree_model.h b/chrome/browser/cookies_tree_model.h
index e99fa7f5188930d2aab740e58e49744c74e18ac8..7a21a4ab37f7c72127786f33728afde5ffda70f8 100644
--- a/chrome/browser/cookies_tree_model.h
+++ b/chrome/browser/cookies_tree_model.h
@@ -51,6 +51,27 @@ class CookieTreeSessionStoragesNode;
 class CookieTreeIndexedDBNode;
 class CookieTreeIndexedDBsNode;
 class CookieTreeOriginNode;
+class CookieTreeAppNode;

[markusheintz_, 2012/06/13 23:40:39]

nit: It looks like this list of forward declarations used to be alphabetically
sorted. Maybe you could add the new lines at the right positions. If you could
even restore the order that would be awsome. But at least move line 54 to line
38.

[nasko, 2012/06/14 18:46:42]

Done.

+class LocalDataContainer;
+
+// Friendly typedefs for the multiple types of lists used in the model.
+namespace {
+  typedef std::map<string16, LocalDataContainer*> ContainerMap;
+  typedef std::list<net::CookieMonster::CanonicalCookie> CookieList;

[markusheintz_, 2012/06/13 23:40:39]

This typedef is not necessary. The cookie monster has already a CookieList. See
net/cookies/cookie_monster.h:966
It's net::CookieList.

[nasko, 2012/06/14 18:46:42]

Just moved code around. If I change it to the net::CookieList, it doesn't behave
identically, so requires some extra work. I can do it once everything else is
complete, assuming you feel strongly about it.

+  typedef std::list<BrowsingDataDatabaseHelper::DatabaseInfo> DatabaseInfoList;
+  typedef std::list<BrowsingDataLocalStorageHelper::LocalStorageInfo>
+      LocalStorageInfoList;
+  typedef std::list<BrowsingDataLocalStorageHelper::LocalStorageInfo>
+      SessionStorageInfoList;
+  typedef std::list<BrowsingDataIndexedDBHelper::IndexedDBInfo>
+      IndexedDBInfoList;
+  typedef std::list<BrowsingDataFileSystemHelper::FileSystemInfo>
+      FileSystemInfoList;
+  typedef std::list<BrowsingDataQuotaHelper::QuotaInfo> QuotaInfoArray;

[markusheintz_, 2012/06/13 23:40:39]

nit: s/QuotaInfoArray/QuotaInfoList 
1) To be consistent with the other names.
2) It is a list and not an Array.

Or is there a particular reason why you named it Array?

[nasko, 2012/06/14 18:46:42]

All of these were just existing code being moved around in the file. I'm happy
to fix it though : ).

+  typedef net::ServerBoundCertStore::ServerBoundCertList ServerBoundCertList;
+
+  typedef std::map<GURL, std::list<appcache::AppCacheInfo> > AppCacheInfoMap;
+}
 
 // CookieTreeNode -------------------------------------------------------------
 // The base node type in the Cookies, Databases, and Local Storage options
@@ -66,6 +87,7 @@ class CookieTreeNode : public ui::TreeNode<CookieTreeNode> {
     enum NodeType {
       TYPE_NONE,
       TYPE_ROOT,  // This is used for CookieTreeRootNode nodes.
+      TYPE_APP, // This is used for CookieTreeAppNode nodes.

[markusheintz_, 2012/06/13 23:40:39]

nit: Add one more space before the comment in this line.

[nasko, 2012/06/14 18:46:42]

Done.

       TYPE_ORIGIN,  // This is used for CookieTreeOriginNode nodes.
       TYPE_COOKIES,  // This is used for CookieTreeCookiesNode nodes.
       TYPE_COOKIE,  // This is used for CookieTreeCookieNode nodes.
@@ -107,6 +129,13 @@ class CookieTreeNode : public ui::TreeNode<CookieTreeNode> {
       return *this;
     }
 
+    DetailedInfo& InitApp(const string16& app_name, const string16& app_id) {
+      Init(TYPE_APP);
+      this->app_name = app_name;
+      this->app_id = app_id;
+      return *this;
+    }
+
     DetailedInfo& InitCookie(
         const net::CookieMonster::CanonicalCookie* cookie) {
       Init(TYPE_COOKIE);
@@ -171,6 +200,8 @@ class CookieTreeNode : public ui::TreeNode<CookieTreeNode> {
       return *this;
     }
 
+    string16 app_name;
+    string16 app_id;
     string16 origin;
     NodeType node_type;
     const net::CookieMonster::CanonicalCookie* cookie;
@@ -207,6 +238,11 @@ class CookieTreeNode : public ui::TreeNode<CookieTreeNode> {
     bool operator() (const CookieTreeNode* lhs, const CookieTreeNode* rhs);
   };
 
+  class AppNodeComparator {
+   public:
+    bool operator() (const CookieTreeNode* lhs, const CookieTreeNode* rhs);
+  };
+
   void AddChildSortedByTitle(CookieTreeNode* new_child);
 
  private:
@@ -221,7 +257,8 @@ class CookieTreeRootNode : public CookieTreeNode {
   explicit CookieTreeRootNode(CookiesTreeModel* model);
   virtual ~CookieTreeRootNode();
 
-  CookieTreeOriginNode* GetOrCreateOriginNode(const GURL& url);
+  CookieTreeAppNode* GetOrCreateAppNode(const string16& app_name,
+                                        const string16& app_id);
 
   // CookieTreeNode methods:
   virtual CookiesTreeModel* GetModel() const OVERRIDE;
@@ -233,6 +270,25 @@ class CookieTreeRootNode : public CookieTreeNode {
   DISALLOW_COPY_AND_ASSIGN(CookieTreeRootNode);
 };
 
+// CookieTreeAppNode -------------------------------------------------------

[markusheintz_, 2012/06/13 23:40:39]

nit: Add some more '-' to make this line as long as the other comments.

[nasko, 2012/06/14 18:46:42]

Done.

+class CookieTreeAppNode : public CookieTreeNode {
+ public:
+  explicit CookieTreeAppNode(const string16& app_name, const string16& app_id);
+  virtual ~CookieTreeAppNode();
+
+  CookieTreeOriginNode* GetOrCreateOriginNode(const GURL& url);
+
+  // CookieTreeNode methods:
+  virtual DetailedInfo GetDetailedInfo() const OVERRIDE;
+
+  const string16 app_id() const { return app_id_; }

[markusheintz_, 2012/06/13 23:40:39]

I think you can return a const reference here: const string16&

[nasko, 2012/06/14 18:46:42]

Done.

+
+ private:
+  string16 app_name_;
+  string16 app_id_;

[markusheintz_, 2012/06/13 23:40:39]

nit: Please add an empty line here.

[nasko, 2012/06/14 18:46:42]

Done.

+  DISALLOW_COPY_AND_ASSIGN(CookieTreeAppNode);
+};
+
 // CookieTreeOriginNode -------------------------------------------------------
 class CookieTreeOriginNode : public CookieTreeNode {
  public:
@@ -614,9 +670,35 @@ class CookieTreeServerBoundCertsNode : public CookieTreeNode {
   DISALLOW_COPY_AND_ASSIGN(CookieTreeServerBoundCertsNode);
 };
 
+class CookiesTreeModelDelegate {

[markusheintz_, 2012/06/13 23:40:39]

Please document this class (interface). Thanks.

[nasko, 2012/06/14 18:46:42]

Done.

+ public:

[markusheintz_, 2012/06/13 23:40:39]

All public methods should be documented. Please document the method below.

[nasko, 2012/06/14 18:46:42]

As discussed, I'm just moving those around and they seem pretty straighforward
methods, so skipping documentation for now.

+  virtual void PopulateAppCacheInfoWithFilter(const string16* app,

[markusheintz_, 2012/06/13 23:40:39]

Is |app| the app name of the app id? maybe you could change the name
accordingly.

Is there a particular reason why you use const pointers here?
Please use const refs (const string16& app) if possible. I think the rule is
that we don't have const pointers in the parameter list but const refs.

same below.

[nasko, 2012/06/14 18:46:42]

Yes, there is a reason for the pointer. I want to distinguish between the app id
of the "browser process" which is the empty string and a case where we don't
have an app id.

[markusheintz_, 2012/06/19 20:01:33]

Just to refresh my memory when is it again possible to have no app_id?

[nasko, 2012/06/19 22:22:40]

In the case where we refresh the tree model from WebUI, in which case we want to
build the full tree, not just a single app.

+                                              const std::wstring& filter) {}

[markusheintz_, 2012/06/13 23:40:39]

Here and below: Please don't use std::wstring. Replace std::wstring with
string16. Everywhere.

You can use the base/utf_string_conversions.h to convert from a wide string to
string16 if really necessary. 

I think there is a presubmit check that check if you introduce new std::wstrings

[nasko, 2012/06/14 18:46:42]

I'll move those over to string16.

+  virtual void PopulateCookieInfoWithFilter(const string16* app,
+                                            const std::wstring& filter) {}
+  virtual void PopulateDatabaseInfoWithFilter(const string16* app,
+                                              const std::wstring& filter) {}

[markusheintz_, 2012/06/13 23:40:39]

Please don't use std::wstring. wstring is replaced with string16.

[nasko, 2012/06/14 18:46:42]

See the above.

+  virtual void PopulateLocalStorageInfoWithFilter(const string16* app,
+                                                  const std::wstring& filter) {}
+  virtual void PopulateSessionStorageInfoWithFilter(
+      const string16* app, const std::wstring& filter) {}
+  virtual void PopulateIndexedDBInfoWithFilter(const string16* app,
+                                               const std::wstring& filter) {}
+  virtual void PopulateFileSystemInfoWithFilter(const string16* app,
+                                                const std::wstring& filter) {}
+  virtual void PopulateQuotaInfoWithFilter(const string16* app,
+                                           const std::wstring& filter) {}
+  virtual void PopulateServerBoundCertInfoWithFilter(
+      const string16* app, const std::wstring& filter) {}
+};
+
 // CookiesTreeModel -----------------------------------------------------------
-class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode> {
+class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode>,
+                         public CookiesTreeModelDelegate {
  public:
+  CookiesTreeModel(ContainerMap& apps_map, bool use_cookie_source);
+  virtual ~CookiesTreeModel();
+
   // 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
@@ -628,19 +710,6 @@ class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode> {
     virtual void TreeModelEndBatch(CookiesTreeModel* model) {}
   };
 
-  CookiesTreeModel(
-      BrowsingDataCookieHelper* cookie_helper,
-      BrowsingDataDatabaseHelper* database_helper,
-      BrowsingDataLocalStorageHelper* local_storage_helper,
-      BrowsingDataLocalStorageHelper* session_storage_helper,
-      BrowsingDataAppCacheHelper* appcache_helper,
-      BrowsingDataIndexedDBHelper* indexed_db_helper,
-      BrowsingDataFileSystemHelper* file_system_helper,
-      BrowsingDataQuotaHelper* quota_helper,
-      BrowsingDataServerBoundCertHelper* server_bound_cert_helper,
-      bool use_cookie_source);
-  virtual ~CookiesTreeModel();
-
   // ui::TreeModel methods:
   // Returns the set of icons for the nodes in the tree. You only need override
   // this if you don't want to use the default folder icons.
@@ -653,6 +722,7 @@ class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode> {
 
   // CookiesTreeModel methods:
   void DeleteAllStoredObjects();
+  void DeleteStoredObjectsForApp(const string16& app);

[markusheintz_, 2012/06/13 23:40:39]

Please document the method and replace app with app_id.

I know there is not much documentation in this class, but we can improve this
going forward.

[nasko, 2012/06/14 18:46:42]

Done.

   void DeleteCookieNode(CookieTreeNode* cookie_node);
 
   // Filter the origins to only display matched results.
@@ -665,26 +735,79 @@ class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode> {
   virtual void AddCookiesTreeObserver(Observer* observer);
   virtual void RemoveCookiesTreeObserver(Observer* observer);
 

[markusheintz_, 2012/06/13 23:40:39]

nit: Add "// CookiesTreeModelDelegate methods:"

[nasko, 2012/06/14 18:46:42]

Done.

+  virtual void PopulateAppCacheInfoWithFilter(

[markusheintz_, 2012/06/13 23:40:39]

In all method signatures:
s/app/app_id

[nasko, 2012/06/14 18:46:42]

Done.

+      const string16* app, const std::wstring& filter) OVERRIDE;
+  virtual void PopulateCookieInfoWithFilter(
+      const string16* app, const std::wstring& filter) OVERRIDE;
+  virtual void PopulateDatabaseInfoWithFilter(
+      const string16* app, const std::wstring& filter) OVERRIDE;
+  virtual void PopulateLocalStorageInfoWithFilter(
+      const string16* app, const std::wstring& filter) OVERRIDE;
+  virtual void PopulateSessionStorageInfoWithFilter(
+      const string16* app, const std::wstring& filter) OVERRIDE;
+  virtual void PopulateIndexedDBInfoWithFilter(
+      const string16* app, const std::wstring& filter) OVERRIDE;
+  virtual void PopulateFileSystemInfoWithFilter(
+      const string16* app, const std::wstring& filter) OVERRIDE;
+  virtual void PopulateQuotaInfoWithFilter(
+      const string16* app, const std::wstring& filter) OVERRIDE;
+  virtual void PopulateServerBoundCertInfoWithFilter(
+      const string16* app, const std::wstring& filter) OVERRIDE;
+
+  BrowsingDataCookieHelper* GetCookieHelper(const string16& app_id);
+  LocalDataContainer* GetLocalDataContainer(const string16& app_id);
+
  private:
   enum CookieIconIndex {
     ORIGIN = 0,
     COOKIE = 1,
     DATABASE = 2
   };
-  typedef std::list<net::CookieMonster::CanonicalCookie> CookieList;
-  typedef std::list<BrowsingDataDatabaseHelper::DatabaseInfo>
-      DatabaseInfoList;
-  typedef std::list<BrowsingDataLocalStorageHelper::LocalStorageInfo>
-      LocalStorageInfoList;
-  typedef std::list<BrowsingDataLocalStorageHelper::LocalStorageInfo>
-      SessionStorageInfoList;
-  typedef std::list<BrowsingDataIndexedDBHelper::IndexedDBInfo>
-      IndexedDBInfoList;
-  typedef std::list<BrowsingDataFileSystemHelper::FileSystemInfo>
-      FileSystemInfoList;
-  typedef std::list<BrowsingDataQuotaHelper::QuotaInfo> QuotaInfoArray;
-  typedef net::ServerBoundCertStore::ServerBoundCertList ServerBoundCertList;
 
+  void NotifyObserverBeginBatch();
+  void NotifyObserverEndBatch();
+
+  // Map of app ids to LocalDataContainer objects to use when retrieving
+  // locally stored data.
+  ContainerMap app_data_map_;
+
+  // The CookiesTreeModel maintains a separate list of observers that are
+  // specifically of the type CookiesTreeModel::Observer.
+  ObserverList<Observer> cookies_observer_list_;
+
+  // If true, use the CanonicalCookie::Source attribute to group cookies.
+  // Otherwise, use the CanonicalCookie::Domain attribute.
+  bool use_cookie_source_;
+
+  // 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_;
+};
+
+// LocalDataContainer ---------------------------------------------------------
+class LocalDataContainer {

[markusheintz_, 2012/06/13 23:40:39]

Please document the class by adding a comment.

[nasko, 2012/06/14 18:46:42]

Done.

+ public:
+  LocalDataContainer(
+      const string16& app_name,
+      const string16& app_id,
+      BrowsingDataCookieHelper* cookie_helper,
+      BrowsingDataDatabaseHelper* database_helper,
+      BrowsingDataLocalStorageHelper* local_storage_helper,
+      BrowsingDataLocalStorageHelper* session_storage_helper,
+      BrowsingDataAppCacheHelper* appcache_helper,
+      BrowsingDataIndexedDBHelper* indexed_db_helper,
+      BrowsingDataFileSystemHelper* file_system_helper,
+      BrowsingDataQuotaHelper* quota_helper,
+      BrowsingDataServerBoundCertHelper* server_bound_cert_helper);
+  virtual ~LocalDataContainer();
+
+  void Init(CookiesTreeModelDelegate* d);
+
+  string16 app_name() { return app_name_; }
+  string16 app_id() { return app_id_; }
+
+ private:
   void OnAppCacheModelInfoLoaded();
   void OnCookiesModelInfoLoaded(const net::CookieList& cookie_list);
   void OnDatabaseModelInfoLoaded(const DatabaseInfoList& database_info);
@@ -699,18 +822,8 @@ class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode> {
   void OnQuotaModelInfoLoaded(const QuotaInfoArray& quota_info);
   void OnServerBoundCertModelInfoLoaded(const ServerBoundCertList& cert_list);
 
-  void PopulateAppCacheInfoWithFilter(const std::wstring& filter);
-  void PopulateCookieInfoWithFilter(const std::wstring& filter);
-  void PopulateDatabaseInfoWithFilter(const std::wstring& filter);
-  void PopulateLocalStorageInfoWithFilter(const std::wstring& filter);
-  void PopulateSessionStorageInfoWithFilter(const std::wstring& filter);
-  void PopulateIndexedDBInfoWithFilter(const std::wstring& filter);
-  void PopulateFileSystemInfoWithFilter(const std::wstring& filter);
-  void PopulateQuotaInfoWithFilter(const std::wstring& filter);
-  void PopulateServerBoundCertInfoWithFilter(const std::wstring& filter);
-
-  void NotifyObserverBeginBatch();
-  void NotifyObserverEndBatch();
+  string16 app_name_;
+  string16 app_id_;
 
   scoped_refptr<BrowsingDataAppCacheHelper> appcache_helper_;
   scoped_refptr<BrowsingDataCookieHelper> cookie_helper_;
@@ -722,7 +835,7 @@ class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode> {
   scoped_refptr<BrowsingDataQuotaHelper> quota_helper_;
   scoped_refptr<BrowsingDataServerBoundCertHelper> server_bound_cert_helper_;
 
-  std::map<GURL, std::list<appcache::AppCacheInfo> > appcache_info_;
+  AppCacheInfoMap appcache_info_;
   CookieList cookie_list_;
   DatabaseInfoList database_info_list_;
   LocalStorageInfoList local_storage_info_list_;
@@ -732,21 +845,13 @@ class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode> {
   QuotaInfoArray quota_info_list_;
   ServerBoundCertList server_bound_cert_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_;
-
-  // If true, use the CanonicalCookie::Source attribute to group cookies.
-  // Otherwise, use the CanonicalCookie::Domain attribute.
-  bool use_cookie_source_;
+  // A delegate, which must outlive this object. The update callbacks use the
+  // delegate to deliver the updated data to the CookieTreeModel.
+  CookiesTreeModelDelegate* delegate_;
 
-  base::WeakPtrFactory<CookiesTreeModel> weak_ptr_factory_;
+  base::WeakPtrFactory<LocalDataContainer> weak_ptr_factory_;
 
+  friend class CookiesTreeModel;
   friend class CookieTreeAppCacheNode;
   friend class CookieTreeCookieNode;
   friend class CookieTreeDatabaseNode;
@@ -757,7 +862,7 @@ class CookiesTreeModel : public ui::TreeNodeModel<CookieTreeNode> {
   friend class CookieTreeQuotaNode;
   friend class CookieTreeServerBoundCertNode;
 
-  DISALLOW_COPY_AND_ASSIGN(CookiesTreeModel);
+  DISALLOW_COPY_AND_ASSIGN(LocalDataContainer);
 };
 
 #endif  // CHROME_BROWSER_COOKIES_TREE_MODEL_H_