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

components/sync_sessions/sessions_sync_manager.cc

Index: components/sync_sessions/sessions_sync_manager.cc
diff --git a/components/sync_sessions/sessions_sync_manager.cc b/components/sync_sessions/sessions_sync_manager.cc
index acb6882b7bcae3f68e4bfc4110af0daf1694b060..be55c33f2049ed084202234fdd6ec65e109cac54 100644
--- a/components/sync_sessions/sessions_sync_manager.cc
+++ b/components/sync_sessions/sessions_sync_manager.cc
@@ -7,9 +7,12 @@
 #include <algorithm>
 #include <utility>
 
+#include "base/format_macros.h"
+#include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "components/sync/base/hash_util.h"
 #include "components/sync/device_info/local_device_info_provider.h"
@@ -21,6 +24,7 @@
 #include "components/sync_sessions/synced_tab_delegate.h"
 #include "components/sync_sessions/synced_window_delegate.h"
 #include "components/sync_sessions/synced_window_delegates_getter.h"
+#include "components/sync_sessions/tab_node_pool.h"
 #include "components/variations/variations_associated_data.h"
 
 using sessions::SerializedNavigationEntry;
@@ -62,17 +66,31 @@ bool SessionsRecencyComparator(const SyncedSession* s1,
   return s1->modified_time > s2->modified_time;
 }
 
+std::string TabNodeIdToTag(const std::string& machine_tag, int tab_node_id) {

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

Is duplicating in anonymous namespaces between .cc and unittest.cc preferred
over making it private static and friending? (test is already a friend)

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

Given it's just a one-liner with no real logic, it seems simpler this way. I can
change if you prefer.

+  return base::StringPrintf("%s %d", machine_tag.c_str(), tab_node_id);
+}
+
 std::string TagFromSpecifics(const sync_pb::SessionSpecifics& specifics) {
   if (specifics.has_header()) {
     return specifics.session_tag();
   } else if (specifics.has_tab()) {
-    return TabNodePool::TabIdToTag(specifics.session_tag(),
-                                   specifics.tab_node_id());
+    return TabNodeIdToTag(specifics.session_tag(), specifics.tab_node_id());
   } else {
     return std::string();
   }
 }
 
+sync_pb::SessionSpecifics SessionTabToSpecifics(
+    const sessions::SessionTab& session_tab,
+    const std::string& local_tag,
+    int tab_node_id) {
+  sync_pb::SessionSpecifics specifics;
+  specifics.mutable_tab()->CopyFrom(session_tab.ToSyncData());
+  specifics.set_session_tag(local_tag);
+  specifics.set_tab_node_id(tab_node_id);
+  return specifics;
+}
+
 }  // namespace
 
 // |local_device| is owned by ProfileSyncService, its lifetime exceeds
@@ -116,7 +134,6 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
     std::unique_ptr<syncer::SyncErrorFactory> error_handler) {
   syncer::SyncMergeResult merge_result(type);
   DCHECK(session_tracker_.Empty());
-  DCHECK_EQ(0U, local_tab_pool_.Capacity());
 
   error_handler_ = std::move(error_handler);
   sync_processor_ = std::move(sync_processor);
@@ -151,13 +168,12 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
     return merge_result;
   }
 
-  session_tracker_.SetLocalSessionTag(current_machine_tag_);
+  session_tracker_.SetLocalSessionTag(current_machine_tag());
 
   syncer::SyncChangeList new_changes;
 
   // First, we iterate over sync data to update our session_tracker_.
