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

components/sync_sessions/sessions_sync_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync_sessions/sessions_sync_manager.h"
 
 #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"
 #include "components/sync/model/sync_error.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/model/sync_merge_result.h"
 #include "components/sync/model/time.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 #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;
 using syncer::DeviceInfo;
 using syncer::LocalDeviceInfoProvider;
 using syncer::SyncChange;
 using syncer::SyncData;
 
 namespace sync_sessions {
 
@@ skipping 21 matching lines @@
   return t1->timestamp > t2->timestamp;
 }
 
 // Comparator function for use with std::sort that will sort sessions by
 // descending modified_time (i.e., most recent first).
 bool SessionsRecencyComparator(const SyncedSession* s1,
                                const SyncedSession* s2) {
   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
 // lifetime of SessionSyncManager.
 SessionsSyncManager::SessionsSyncManager(
     sync_sessions::SyncSessionsClient* sessions_client,
     syncer::SyncPrefs* sync_prefs,
     LocalDeviceInfoProvider* local_device,
     std::unique_ptr<LocalSessionEventRouter> router,
     const base::Closure& sessions_updated_callback,
@@ skipping 23 matching lines @@
   return machine_tag;
 }
 
 syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     std::unique_ptr<syncer::SyncChangeProcessor> sync_processor,
     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);
 
   // It's possible(via RebuildAssociations) for lost_navigations_recorder_ to
   // persist between sync being stopped and started. If it did persist, it's
   // already associated with |sync_processor|, so leave it alone.
   if (!lost_navigations_recorder_.get()) {
     lost_navigations_recorder_ =
         base::MakeUnique<sync_sessions::LostNavigationsRecorder>();
@@ skipping 14 matching lines @@
   DCHECK(local_device_);
   const DeviceInfo* local_device_info = local_device_->GetLocalDeviceInfo();
   if (local_device_info) {
     current_session_name_ = local_device_info->client_name();
   } else {
     merge_result.set_error(error_handler_->CreateAndUploadError(
         FROM_HERE, "Failed to get local device info."));
     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();
     base_specifics->set_session_tag(current_machine_tag());
     sync_pb::SessionHeader* header_s = base_specifics->mutable_header();
     header_s->set_client_name(current_session_name_);
     header_s->set_device_type(local_device_info->device_type());
     syncer::SyncData data = syncer::SyncData::CreateLocalData(
         current_machine_tag(), current_session_name_, specifics);
     new_changes.push_back(
         syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_ADD, data));
   }
 
 #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(
       sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes));
 
   local_event_router_->StartRoutingTo(this);
   return merge_result;
 }
 
 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;
   specifics.set_session_tag(local_tag);
   sync_pb::SessionHeader* header_s = specifics.mutable_header();
   SyncedSession* current_session = session_tracker_.GetSession(local_tag);
   current_session->modified_time = base::Time::Now();
   header_s->set_client_name(current_session_name_);
   // SessionDataTypeController ensures that the local device info
   // is available before activating this datatype.
@@ skipping 61 matching lines @@
 
         if (synced_tab->IsPlaceholderTab()) {
           // For tabs without WebContents update the |tab_id| and |window_id|,
           // as it could have changed after a session restore.
           // Note: We cannot check if a tab is valid if it has no WebContents.
           // We assume any such tab is valid and leave the contents of
           // corresponding sync node unchanged.
           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);
           }
           continue;
         }
 
         if (RELOAD_TABS == option)
           AssociateTab(synced_tab, change_output);
 
         // If the tab is valid, it would have been added to the tracker either
@@ skipping 12 matching lines @@
         *header_window = window_s;
 
         // Update this window's representation in the synced session tracker.
         session_tracker_.PutWindowInSession(local_tag, window_id);
         BuildSyncedSessionFromSpecifics(
             local_tag, window_s, current_session->modified_time,
             current_session->windows[window_id].get());
       }
     }
   }
-  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
   // not change).
   sync_pb::EntitySpecifics entity;
   entity.mutable_session()->CopyFrom(specifics);
   syncer::SyncData data = syncer::SyncData::CreateLocalData(
       current_machine_tag(), current_session_name_, entity);
   change_output->push_back(
       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();
+  DCHECK(!tab_delegate->IsPlaceholderTab());
 
-  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);
+  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;
+  SessionID::id_type tab_id = tab_delegate->GetSessionId();
+  DVLOG(1) << "Reloading tab " << tab_id << " from window "
+           << tab_delegate->GetWindowId();
 
