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

chrome/browser/sync/glue/tab_node_pool.cc

Index: chrome/browser/sync/glue/tab_node_pool.cc
diff --git a/chrome/browser/sync/glue/tab_node_pool.cc b/chrome/browser/sync/glue/tab_node_pool.cc
index 69ed09f83d70f7746a380af96b6c0beb72d27733..194eaeb94b546b8ddab51fa90fd1c89fc71225d2 100644
--- a/chrome/browser/sync/glue/tab_node_pool.cc
+++ b/chrome/browser/sync/glue/tab_node_pool.cc
@@ -23,8 +23,8 @@ static const char kNoSessionsFolderError[] =
 TabNodePool::TabNodePool(
     ProfileSyncService* sync_service)
     : tab_pool_fp_(-1),
-      sync_service_(sync_service) {
-}
+      max_used_tab_node_id_(0),
+      sync_service_(sync_service) {}
 
 TabNodePool::~TabNodePool() {}
 
@@ -35,9 +35,14 @@ std::string TabNodePool::TabIdToTag(
   return base::StringPrintf("%s %" PRIuS "", machine_tag.c_str(), tab_node_id);
 }
 
-void TabNodePool::AddTabNode(int64 sync_id) {
+void TabNodePool::AddOldTabNode(int64 sync_id, size_t tab_node_id) {
+  DCHECK_GT(sync_id, 0);
+  DCHECK(old_node_syncids_.find(sync_id) == old_node_syncids_.end());
+  if (max_used_tab_node_id_ < tab_node_id)
+    max_used_tab_node_id_ = tab_node_id;
+  // Grow the tab node pool by 1, to ensure tab pool is of correct size.
   tab_syncid_pool_.resize(tab_syncid_pool_.size() + 1);
-  tab_syncid_pool_[static_cast<size_t>(++tab_pool_fp_)] = sync_id;
+  old_node_syncids_.insert(sync_id);

[Nicolas Zea, 2013/06/06 21:27:39]

I'm not sure we really need to have the notion of "old tab nodes" at all. Could
you not track all these old syncids as just free nodes within the tab syncid
pool (although this might switching from vector to something else)

The general flow could be:
AddTabNode // adds old node to sync id pool, but doesn't track it as associated
AssociateTabNode  // marks it as associated, and therefore not free
FreeUnusuedTabNodes  // free's all non-associated nodes
GetFreeTabNode  // returns next free node, or creates one as necessary

[shashi, 2013/06/07 01:44:08]

In case of Android we have two requirements:
1. Old tab nodes must not be deleted.
2. Less obvious, old tab nodes should get reassociated with the *same* old sync
node.

Requirement 1 should be obvious, let me explain reason for requirement 2:

On Android some tabs can be without webcontents. For tabs without webcontents
(TWW) we do not have data to write tabs contents to sync model. Instead we use a
trick, we let the old sync nodes associated with those tabs to be unchanged. 

Now consider this from the point of view of a remote client.
Remote client views that the sync nodes associated with TWW are unchanged, it
will display TWW as a regular tab.
For tabs which have webcontents any free node will work, because SMA will
rewrite the tab contents to the sync node (in SMA::WriteTabContentsToSyncModel).


Keeping track of old sync ids is simply to make sure that we do not accidentally
overwrite a sync node associated with TWW.

On 2013/06/06 21:27:39, Nicolas Zea wrote:

[Nicolas Zea, 2013/06/07 14:42:28]

Right, and I agree with that approach. My point is that (I think) the API itself
doesn't need to reflect this. From an external point of view there are the
following categories of nodes:
1. Those associated with an active tab (associated nodes)
2. Those not associated with an active tab (free nodes)

All nodes have a "last associated tab node id", but start off as free on any
particular restart. The associator, at association time, tells the tab pool
which of the free nodes are in fact associated. Previously it didn't care about
associating with a particular node, so GetFreeTabNode was enough. Now, we do, so
a specific AssociateTabNode(sync_id) method is probably necessary.

But, given some sort of AssociateTabNode method, it's not clear to me that the
notion of "old" nodes really needs to be exposed.

[shashi, 2013/06/07 19:08:35]

Got it, I am going to cleanup the API as per the suggestion.
On 2013/06/07 14:42:28, Nicolas Zea wrote:

 }
 
 int64 TabNodePool::GetFreeTabNode() {
@@ -51,7 +56,7 @@ int64 TabNodePool::GetFreeTabNode() {
       LOG(ERROR) << kNoSessionsFolderError;
       return syncer::kInvalidId;
     }
-    size_t tab_node_id = tab_syncid_pool_.size();
+    size_t tab_node_id = ++max_used_tab_node_id_;
     std::string tab_node_tag = TabIdToTag(machine_tag_, tab_node_id);
     syncer::WriteNode tab_node(&trans);
     syncer::WriteNode::InitUniqueByCreationResult result =
@@ -72,9 +77,9 @@ int64 TabNodePool::GetFreeTabNode() {
     // Grow the pool by 1 since we created a new node. We don't actually need
     // to put the node's id in the pool now, since the pool is still empty.
     // The id will be added when that tab is closed and the node is freed.
-    tab_syncid_pool_.resize(tab_node_id + 1);
-    DVLOG(1) << "Adding sync node "
-             << tab_node.GetId() << " to tab syncid pool";
+    tab_syncid_pool_.resize(tab_syncid_pool_.size() + 1);
+    DVLOG(1) << "Adding sync node " << tab_node.GetId()
+             << " to tab syncid pool";
     return tab_node.GetId();
   } else {
     // There are nodes available, grab next free and decrement free pointer.
@@ -88,4 +93,26 @@ void TabNodePool::FreeTabNode(int64 sync_id) {
   tab_syncid_pool_[static_cast<size_t>(++tab_pool_fp_)] = sync_id;
 }
 
+bool TabNodePool::RemoveIfOldSyncNodeExists(int64 sync_id) {
+  std::set<int64>::iterator position = old_node_syncids_.find(sync_id);
+  if (position != old_node_syncids_.end()) {
+    // If sync_id exists remove it and return.
+    old_node_syncids_.erase(position);
+    return true;
+  }
+  return false;
+}
+
+void TabNodePool::FreeUnusedOldSyncNodes(const std::set<int64>& used_sync_ids) {
+  for (std::set<int64>::iterator it = old_node_syncids_.begin();
+       it != old_node_syncids_.end();) {
+    if (used_sync_ids.find(*it) == used_sync_ids.end()) {
+      FreeTabNode(*it);
+      old_node_syncids_.erase(it++);
+    } else {
+      ++it;
+    }
+  }
+}
+
 }  // namespace browser_sync