Reland of [Sync] Put session tracker in charge of maintaining local state.

components/sync_sessions/tab_node_pool.cc

Index: components/sync_sessions/tab_node_pool.cc
diff --git a/components/sync_sessions/tab_node_pool.cc b/components/sync_sessions/tab_node_pool.cc
index 4067b92894da4b50887a56fcbd07566090e98ab1..831629f98395720936df7f035183fa332e1af625 100644
--- a/components/sync_sessions/tab_node_pool.cc
+++ b/components/sync_sessions/tab_node_pool.cc
@@ -4,12 +4,8 @@
 
 #include "components/sync_sessions/tab_node_pool.h"
 
-#include "base/format_macros.h"
 #include "base/logging.h"
-#include "base/strings/stringprintf.h"
 #include "components/sync/base/model_type.h"
-#include "components/sync/model/sync_change.h"
-#include "components/sync/model/sync_data.h"
 #include "components/sync/protocol/session_specifics.pb.h"
 #include "components/sync/protocol/sync.pb.h"
 
@@ -26,114 +22,95 @@ const int TabNodePool::kInvalidTabNodeID = -1;
 
 TabNodePool::~TabNodePool() {}
 
-// Static
-std::string TabNodePool::TabIdToTag(const std::string& machine_tag,
-                                    int tab_node_id) {
-  return base::StringPrintf("%s %d", machine_tag.c_str(), tab_node_id);
-}
-
 void TabNodePool::AddTabNode(int tab_node_id) {
   DCHECK_GT(tab_node_id, kInvalidTabNodeID);
   DCHECK(nodeid_tabid_map_.find(tab_node_id) == nodeid_tabid_map_.end());
-  unassociated_nodes_.insert(tab_node_id);
+  DVLOG(1) << "Adding tab node " << tab_node_id << " to pool.";
   if (max_used_tab_node_id_ < tab_node_id)

[skym, 2016/12/03 01:20:46]

do you think std::max would be more clear?

max_used_tab_node_id_ = std::max(max_used_tab_node_id_, tab_node_id);

[Nicolas Zea, 2016/12/06 01:32:55]

Done.

     max_used_tab_node_id_ = tab_node_id;
+  free_nodes_pool_.insert(tab_node_id);
 }
 
 void TabNodePool::AssociateTabNode(int tab_node_id, SessionID::id_type tab_id) {
   DCHECK_GT(tab_node_id, kInvalidTabNodeID);
-  // Remove sync node if it is in unassociated nodes pool.
-  std::set<int>::iterator u_it = unassociated_nodes_.find(tab_node_id);
-  if (u_it != unassociated_nodes_.end()) {
-    unassociated_nodes_.erase(u_it);
-  } else {
-    // This is a new node association, the sync node should be free.
-    // Remove node from free node pool and then associate it with the tab.
-    std::set<int>::iterator it = free_nodes_pool_.find(tab_node_id);
-    DCHECK(it != free_nodes_pool_.end());
-    free_nodes_pool_.erase(it);
-  }
+  DCHECK_GT(tab_id, kInvalidTabID);
+
+  // This is a new node association, the sync node should be free.
+  // Remove node from free node pool and then associate it with the tab.
+  std::set<int>::iterator it = free_nodes_pool_.find(tab_node_id);
+  DCHECK(it != free_nodes_pool_.end());
+  free_nodes_pool_.erase(it);
+
   DCHECK(nodeid_tabid_map_.find(tab_node_id) == nodeid_tabid_map_.end());
+  DVLOG(1) << "Associating tab node " << tab_node_id << " with tab " << tab_id;
   nodeid_tabid_map_[tab_node_id] = tab_id;
+  tabid_nodeid_map_[tab_id] = tab_node_id;
 }
 