-  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);
-  DVLOG(1) << "Reloading tab " << tab_id << " from window "
-           << tab->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);
 
-  // Write to sync model.
-  sync_pb::EntitySpecifics specifics;
-  LocalTabDelegateToSpecifics(*tab, specifics.mutable_session());
-  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));
+  // 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();
 
-  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));
-    page_revisit_broadcaster_.OnPageVisit(
-        new_url, tab->GetTransitionAtIndex(current_index));
-  }
-
+  // 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 the sync model itself.
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_session()->CopyFrom(
+      SessionTabToSpecifics(*session_tab, current_machine_tag(), tab_node_id));
+  syncer::SyncData data = syncer::SyncData::CreateLocalData(
+      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_delegate->GetTransitionAtIndex(current_index));
+  }
 }
 
 void SessionsSyncManager::RebuildAssociations() {
   syncer::SyncDataList data(sync_processor_->GetAllSyncData(syncer::SESSIONS));
   std::unique_ptr<syncer::SyncErrorFactory> error_handler(
       std::move(error_handler_));
   std::unique_ptr<syncer::SyncChangeProcessor> processor(
       std::move(sync_processor_));
 
   StopSyncing(syncer::SESSIONS);
@@ skipping 38 matching lines @@
     DCHECK(!local_tab_pool_out_of_sync_);
     return;
   }
 
   syncer::SyncChangeList changes;
   AssociateTab(modified_tab, &changes);
   // Note, we always associate windows because it's possible a tab became
   // "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) {
   local_event_router_->Stop();
   if (sync_processor_.get() && lost_navigations_recorder_.get()) {
     sync_processor_->RemoveLocalChangeObserver(
         lost_navigations_recorder_.get());
     lost_navigations_recorder_.reset();
   }
   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;
 }
 
 syncer::SyncDataList SessionsSyncManager::GetAllSyncData(
     syncer::ModelType type) const {
   syncer::SyncDataList list;
   const SyncedSession* session = nullptr;
   if (!session_tracker_.LookupLocalSession(&session))
     return syncer::SyncDataList();
 
   // First construct the header node.
   sync_pb::EntitySpecifics header_entity;
   header_entity.mutable_session()->set_session_tag(current_machine_tag());
   sync_pb::SessionHeader* header_specifics =
       header_entity.mutable_session()->mutable_header();
   header_specifics->MergeFrom(session->ToSessionHeader());
   syncer::SyncData data = syncer::SyncData::CreateLocalData(
       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);
     }
   }
   return list;
 }
 
 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;
 }
 
 syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
   if (!sync_processor_.get()) {
     syncer::SyncError error(FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
@@ skipping 39 matching lines @@
         break;
       case syncer::SyncChange::ACTION_ADD:
       case syncer::SyncChange::ACTION_UPDATE:
         if (current_machine_tag() == session.session_tag()) {
           // We should only ever receive a change to our own machine's session
           // info if encryption was turned on. In that case, the data is still
           // the same, so we can ignore.
           LOG(WARNING) << "Dropping modification to local session.";
           return syncer::SyncError();
         }
-        UpdateTrackerWithForeignSession(
+        UpdateTrackerWithSpecifics(
             session, syncer::SyncDataRemote(it->sync_data()).GetModifiedTime());
         break;
       default:
         NOTREACHED() << "Processing sync changes failed, unknown change type.";
     }
   }
 
   if (!sessions_updated_callback_.is_null())
     sessions_updated_callback_.Run();
   return syncer::SyncError();
 }
 
 syncer::SyncChange SessionsSyncManager::TombstoneTab(
     const sync_pb::SessionSpecifics& tab) {
   if (!tab.has_tab_node_id()) {
     LOG(WARNING) << "Old sessions node without tab node id; can't tombstone.";
     return syncer::SyncChange();
   } else {
     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));
   }
 }
 
 bool SessionsSyncManager::GetAllForeignSessions(
     std::vector<const SyncedSession*>* sessions) {
   if (!session_tracker_.LookupAllForeignSessions(
           sessions, SyncedSessionTracker::PRESENTABLE))
     return false;
   std::sort(sessions->begin(), sessions->end(), SessionsRecencyComparator);
   return true;
 }
 
 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;
   for (syncer::SyncDataList::const_iterator it = sync_data.begin();
        it != sync_data.end(); ++it) {
     const syncer::SyncData& data = *it;
     DCHECK(data.GetSpecifics().has_session());
     syncer::SyncDataRemote remote(data);
     const sync_pb::SessionSpecifics& specifics = data.GetSpecifics().session();
     if (specifics.session_tag().empty() ||
         (specifics.has_tab() &&
          (!specifics.has_tab_node_id() || !specifics.tab().has_tab_id()))) {
       syncer::SyncChange tombstone(TombstoneTab(specifics));
       if (tombstone.IsValid())
         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
         // to that foreign data (like deletion through garbage collection) to
         // trigger a data type error because the tag looking mechanism fails. So
         // look for these and delete via remote SyncData, which uses a server id
         // lookup mechanism instead, see crbug.com/604657.
         bad_foreign_hash_count++;
         new_changes->push_back(
             syncer::SyncChange(FROM_HERE, SyncChange::ACTION_DELETE, remote));
       }
     } else {
       // This is previously stored local session information.
       if (specifics.has_header() && !found_current_header) {
         // This is our previous header node, reuse it.
         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 "
                        << "tag.";
           syncer::SyncChange tombstone(TombstoneTab(specifics));
           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());
         }
       }
     }
   }
 
   // Cleanup all foreign sessions, since orphaned tabs may have been added after
   // the header.
   std::vector<const SyncedSession*> sessions;
   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",
                            bad_foreign_hash_count);
 
   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);
 
     // If a favicon or favicon urls are present, load the URLs and visit
     // times into the in-memory favicon cache.
     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 << ".";
   }
 }
 
 void SessionsSyncManager::InitializeCurrentMachineTag() {
   DCHECK(current_machine_tag_.empty());
   std::string persisted_guid;
   persisted_guid = sync_prefs_->GetSyncSessionsGUID();
   if (!persisted_guid.empty()) {
     current_machine_tag_ = persisted_guid;
     DVLOG(1) << "Restoring persisted session sync guid: " << persisted_guid;
   } else {
     DCHECK(local_device_);
     std::string cache_guid = local_device_->GetLocalSyncCacheGUID();
     DCHECK(!cache_guid.empty());
     current_machine_tag_ = BuildMachineTag(cache_guid);
     DVLOG(1) << "Creating session sync guid: " << current_machine_tag_;
     sync_prefs_->SetSyncSessionsGUID(current_machine_tag_);
   }
-
-  local_tab_pool_.SetMachineTag(current_machine_tag_);
 }
 
 // static
 void SessionsSyncManager::PopulateSessionHeaderFromSpecifics(
     const sync_pb::SessionHeader& header_specifics,
     base::Time mtime,
     SyncedSession* session_header) {
   if (header_specifics.has_client_name())
     session_header->session_name = header_specifics.client_name();
   if (header_specifics.has_device_type()) {
@@ skipping 89 matching lines @@
 void SessionsSyncManager::DeleteForeignSessionInternal(
     const std::string& tag,
     syncer::SyncChangeList* change_output) {
   if (tag == current_machine_tag()) {
     LOG(ERROR) << "Attempting to delete local session. This is not currently "
                << "supported.";
     return;
   }
 
   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(
         syncer::SyncChange(FROM_HERE, SyncChange::ACTION_DELETE,
                            SyncData::CreateLocalDelete(tag, syncer::SESSIONS)));
   }
   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();
 }
 
 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(
     const std::string& tag,
     std::vector<const sessions::SessionWindow*>* windows) {
   return session_tracker_.LookupSessionWindows(tag, windows);
 }
 
 bool SessionsSyncManager::GetForeignSessionTabs(
     const std::string& tag,
@@ skipping 25 matching lines @@
 bool SessionsSyncManager::GetForeignTab(const std::string& tag,
                                         const SessionID::id_type tab_id,
                                         const sessions::SessionTab** tab) {
   const sessions::SessionTab* synced_tab = nullptr;
   bool success = session_tracker_.LookupSessionTab(tag, tab_id, &synced_tab);
   if (success)
     *tab = synced_tab;
   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
 void SessionsSyncManager::SetSessionTabFromDelegate(
     const SyncedTabDelegate& tab_delegate,
     base::Time mtime,
     sessions::SessionTab* session_tab) {
   DCHECK(session_tab);
   session_tab->window_id.set_id(tab_delegate.GetWindowId());
   session_tab->tab_id.set_id(tab_delegate.GetSessionId());
@@ skipping 98 matching lines @@
 }
 
 // static
 std::string SessionsSyncManager::TagHashFromSpecifics(
     const sync_pb::SessionSpecifics& specifics) {
   return syncer::GenerateSyncableHash(syncer::SESSIONS,
                                       TagFromSpecifics(specifics));
 }
 
 };  // namespace sync_sessions