-  syncer::SyncDataList restored_tabs;
-  if (!InitFromSyncModel(initial_sync_data, &restored_tabs, &new_changes)) {
+  if (!InitFromSyncModel(initial_sync_data, &new_changes)) {
     // The sync db didn't have a header node for us. Create one.
     sync_pb::EntitySpecifics specifics;
     sync_pb::SessionSpecifics* base_specifics = specifics.mutable_session();
@@ -174,12 +190,12 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
 #if defined(OS_ANDROID)
   std::string sync_machine_tag(
       BuildMachineTag(local_device_->GetLocalSyncCacheGUID()));
-  if (current_machine_tag_.compare(sync_machine_tag) != 0)
+  if (current_machine_tag().compare(sync_machine_tag) != 0)
     DeleteForeignSessionInternal(sync_machine_tag, &new_changes);
 #endif
 
   // Check if anything has changed on the local client side.
-  AssociateWindows(RELOAD_TABS, restored_tabs, &new_changes);
+  AssociateWindows(RELOAD_TABS, &new_changes);
   local_tab_pool_out_of_sync_ = false;
 
   merge_result.set_error(
@@ -191,7 +207,6 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
 
 void SessionsSyncManager::AssociateWindows(
     ReloadTabsOption option,
-    const syncer::SyncDataList& restored_tabs,
     syncer::SyncChangeList* change_output) {
   const std::string local_tag = current_machine_tag();
   sync_pb::SessionSpecifics specifics;
@@ -273,7 +288,7 @@ void SessionsSyncManager::AssociateWindows(
           if (synced_tab->GetSyncId() > TabNodePool::kInvalidTabNodeID &&
               tab_id > TabNodePool::kInvalidTabID) {
             AssociateRestoredPlaceholderTab(*synced_tab, tab_id, window_id,
-                                            restored_tabs, change_output);
+                                            change_output);
             found_tabs = true;
             window_s.add_tab(tab_id);
           }
@@ -306,8 +321,15 @@ void SessionsSyncManager::AssociateWindows(
       }
     }
   }
-  local_tab_pool_.DeleteUnassociatedTabNodes(change_output);
-  session_tracker_.CleanupSession(local_tag);
+  std::set<int> deleted_tab_node_ids;
+  session_tracker_.CleanupLocalTabs(&deleted_tab_node_ids);
+  for (int tab_node_id : deleted_tab_node_ids) {
+    std::string tab_node_tag =
+        TabNodeIdToTag(current_machine_tag(), tab_node_id);
+    change_output->push_back(syncer::SyncChange(
+        FROM_HERE, syncer::SyncChange::ACTION_DELETE,
+        syncer::SyncData::CreateLocalDelete(tab_node_tag, syncer::SESSIONS)));
+  }
 
   // Always update the header.  Sync takes care of dropping this update
   // if the entity specifics are identical (i.e windows, client name did
@@ -320,73 +342,62 @@ void SessionsSyncManager::AssociateWindows(
       syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
 }
 
-void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab,
+void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab_delegate,
                                        syncer::SyncChangeList* change_output) {
-  DCHECK(!tab->IsPlaceholderTab());
-  SessionID::id_type tab_id = tab->GetSessionId();
-
-  if (tab->IsBeingDestroyed()) {
-    // This tab is closing.
-    TabLinksMap::iterator tab_iter = local_tab_map_.find(tab_id);
-    if (tab_iter == local_tab_map_.end()) {
-      // We aren't tracking this tab (for example, sync setting page).
-      return;
-    }
-    local_tab_pool_.FreeTabNode(tab_iter->second->tab_node_id(), change_output);
-    local_tab_map_.erase(tab_iter);
+  DCHECK(!tab_delegate->IsPlaceholderTab());
+
+  if (tab_delegate->IsBeingDestroyed()) {
+    // Do nothing. By not proactively adding the tab to the session, it will be
+    // removed if necessary during subsequent cleanup.
     return;
   }
 
-  if (!tab->ShouldSync(sessions_client_))
+  if (!tab_delegate->ShouldSync(sessions_client_))
     return;
 
-  TabLinksMap::iterator local_tab_map_iter = local_tab_map_.find(tab_id);
-  TabLink* tab_link = nullptr;
-
-  if (local_tab_map_iter == local_tab_map_.end()) {
-    int tab_node_id = tab->GetSyncId();
-    // If there is an old sync node for the tab, reuse it.  If this is a new
-    // tab, get a sync node for it.
-    if (!local_tab_pool_.IsUnassociatedTabNode(tab_node_id)) {
-      tab_node_id = local_tab_pool_.GetFreeTabNode(change_output);
-      tab->SetSyncId(tab_node_id);
-    }
-    local_tab_pool_.AssociateTabNode(tab_node_id, tab_id);
-    tab_link = new TabLink(tab_node_id, tab);
-    local_tab_map_[tab_id] = make_linked_ptr<TabLink>(tab_link);
-  } else {
-    // This tab is already associated with a sync node, reuse it.
-    // Note: on some platforms the tab object may have changed, so we ensure
-    // the tab link is up to date.
-    tab_link = local_tab_map_iter->second.get();
-    local_tab_map_iter->second->set_tab(tab);
-  }
-  DCHECK(tab_link);
-  DCHECK_NE(tab_link->tab_node_id(), TabNodePool::kInvalidTabNodeID);
+  SessionID::id_type tab_id = tab_delegate->GetSessionId();
   DVLOG(1) << "Reloading tab " << tab_id << " from window "
-           << tab->GetWindowId();
+           << tab_delegate->GetWindowId();
+
+  int tab_node_id = TabNodePool::kInvalidTabNodeID;
+  bool existing_tab_node =
+      session_tracker_.GetTabNodeForLocalTab(tab_id, &tab_node_id);
+  DCHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id);
+  sessions::SessionTab* session_tab =
+      session_tracker_.GetTab(current_machine_tag(), tab_id);
+
+  // Get the previously synced url.
+  int old_index = session_tab->normalized_navigation_index();
+  GURL old_url;
+  if (session_tab->navigations.size() > static_cast<size_t>(old_index))
+    old_url = session_tab->navigations[old_index].virtual_url();
+
+  // Update the tracker's session representation.
+  SetSessionTabFromDelegate(*tab_delegate, base::Time::Now(), session_tab);
+  SetVariationIds(session_tab);
+  session_tracker_.GetSession(current_machine_tag())->modified_time =
+      base::Time::Now();
 
-  // Write to sync model.
+  // Write to the sync model itself.
   sync_pb::EntitySpecifics specifics;
-  LocalTabDelegateToSpecifics(*tab, specifics.mutable_session());
+  specifics.mutable_session()->CopyFrom(
+      SessionTabToSpecifics(*session_tab, current_machine_tag(), tab_node_id));
   syncer::SyncData data = syncer::SyncData::CreateLocalData(
-      TabNodePool::TabIdToTag(current_machine_tag_, tab_link->tab_node_id()),
-      current_session_name_, specifics);
-  change_output->push_back(
-      syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
-
-  int current_index = tab->GetCurrentEntryIndex();
-  const GURL new_url = tab->GetVirtualURLAtIndex(current_index);
-  if (new_url != tab_link->url()) {
-    tab_link->set_url(new_url);
-    favicon_cache_.OnFaviconVisited(new_url,
-                                    tab->GetFaviconURLAtIndex(current_index));
+      TabNodeIdToTag(current_machine_tag(), tab_node_id), current_session_name_,
+      specifics);
+  change_output->push_back(syncer::SyncChange(
+      FROM_HERE, existing_tab_node ? syncer::SyncChange::ACTION_UPDATE
+                                   : syncer::SyncChange::ACTION_ADD,
+      data));
+
+  int current_index = tab_delegate->GetCurrentEntryIndex();
+  const GURL new_url = tab_delegate->GetVirtualURLAtIndex(current_index);
+  if (new_url != old_url) {
+    favicon_cache_.OnFaviconVisited(
+        new_url, tab_delegate->GetFaviconURLAtIndex(current_index));
     page_revisit_broadcaster_.OnPageVisit(
-        new_url, tab->GetTransitionAtIndex(current_index));
+        new_url, tab_delegate->GetTransitionAtIndex(current_index));
   }
-
-  session_tracker_.GetSession(current_machine_tag())->modified_time =
-      base::Time::Now();
 }
 
 void SessionsSyncManager::RebuildAssociations() {
@@ -445,21 +456,14 @@ void SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
   // "interesting" by going to a valid URL, in which case it needs to be added
   // to the window's tab information. Similarly, if a tab became
   // "uninteresting", we remove it from the window's tab information.
-  AssociateWindows(DONT_RELOAD_TABS, syncer::SyncDataList(), &changes);
+  AssociateWindows(DONT_RELOAD_TABS, &changes);
   sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
 void SessionsSyncManager::OnFaviconsChanged(const std::set<GURL>& page_urls,
                                             const GURL& /* icon_url */) {
-  // TODO(zea): consider a separate container for tabs with outstanding favicon
-  // loads so we don't have to iterate through all tabs comparing urls.
-  for (const GURL& page_url : page_urls) {
-    for (TabLinksMap::iterator tab_iter = local_tab_map_.begin();
-         tab_iter != local_tab_map_.end(); ++tab_iter) {
-      if (tab_iter->second->url() == page_url)
-        favicon_cache_.OnPageFaviconUpdated(page_url);
-    }
-  }
+  for (const GURL& page_url : page_urls)
+    favicon_cache_.OnPageFaviconUpdated(page_url);
 }
 
 void SessionsSyncManager::StopSyncing(syncer::ModelType type) {
@@ -472,8 +476,6 @@ void SessionsSyncManager::StopSyncing(syncer::ModelType type) {
   sync_processor_.reset(nullptr);
   error_handler_.reset();
   session_tracker_.Clear();
-  local_tab_map_.clear();
-  local_tab_pool_.Clear();
   current_machine_tag_.clear();
   current_session_name_.clear();
   local_session_header_node_id_ = TabNodePool::kInvalidTabNodeID;
@@ -496,22 +498,17 @@ syncer::SyncDataList SessionsSyncManager::GetAllSyncData(
       current_machine_tag(), current_session_name_, header_entity);
   list.push_back(data);
 
-  for (auto win_iter = session->windows.begin();
-       win_iter != session->windows.end(); ++win_iter) {
-    for (auto tabs_iter = win_iter->second->tabs.begin();
-         tabs_iter != win_iter->second->tabs.end(); ++tabs_iter) {
+  for (auto& win_iter : session->windows) {
+    for (auto& tab : win_iter.second->tabs) {
+      // TODO(zea): replace with with the correct tab node id once there's a
+      // sync specific wrapper for SessionTab. This method is only used in
+      // tests though, so it's fine for now. crbug.com/662597
+      int tab_node_id = 0;
       sync_pb::EntitySpecifics entity;
-      sync_pb::SessionSpecifics* specifics = entity.mutable_session();
-      specifics->mutable_tab()->MergeFrom((*tabs_iter)->ToSyncData());
-      specifics->set_session_tag(current_machine_tag_);
-
-      TabLinksMap::const_iterator tab_map_iter =
-          local_tab_map_.find((*tabs_iter)->tab_id.id());
-      DCHECK(tab_map_iter != local_tab_map_.end());
-      specifics->set_tab_node_id(tab_map_iter->second->tab_node_id());
+      entity.mutable_session()->CopyFrom(
+          SessionTabToSpecifics(*tab, current_machine_tag(), tab_node_id));
       syncer::SyncData data = syncer::SyncData::CreateLocalData(
-          TabNodePool::TabIdToTag(current_machine_tag_,
-                                  specifics->tab_node_id()),
+          TabNodeIdToTag(current_machine_tag(), tab_node_id),
           current_session_name_, entity);
       list.push_back(data);
     }
@@ -520,7 +517,7 @@ syncer::SyncDataList SessionsSyncManager::GetAllSyncData(
 }
 
 bool SessionsSyncManager::GetLocalSession(const SyncedSession** local_session) {
-  if (current_machine_tag_.empty())
+  if (current_machine_tag().empty())
     return false;
   *local_session = session_tracker_.GetSession(current_machine_tag());
   return true;
@@ -580,7 +577,7 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
           LOG(WARNING) << "Dropping modification to local session.";
           return syncer::SyncError();
         }
-        UpdateTrackerWithForeignSession(
+        UpdateTrackerWithSpecifics(
             session, syncer::SyncDataRemote(it->sync_data()).GetModifiedTime());
         break;
       default:
@@ -602,7 +599,7 @@ syncer::SyncChange SessionsSyncManager::TombstoneTab(
     return syncer::SyncChange(
         FROM_HERE, SyncChange::ACTION_DELETE,
         SyncData::CreateLocalDelete(
-            TabNodePool::TabIdToTag(current_machine_tag(), tab.tab_node_id()),
+            TabNodeIdToTag(current_machine_tag(), tab.tab_node_id()),
             syncer::SESSIONS));
   }
 }
@@ -618,7 +615,6 @@ bool SessionsSyncManager::GetAllForeignSessions(
 
 bool SessionsSyncManager::InitFromSyncModel(
     const syncer::SyncDataList& sync_data,
-    syncer::SyncDataList* restored_tabs,
     syncer::SyncChangeList* new_changes) {
   bool found_current_header = false;
   int bad_foreign_hash_count = 0;
@@ -636,7 +632,7 @@ bool SessionsSyncManager::InitFromSyncModel(
         new_changes->push_back(tombstone);
     } else if (specifics.session_tag() != current_machine_tag()) {
       if (TagHashFromSpecifics(specifics) == remote.GetClientTagHash()) {
-        UpdateTrackerWithForeignSession(specifics, remote.GetModifiedTime());
+        UpdateTrackerWithSpecifics(specifics, remote.GetModifiedTime());
       } else {
         // In the past, like years ago, we believe that some session data was
         // created with bad tag hashes. This causes any change this client makes
@@ -655,6 +651,10 @@ bool SessionsSyncManager::InitFromSyncModel(
         found_current_header = true;
         if (specifics.header().has_client_name())
           current_session_name_ = specifics.header().client_name();
+
+        // TODO(zea): crbug.com/639009 update the tracker with the specifics
+        // from the header node as well. This will be necessary to preserve
+        // the set of open tabs when a custom tab is opened.
       } else {
         if (specifics.has_header() || !specifics.has_tab()) {
           LOG(WARNING) << "Found more than one session header node with local "
@@ -663,10 +663,11 @@ bool SessionsSyncManager::InitFromSyncModel(
           if (tombstone.IsValid())
             new_changes->push_back(tombstone);
         } else {
-          // This is a valid old tab node, add it to the pool so it can be
-          // reused for reassociation.
-          local_tab_pool_.AddTabNode(specifics.tab_node_id());
-          restored_tabs->push_back(*it);
+          // This is a valid old tab node, add it to the tracker and associate
+          // it.
+          session_tracker_.ReassociateLocalTab(specifics.tab_node_id(),

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

Err, is this really correct? What if you've already received a
OnLocalTabModified for a SyncedTabDelegate with this tab node id, but they've
swapped the tab id on you? This would overwrite correct data with stale data,
right? Or am I missing something?

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

InitFromSyncModel is called before OnLocalTabModified can be called
(InitFromSyncModel is part of the initial associated).

+                                               specifics.tab().tab_id());
+          UpdateTrackerWithSpecifics(specifics, remote.GetModifiedTime());
         }
       }
     }
@@ -678,7 +679,7 @@ bool SessionsSyncManager::InitFromSyncModel(
   session_tracker_.LookupAllForeignSessions(&sessions,
                                             SyncedSessionTracker::RAW);
   for (const auto* session : sessions) {
-    session_tracker_.CleanupSession(session->session_tag);
+    session_tracker_.CleanupForeignSession(session->session_tag);
   }
 
   UMA_HISTOGRAM_COUNTS_100("Sync.SessionsBadForeignHashOnMergeCount",
@@ -687,69 +688,62 @@ bool SessionsSyncManager::InitFromSyncModel(
   return found_current_header;
 }
 
-void SessionsSyncManager::UpdateTrackerWithForeignSession(
+void SessionsSyncManager::UpdateTrackerWithSpecifics(
     const sync_pb::SessionSpecifics& specifics,
     const base::Time& modification_time) {
-  std::string foreign_session_tag = specifics.session_tag();
-  DCHECK_NE(foreign_session_tag, current_machine_tag());
-
-  SyncedSession* foreign_session =
-      session_tracker_.GetSession(foreign_session_tag);
+  std::string session_tag = specifics.session_tag();
+  SyncedSession* session = session_tracker_.GetSession(session_tag);
   if (specifics.has_header()) {
-    // Read in the header data for this foreign session. Header data is
+    // Read in the header data for this session. Header data is
     // essentially a collection of windows, each of which has an ordered id list
     // for their tabs.
 
     if (!IsValidSessionHeader(specifics.header())) {
-      LOG(WARNING) << "Ignoring foreign session node with invalid header "
-                   << "and tag " << foreign_session_tag << ".";
+      LOG(WARNING) << "Ignoring session node with invalid header "
+                   << "and tag " << session_tag << ".";
       return;
     }
 
     // Load (or create) the SyncedSession object for this client.
     const sync_pb::SessionHeader& header = specifics.header();
-    PopulateSessionHeaderFromSpecifics(header, modification_time,
-                                       foreign_session);
+    PopulateSessionHeaderFromSpecifics(header, modification_time, session);
 
     // Reset the tab/window tracking for this session (must do this before
     // we start calling PutWindowInSession and PutTabInWindow so that all
     // unused tabs/windows get cleared by the CleanupSession(...) call).
-    session_tracker_.ResetSessionTracking(foreign_session_tag);
+    session_tracker_.ResetSessionTracking(session_tag);
 
     // Process all the windows and their tab information.
     int num_windows = header.window_size();
-    DVLOG(1) << "Associating " << foreign_session_tag << " with " << num_windows
+    DVLOG(1) << "Associating " << session_tag << " with " << num_windows
              << " windows.";
 
     for (int i = 0; i < num_windows; ++i) {
       const sync_pb::SessionWindow& window_s = header.window(i);
       SessionID::id_type window_id = window_s.window_id();
-      session_tracker_.PutWindowInSession(foreign_session_tag, window_id);
-      BuildSyncedSessionFromSpecifics(
-          foreign_session_tag, window_s, modification_time,
-          foreign_session->windows[window_id].get());
+      session_tracker_.PutWindowInSession(session_tag, window_id);
+      BuildSyncedSessionFromSpecifics(session_tag, window_s, modification_time,
+                                      session->windows[window_id].get());
     }
     // Delete any closed windows and unused tabs as necessary.
-    session_tracker_.CleanupSession(foreign_session_tag);
+    session_tracker_.CleanupForeignSession(session_tag);
   } else if (specifics.has_tab()) {
     const sync_pb::SessionTab& tab_s = specifics.tab();
     SessionID::id_type tab_id = tab_s.tab_id();
 
+    DVLOG(1) << "Associating tab " << tab_id << " with node "

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

Nit: We only actually associate the tab_id with tab_node_id if the tab_id is
already present in session_tracker_, right? This would be more accurate to say
we're "Attempting to associate ..." I guess maybe that's implied.

[skym, 2016/12/05 19:16:57]

Errm, now I'm confused by my comment from last week. And the code. What's going
on here. Which call does the actual "associating"?

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

Are you referring to the if conditional just below? That's just there to catch
cases where two different tab node ids are attempting to refer to the same tab
id (which is invalid). Otherwise, we'll create the tab id in the session tracker
via the GetTab below.

The actual "associating" happens in the caller of this method
(InitFromSyncModel), which invokes ReassociateLocalTab. All we're doing here is
populating the SessionTab with the appropriate data. You're right that the debug
log message isn't quite accurate here. Updated it to better reflect what's
happening.

+             << specifics.tab_node_id();
+    session_tracker_.AddTabNode(session_tag, specifics.tab_node_id());

[skym, 2016/12/05 19:16:57]

I think AddX(...) is a much better method name than GetX(...) being used to
ensure we give data to the tracker when it needs it. But can you add a comment
explaining why this call is needed? (Is it both so we can garbage collect
orphaned foreign tabs, and so we can re-use existing tab ids for local content?
I don't see the tab node id getting passed to the pool though.)

I'm really having a hard time understanding the API that the tracker and pool
classes have, when they need to get called, and what those methods need to do.

All this call is doing is making sure the tracker remembers this tab node id.
But from the method name, that's super not obvious.

...

Actually the more I understand here, the old approach was awkwardly and clunkily
used here, but tried to enforced consistency. If you wanted the
sessions::SessionTab to be auto created, then you had to pass a tab node id. Now
the tracker just trusts that the manager is going to AddTabNode as well here.

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

Cleaned up the comments here. PTAL!

Also re: AddX vs GetX, the issue is that the SessionTracker doesn't understand
specifics, and that's something I kind of want to preserve. It's useful to think
of it as only dealing with the bookkeeping, but not the contents. The manager is
doing a fair amount of work in the processing of the actual sync entities that I
wanted to keep separate, and I think the best way is to provide it direct access
to the SessionTabs themselves.

And yeah, the old API was I think more clunky, so I pulled it into two methods
(since many of the times you call GetTab you don't actually know the tab node
associated with it).

I hoped that AddTabNode would be clear that it's just adding bookkeeping for the
node (as opposed to AddTab, which sounds like it's adding the tab's contents.
Maybe this should be OnTabNodeSeen, or AddTabNodeID?

[skym, 2016/12/06 19:37:21]

Both options sound good to me.

[Nicolas Zea, 2016/12/06 22:31:37]

Switched to OnTabNodeSeen.

+
     const sessions::SessionTab* existing_tab;
-    if (session_tracker_.LookupSessionTab(foreign_session_tag, tab_id,
-                                          &existing_tab) &&
+    if (session_tracker_.LookupSessionTab(session_tag, tab_id, &existing_tab) &&
         existing_tab->timestamp > modification_time) {
-      // Force the tracker to remember this tab node id, even if it isn't
-      // currently being used.
-      session_tracker_.GetTab(foreign_session_tag, tab_id,
-                              specifics.tab_node_id());
-      DVLOG(1) << "Ignoring " << foreign_session_tag << "'s session tab "
-               << tab_id << " with earlier modification time";
+      DVLOG(1) << "Ignoring " << session_tag << "'s session tab " << tab_id
+               << " with earlier modification time";
       return;
     }
 
-    sessions::SessionTab* tab = session_tracker_.GetTab(
-        foreign_session_tag, tab_id, specifics.tab_node_id());
+    sessions::SessionTab* tab = session_tracker_.GetTab(session_tag, tab_id);

[skym, 2016/12/05 19:16:57]

Without passing tab node id to this call, why do we have a separate GetTab and
LookupSessionTab call? They both take the same arguments and return a
sessions::SessionTab object. GetTab(...) does a bunch of creation, but if that's
going to happen, then you will never enter the if clause above to break out
early.

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

Good point, the calls can be combined. Done.

 
     // Update SessionTab based on protobuf.
     tab->SetFromSyncData(tab_s, modification_time);
@@ -759,11 +753,11 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(
     RefreshFaviconVisitTimesFromForeignTab(tab_s, modification_time);
 
     // Update the last modified time.
-    if (foreign_session->modified_time < modification_time)
-      foreign_session->modified_time = modification_time;
+    if (session->modified_time < modification_time)
+      session->modified_time = modification_time;
   } else {
-    LOG(WARNING) << "Ignoring foreign session node with missing header/tab "
-                 << "fields and tag " << foreign_session_tag << ".";
+    LOG(WARNING) << "Ignoring session node with missing header/tab "
+                 << "fields and tag " << session_tag << ".";
   }
 }
 
@@ -782,8 +776,6 @@ void SessionsSyncManager::InitializeCurrentMachineTag() {
     DVLOG(1) << "Creating session sync guid: " << current_machine_tag_;
     sync_prefs_->SetSyncSessionsGUID(current_machine_tag_);
   }
-
-  local_tab_pool_.SetMachineTag(current_machine_tag_);
 }
 
 // static
@@ -893,7 +885,7 @@ void SessionsSyncManager::DeleteForeignSessionInternal(
   }
 
   std::set<int> tab_node_ids_to_delete;
-  session_tracker_.LookupTabNodeIds(tag, &tab_node_ids_to_delete);
+  session_tracker_.LookupForeignTabNodeIds(tag, &tab_node_ids_to_delete);
   if (DisassociateForeignSession(tag)) {
     // Only tell sync to delete the header if there was one.
     change_output->push_back(
@@ -902,10 +894,10 @@ void SessionsSyncManager::DeleteForeignSessionInternal(
   }
   for (std::set<int>::const_iterator it = tab_node_ids_to_delete.begin();
        it != tab_node_ids_to_delete.end(); ++it) {
-    change_output->push_back(syncer::SyncChange(
-        FROM_HERE, SyncChange::ACTION_DELETE,
-        SyncData::CreateLocalDelete(TabNodePool::TabIdToTag(tag, *it),
-                                    syncer::SESSIONS)));
+    change_output->push_back(
+        syncer::SyncChange(FROM_HERE, SyncChange::ACTION_DELETE,
+                           SyncData::CreateLocalDelete(TabNodeIdToTag(tag, *it),
+                                                       syncer::SESSIONS)));
   }
   if (!sessions_updated_callback_.is_null())
     sessions_updated_callback_.Run();
@@ -915,7 +907,7 @@ bool SessionsSyncManager::DisassociateForeignSession(
     const std::string& foreign_session_tag) {
   DCHECK_NE(foreign_session_tag, current_machine_tag());
   DVLOG(1) << "Disassociating session " << foreign_session_tag;
-  return session_tracker_.DeleteSession(foreign_session_tag);
+  return session_tracker_.DeleteForeignSession(foreign_session_tag);
 }
 
 bool SessionsSyncManager::GetForeignSession(
@@ -961,66 +953,32 @@ bool SessionsSyncManager::GetForeignTab(const std::string& tag,
   return success;
 }
 
-void SessionsSyncManager::LocalTabDelegateToSpecifics(
-    const SyncedTabDelegate& tab_delegate,
-    sync_pb::SessionSpecifics* specifics) {
-  sessions::SessionTab* session_tab = nullptr;
-  session_tab = session_tracker_.GetTab(current_machine_tag(),
-                                        tab_delegate.GetSessionId(),
-                                        tab_delegate.GetSyncId());
-  SetSessionTabFromDelegate(tab_delegate, base::Time::Now(), session_tab);
-  SetVariationIds(session_tab);
-  sync_pb::SessionTab tab_s = session_tab->ToSyncData();
-  specifics->set_session_tag(current_machine_tag_);
-  specifics->set_tab_node_id(tab_delegate.GetSyncId());
-  specifics->mutable_tab()->CopyFrom(tab_s);
-}
-
 void SessionsSyncManager::AssociateRestoredPlaceholderTab(
     const SyncedTabDelegate& tab_delegate,
     SessionID::id_type new_tab_id,
     SessionID::id_type new_window_id,
-    const syncer::SyncDataList& restored_tabs,
     syncer::SyncChangeList* change_output) {
   DCHECK_NE(tab_delegate.GetSyncId(), TabNodePool::kInvalidTabNodeID);
-  // Rewrite the tab using |restored_tabs| to retrieve the specifics.
-  if (restored_tabs.empty()) {
-    DLOG(WARNING) << "Can't Update tab ID.";
-    return;
-  }
 
-  for (syncer::SyncDataList::const_iterator it = restored_tabs.begin();
-       it != restored_tabs.end(); ++it) {
-    if (it->GetSpecifics().session().tab_node_id() !=
-        tab_delegate.GetSyncId()) {
-      continue;
-    }
+  // Update tracker with the new association (and inform it of the tab node
+  // in the process).
+  session_tracker_.ReassociateLocalTab(tab_delegate.GetSyncId(), new_tab_id);
 
-    sync_pb::EntitySpecifics entity;
-    sync_pb::SessionSpecifics* specifics = entity.mutable_session();
-    specifics->CopyFrom(it->GetSpecifics().session());
-    DCHECK(specifics->has_tab());
+  // Update the window id on the SessionTab itself.
+  sessions::SessionTab* local_tab =
+      session_tracker_.GetTab(current_machine_tag(), new_tab_id);
+  local_tab->window_id.set_id(new_window_id);
 
-    // Update tab node pool with the new association.
-    local_tab_pool_.ReassociateTabNode(tab_delegate.GetSyncId(), new_tab_id);
-    TabLink* tab_link = new TabLink(tab_delegate.GetSyncId(), &tab_delegate);
-    local_tab_map_[new_tab_id] = make_linked_ptr<TabLink>(tab_link);
-
-    if (specifics->tab().tab_id() == new_tab_id &&
-        specifics->tab().window_id() == new_window_id)
-      return;
-
-    // Either the tab_id or window_id changed (e.g due to session restore), so
-    // update the sync node.
-    specifics->mutable_tab()->set_tab_id(new_tab_id);
-    specifics->mutable_tab()->set_window_id(new_window_id);
-    syncer::SyncData data = syncer::SyncData::CreateLocalData(
-        TabNodePool::TabIdToTag(current_machine_tag_, specifics->tab_node_id()),
-        current_session_name_, entity);
-    change_output->push_back(
-        syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
-    return;
-  }
+  // Rewrite the specifics based on the reassociated SessionTab to preserve
+  // the new tab and window ids.
+  sync_pb::EntitySpecifics entity;
+  entity.mutable_session()->CopyFrom(SessionTabToSpecifics(
+      *local_tab, current_machine_tag(), tab_delegate.GetSyncId()));
+  syncer::SyncData data = syncer::SyncData::CreateLocalData(
+      TabNodeIdToTag(current_machine_tag(), tab_delegate.GetSyncId()),
+      current_session_name_, entity);
+  change_output->push_back(
+      syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
 }
 
 // static