[Sync] Add logic to reassociate tab nodes after restart.

chrome/browser/sync/glue/tab_node_pool.h

Index: chrome/browser/sync/glue/tab_node_pool.h
diff --git a/chrome/browser/sync/glue/tab_node_pool.h b/chrome/browser/sync/glue/tab_node_pool.h
index 8327d061e08551ecf4fb835e39b06b80f1a613f5..d2bce43ab7ac90d08434689216be4b7d9af03b36 100644
--- a/chrome/browser/sync/glue/tab_node_pool.h
+++ b/chrome/browser/sync/glue/tab_node_pool.h
@@ -5,10 +5,13 @@
 #ifndef CHROME_BROWSER_SYNC_GLUE_TAB_NODE_POOL_H_
 #define CHROME_BROWSER_SYNC_GLUE_TAB_NODE_POOL_H_
 
+#include <map>
+#include <set>
 #include <string>
-#include <vector>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
+#include "chrome/browser/sessions/session_id.h"
 
 class ProfileSyncService;
 
@@ -20,20 +23,16 @@ class TabNodePool {
  public:
   explicit TabNodePool(ProfileSyncService* sync_service);
   ~TabNodePool();
-
+  enum InvalidTab {
+    kInvalidTabID = -1
+  };
   // Build a sync tag from tab_node_id.
   static std::string TabIdToTag(const std::string machine_tag,
                                 size_t tab_node_id);
 
-  // Add a previously allocated tab sync node to our pool. Increases the size
-  // of tab_syncid_pool_ by one and marks the new tab node as free.
-  // Note: this should only be called when we discover tab sync nodes from
-  // previous sessions, not for freeing tab nodes we created through
-  // GetFreeTabNode (use FreeTabNode below for that).
-  void AddTabNode(int64 sync_id);
-
   // Returns the sync_id for the next free tab node. If none are available,
-  // creates a new tab node.
+  // creates a new tab node and adds it to free nodes pool. The free node can
+  // then be used to associate with a tab by calling AssociateTabNode.

[Nicolas Zea, 2013/06/19 21:35:33]

Mention explicitly that the node is still considered free after this is called,
and will only be removed from the pool at Associate time.

That said, it seems risky to have repeated calls to this return the same value.
Perhaps we should just go ahead and remove the node from the pool at the time
this is called?

[shashi, 2013/06/20 00:49:32]

Updated the comment. AssociateTabNode does the actual association and requires
that the node to be associated should be free.
If we remove node from the pool, then we may have to put it in syncid_tabid_map_
and make sure we always reassociate the sync node with a tab node and do not end
up with any unused free nodes.

I have also added DCHECKs to guard against potential misuse.

On 2013/06/19 21:35:33, Nicolas Zea wrote:

   // Note: We make use of the following "id's"

[Nicolas Zea, 2013/06/19 21:35:33]

I think at this point this section on what the ids are should be moved to the
comment just above the class declaration (i.e. right above "class TabNodePool
{")

[shashi, 2013/06/20 00:49:32]

Done
On 2013/06/19 21:35:33, Nicolas Zea wrote:

   // - a sync_id: an int64 used in |syncer::InitByIdLookup|
   // - a tab_id: created by session service, unique to this client
@@ -46,47 +45,76 @@ class TabNodePool {
   // by the sync db.
   int64 GetFreeTabNode();
 
-  // Return a tab node to our free pool.
-  // Note: the difference between FreeTabNode and AddTabNode is that
-  // FreeTabNode does not modify the size of |tab_syncid_pool_|, while
-  // AddTabNode increases it by one. In the case of FreeTabNode, the size of
-  // the |tab_syncid_pool_| should always be equal to the amount of tab nodes
-  // associated with this machine.
+  // Removes the node from |tab_syncid_tab_id_map_| and returns the node to free
+  // pool.
   void FreeTabNode(int64 sync_id);
 
+  // Removes |sync_id| from free node pool and associates it with |tab_id|.
+  // sync_id should be id of a free node. In order to associate a non free
+  // sync node, use ReassociateTabNode.
+  void AssociateTabNode(int64 sync_id, SessionID::id_type tab_id);
+
+  // Associate sync_id with tab_id but does not add it to free node pool.
+  // Note: this should only be called when we discover tab sync nodes from
+  // previous sessions, not for freeing tab nodes we created through
+  // GetFreeTabNode (use FreeTabNode below for that).
+  // The difference between AddTabNode and AssociateTabNode is that
+  // AssociateTabNode requires the sync node to be free to be associated.
+  // AddTabNode just adds the association.
+  void AddTabNode(int64 sync_id, const SessionID& tab_id, size_t tab_node_id);
+
+  // Returns the tab_id for |sync_id| if it exists else returns kInvalidTabID.
+  SessionID::id_type TabIDForSyncID(int64 sync_id) const;

[Nicolas Zea, 2013/06/19 21:35:33]

Rename to GetTabIdFromSyncId

[shashi, 2013/06/20 00:49:32]

Done.

+
+  // Reassociates sync node with id |sync_id| with tab node with |tab_id|.
+  // Returns true if it is able to reassociate the node, false if sync node

[Nicolas Zea, 2013/06/19 21:35:33]

"if a sync node with id |sync_id|"

[shashi, 2013/06/20 00:49:32]

Done.

+  // with |sync_id| is not associated with any tab.
+  bool ReassociateTabNode(int64 sync_id, SessionID::id_type tab_id);
+
+  // Returns any nodes with sync_ids not in |used_synced_ids| to free node pool.
+  void FreeUnusedTabNodes(const std::set<int64>& used_sync_ids);
+
   // Clear tab pool.
   void clear() {

[Nicolas Zea, 2013/06/19 21:35:33]

nit: this was violating style guide before too, but this should now be named
Clear() and have the definition moved into the cc file. Same with the other
methods below (they're not simple getter/setters).

[shashi, 2013/06/20 00:49:32]

Done.

-    tab_syncid_pool_.clear();
-    tab_pool_fp_ = -1;
+    free_nodes_pool_.clear();
+    tab_syncid_tab_id_map_.clear();
+    max_used_tab_node_id_ = 0;
   }
 
   // Return the number of tab nodes this client currently has allocated
   // (including both free and used nodes)
-  size_t capacity() const { return tab_syncid_pool_.size(); }
+  size_t capacity() const {
+    return tab_syncid_tab_id_map_.size() + free_nodes_pool_.size();
+  }
 
   // Return empty status (all tab nodes are in use).
-  bool empty() const { return tab_pool_fp_ == -1; }
+  bool empty() const { return free_nodes_pool_.empty(); }
 
   // Return full status (no tab nodes are in use).
-  bool full() {
-    return tab_pool_fp_ == static_cast<int64>(tab_syncid_pool_.size())-1;
-  }
+  bool full() { return tab_syncid_tab_id_map_.empty(); }
 
   void set_machine_tag(const std::string& machine_tag) {
     machine_tag_ = machine_tag;
   }
 
  private:
-  // Pool of all available syncid's for tab's we have created.
-  std::vector<int64> tab_syncid_pool_;
+  FRIEND_TEST_ALL_PREFIXES(SyncTabNodePoolTest, TabNodeIdIncreases);

[Nicolas Zea, 2013/06/19 21:35:33]

I prefer friending a test class, and then having the test class expose methods
for accessing the protected ones. That way you don't wind up with tons of
individually friended tests.

[shashi, 2013/06/20 00:49:32]

Thanks, done, it is more cleaner now :).
On 2013/06/19 21:35:33, Nicolas Zea wrote:

+  typedef std::map<int64, SessionID::id_type> SyncIDToTabIDMap;
+
+  // Stores mapping of sync_ids associated with tab_ids, these are the used
+  // nodes of tab node pool.
+  // The nodes in the map can be returned to free tab node pool by calling
+  // FreeTabNode(sync_id).
+  SyncIDToTabIDMap tab_syncid_tab_id_map_;

[Nicolas Zea, 2013/06/19 21:35:33]

I think it's implied that sync ids belong to tabs, so perhaps syncid_tabid_map_
is better

[shashi, 2013/06/20 00:49:32]

Done.

+
+  // The set of free nodes in the free node pool.

[Nicolas Zea, 2013/06/19 21:35:33]

"The sync ids for the set of free sync nodes" is a bit clearer I think

+  std::set<int64> free_nodes_pool_;
 
-  // Free pointer for tab pool. Only those node id's, up to and including the
-  // one indexed by the free pointer, are valid and free. The rest of the
-  // |tab_syncid_pool_| is invalid because the nodes are in use.
-  // To get the next free node, use tab_syncid_pool_[tab_pool_fp_--].
-  int64 tab_pool_fp_;
+  // The maximum used tab_node id for a sync node. A new sync node will always
+  // be created with max_used_tab_node_id_ + 1.
+  size_t max_used_tab_node_id_;
 
-  // The machiine tag associated with this tab pool. Used in the title of new
+  // The machine tag associated with this tab pool. Used in the title of new
   // sync nodes.
   std::string machine_tag_;