-int TabNodePool::GetFreeTabNode(syncer::SyncChangeList* append_changes) {
-  DCHECK_GT(machine_tag_.length(), 0U);
-  DCHECK(append_changes);
+bool TabNodePool::GetTabNodeForTab(SessionID::id_type tab_id,
+                                   int* tab_node_id) {
+  if (tabid_nodeid_map_.find(tab_id) != tabid_nodeid_map_.end()) {
+    *tab_node_id = tabid_nodeid_map_[tab_id];
+    return true;
+  }
+
   if (free_nodes_pool_.empty()) {
     // Tab pool has no free nodes, allocate new one.
-    int tab_node_id = ++max_used_tab_node_id_;
-    std::string tab_node_tag = TabIdToTag(machine_tag_, tab_node_id);
-
-    // We fill the new node with just enough data so that in case of a crash/bug
-    // we can identify the node as our own on re-association and reuse it.
-    sync_pb::EntitySpecifics entity;
-    sync_pb::SessionSpecifics* specifics = entity.mutable_session();
-    specifics->set_session_tag(machine_tag_);
-    specifics->set_tab_node_id(tab_node_id);
-    append_changes->push_back(syncer::SyncChange(
-        FROM_HERE, syncer::SyncChange::ACTION_ADD,
-        syncer::SyncData::CreateLocalData(tab_node_tag, tab_node_tag, entity)));
+    *tab_node_id = ++max_used_tab_node_id_;
 
     // Grow the pool by 1 since we created a new node.
-    DVLOG(1) << "Adding sync node " << tab_node_id << " to tab node id pool";
-    free_nodes_pool_.insert(tab_node_id);
-    return tab_node_id;
+    DVLOG(1) << "Adding tab node " << *tab_node_id << " to pool";
+    free_nodes_pool_.insert(*tab_node_id);

[skym, 2016/12/03 01:20:46]

This is kind of odd  that you put it in free_nodes_pool_ and then immediately
erase it in the functional call below.

[Nicolas Zea, 2016/12/06 01:32:55]

It allows for consistency. AssociateTabNode assumes the node is free. We're
basically creating a tab node here, so it makes sense to create it as free
first.

Changed this to just call AddTabNode instead for clarity.

+
+    AssociateTabNode(*tab_node_id, tab_id);
+    return false;
   } else {
     // Return the next free node.
-    return *free_nodes_pool_.begin();
+    *tab_node_id = *free_nodes_pool_.begin();
+    AssociateTabNode(*tab_node_id, tab_id);
+    return true;
   }
 }
 
-void TabNodePool::FreeTabNode(int tab_node_id,
-                              syncer::SyncChangeList* append_changes) {
-  DCHECK(append_changes);
-  TabNodeIDToTabIDMap::iterator it = nodeid_tabid_map_.find(tab_node_id);
-  DCHECK(it != nodeid_tabid_map_.end());
-  nodeid_tabid_map_.erase(it);
-  FreeTabNodeInternal(tab_node_id, append_changes);
-}
-
-void TabNodePool::FreeTabNodeInternal(int tab_node_id,
-                                      syncer::SyncChangeList* append_changes) {
-  DCHECK(free_nodes_pool_.find(tab_node_id) == free_nodes_pool_.end());
-  DCHECK(append_changes);
-  free_nodes_pool_.insert(tab_node_id);
-
-  // If number of free nodes exceed kFreeNodesHighWatermark,
-  // delete sync nodes till number reaches kFreeNodesLowWatermark.
-  // Note: This logic is to mitigate temporary disassociation issues with old
-  // clients: http://crbug.com/259918. Newer versions do not need this.
-  if (free_nodes_pool_.size() > kFreeNodesHighWatermark) {
-    for (std::set<int>::iterator free_it = free_nodes_pool_.begin();
-         free_it != free_nodes_pool_.end();) {
-      const std::string tab_node_tag = TabIdToTag(machine_tag_, *free_it);
-      append_changes->push_back(syncer::SyncChange(
-          FROM_HERE, syncer::SyncChange::ACTION_DELETE,
-          syncer::SyncData::CreateLocalDelete(tab_node_tag, syncer::SESSIONS)));
-      free_nodes_pool_.erase(free_it++);
-      if (free_nodes_pool_.size() <= kFreeNodesLowWatermark) {
-        return;
-      }
-    }
+void TabNodePool::FreeTab(int tab_id) {
+  DCHECK_GT(tab_id, kInvalidTabID);
+  TabIDToTabNodeIDMap::iterator it = tabid_nodeid_map_.find(tab_id);
+  if (it == tabid_nodeid_map_.end()) {
+    return;  // Already freed.
   }
-}
 
-bool TabNodePool::IsUnassociatedTabNode(int tab_node_id) {
-  return unassociated_nodes_.find(tab_node_id) != unassociated_nodes_.end();
+  int tab_node_id = it->second;
+  DVLOG(1) << "Freeing tab " << tab_id << " at node " << tab_node_id;
+  nodeid_tabid_map_.erase(nodeid_tabid_map_.find(tab_node_id));
+  tabid_nodeid_map_.erase(it);
+  free_nodes_pool_.insert(tab_node_id);
 }
 
 void TabNodePool::ReassociateTabNode(int tab_node_id,
                                      SessionID::id_type tab_id) {
-  // Remove from list of unassociated sync_nodes if present.
-  std::set<int>::iterator it = unassociated_nodes_.find(tab_node_id);
-  if (it != unassociated_nodes_.end()) {
-    unassociated_nodes_.erase(it);
+  DCHECK_GT(tab_node_id, kInvalidTabNodeID);
+  DCHECK_GT(tab_id, kInvalidTabID);
+
+  auto tabid_it = tabid_nodeid_map_.find(tab_id);
+  if (tabid_it != tabid_nodeid_map_.end()) {
+    if (tabid_it->second == tab_node_id) {
+      return;  // Already associated properly.
+    } else {
+      // Another node is already associated with this tab. Free it.
+      FreeTab(tab_id);
+    }
+  }
+
+  auto nodeid_it = nodeid_tabid_map_.find(tab_node_id);
+  if (nodeid_it != nodeid_tabid_map_.end()) {
+    // This node was already associated with another tab. Free it.
+    FreeTab(nodeid_it->second);
   } else {
-    // tab_node_id must be an already associated node.
-    DCHECK(nodeid_tabid_map_.find(tab_node_id) != nodeid_tabid_map_.end());
+    // This is a new tab node. Add it before association.
+    AddTabNode(tab_node_id);
   }
-  nodeid_tabid_map_[tab_node_id] = tab_id;
+
+  AssociateTabNode(tab_node_id, tab_id);
 }
 
 SessionID::id_type TabNodePool::GetTabIdFromTabNodeId(int tab_node_id) const {
@@ -144,27 +121,33 @@ SessionID::id_type TabNodePool::GetTabIdFromTabNodeId(int tab_node_id) const {
   return kInvalidTabID;
 }
 
-void TabNodePool::DeleteUnassociatedTabNodes(
-    syncer::SyncChangeList* append_changes) {
-  for (std::set<int>::iterator it = unassociated_nodes_.begin();
-       it != unassociated_nodes_.end();) {
-    FreeTabNodeInternal(*it, append_changes);
-    unassociated_nodes_.erase(it++);
+void TabNodePool::CleanupTabNodes(std::set<int>* deleted_node_ids) {
+  // If number of free nodes exceed kFreeNodesHighWatermark,
+  // delete sync nodes till number reaches kFreeNodesLowWatermark.
+  // Note: This logic is to mitigate temporary disassociation issues with old
+  // clients: http://crbug.com/259918. Newer versions do not need this.
+  if (free_nodes_pool_.size() > kFreeNodesHighWatermark) {
+    for (std::set<int>::iterator free_it = free_nodes_pool_.begin();
+         free_it != free_nodes_pool_.end();) {
+      deleted_node_ids->insert(*free_it);
+      free_nodes_pool_.erase(free_it++);
+      if (free_nodes_pool_.size() <= kFreeNodesLowWatermark) {
+        return;
+      }
+    }
   }
-  DCHECK(unassociated_nodes_.empty());
 }
 
 // Clear tab pool.
 void TabNodePool::Clear() {
-  unassociated_nodes_.clear();
   free_nodes_pool_.clear();
   nodeid_tabid_map_.clear();
+  tabid_nodeid_map_.clear();
   max_used_tab_node_id_ = kInvalidTabNodeID;
 }
 
 size_t TabNodePool::Capacity() const {
-  return nodeid_tabid_map_.size() + unassociated_nodes_.size() +
-         free_nodes_pool_.size();
+  return nodeid_tabid_map_.size() + free_nodes_pool_.size();
 }
 
 bool TabNodePool::Empty() const {
@@ -175,8 +158,4 @@ bool TabNodePool::Full() {
   return nodeid_tabid_map_.empty();
 }
 
-void TabNodePool::SetMachineTag(const std::string& machine_tag) {
-  machine_tag_ = machine_tag;
-}
-
 }  // namespace sync_sessions