Do not submit: high level overview patch.

chrome/browser/sync/glue/session_model_associator.cc

Index: chrome/browser/sync/glue/session_model_associator.cc
diff --git a/chrome/browser/sync/glue/session_model_associator.cc b/chrome/browser/sync/glue/session_model_associator.cc
index 798db3a965c303ad16c5cbc22b4db6e9e4e55d31..67a6518dcc98fb7441a7528af8895bc5a52fcc43 100644
--- a/chrome/browser/sync/glue/session_model_associator.cc
+++ b/chrome/browser/sync/glue/session_model_associator.cc
@@ -143,6 +143,14 @@ bool SessionModelAssociator::InitSyncNodeFromChromeId(
   return false;
 }
 
+void SessionModelAssociator::NotifySyncIdGenerated(
+    const SyncedTabDelegate& tab) {
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_SESSION_SYNC_ID_GENERATED,
+      content::Source<SessionModelAssociator>(this),
+      content::Details<const SyncedTabDelegate>(&tab));
+}
+
 bool SessionModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) {
   DCHECK(CalledOnValidThread());
   CHECK(has_nodes);
@@ -202,6 +210,7 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
   header_s->set_device_type(DeviceInfo::GetLocalDeviceType());
 
   synced_session_tracker_.ResetSessionTracking(local_tag);
+  std::vector<int64> used_sync_ids;
   std::set<SyncedWindowDelegate*> windows =
       SyncedWindowDelegate::GetSyncedWindowDelegates();
   for (std::set<SyncedWindowDelegate*>::const_iterator i =
@@ -219,7 +228,7 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
                << (*i)->GetTabCount() << " tabs.";
       window_s.set_window_id(window_id);
       // Note: We don't bother to set selected tab index anymore. We still
-      // consume it when receiving foreign sessions, as reading it is free, but
+      // consume it when   foreign sessions, as reading it is free, but
       // it triggers too many sync cycles with too little value to make setting
       // it worthwhile.
       if ((*i)->IsTypeTabbed()) {
@@ -232,15 +241,26 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
 
       // Store the order of tabs.
       bool found_tabs = false;
+      std::vector<SessionID::id_type> used_tabs;
       for (int j = 0; j < (*i)->GetTabCount(); ++j) {
         SessionID::id_type tab_id = (*i)->GetTabIdAt(j);
+        SyncedTabDelegate* tab_delegate = (*i)->GetTabAt(j);
 
         if (reload_tabs) {
-          SyncedTabDelegate* tab = (*i)->GetTabAt(j);
+          if (tab_delegate && !tab_delegate->IsTabInMemory()) {
+            found_tabs = true;
+            // Update tab id since it might have changed.
+            UpdateTabIdForOldTab(tab_delegate->GetSyncSessionId(), tab_id);

[Nicolas Zea, 2013/05/16 22:59:03]

I don't think you should ever update the tab id. That's just sync's id for the
current tab node count at the time it was created. Multiple tabs, over the life
of a sync session, can use the tab sync node, but the tab id for that sync node
should never change. Perhaps I'm misunderstanding something?

[shashi, 2013/05/17 00:29:16]

When restore happens, the tab ids (different from tab_node_id) are
reinitialized.
I discovered this during prototyping, easiest way to explain this is by an
example:
Lets say before restore I had 3 tabs (A,B,C) with tab ids (1, 2, 3) after
restore the tab ids may be (20, 30, 10).
Suppose only tab A is in memory.
After tab association, the window_s will have tabs:
(2, 30, 10). We will rewrite the session node for A. 
So A will have tab_id=20 but since B and C are not in memory the session nodes
for them will be unchanged and will have 2 and 3 tab ids.

Remote client will see a window with tab ids: (20, 30, 10) but will not able to
make out the tab nodes corresponding to these tab ids.


On 2013/05/16 22:59:03, Nicolas Zea wrote:

+            window_s.add_tab(tab_id);
+            used_sync_ids.push_back(tab_delegate->GetSyncSessionId());
+            continue;
+          }
+
           // It's possible for GetTabAt to return a null tab if it's not in
           // memory. We can assume this means the tab already existed but hasn't
           // changed, so no need to reassociate.
-          if (tab && !AssociateTab(*tab, error)) {
+          if (tab_delegate && tab_delegate->IsTabInMemory() &&
+              !AssociateTab(*tab_delegate, error)) {
             // Association failed. Either we need to re-associate, or this is an
             // unrecoverable error.
             return false;
@@ -254,7 +274,9 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
         // the tab's presence in the tracker.
         const SessionTab* tab = NULL;
         if (synced_session_tracker_.LookupSessionTab(local_tag, tab_id, &tab)) {
+          used_tabs.push_back(tab_id);
           found_tabs = true;
+          used_sync_ids.push_back(tab_delegate->GetSyncSessionId());
           window_s.add_tab(tab_id);
         }
       }
@@ -276,6 +298,7 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
   }
   // Free memory for closed windows and tabs.
   synced_session_tracker_.CleanupSession(local_tag);
+  tab_pool_.PurgeOldSyncNodes(used_sync_ids);
 
   syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
   syncer::WriteNode header_node(&trans);
@@ -294,6 +317,22 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
   return true;
 }
 
+void SessionModelAssociator::UpdateTabIdForOldTab(
+    int64 sync_id,
+    SessionID::id_type new_tab_id) {
+  syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
+  syncer::WriteNode tab_node(&trans);
+  // Rewrite tab id if required.
+  if (tab_node.InitByIdLookup(sync_id) != syncer::BaseNode::INIT_OK) {
+    return;
+  }
+  sync_pb::SessionSpecifics session_specifics = tab_node.GetSessionSpecifics();
+  DCHECK(session_specifics.has_tab());
+  sync_pb::SessionTab* tab_s = session_specifics.mutable_tab();
+  tab_s->set_tab_id(new_tab_id);
+  tab_node.SetSessionSpecifics(session_specifics);
+}
+
 // Static.
 bool SessionModelAssociator::ShouldSyncWindow(
     const SyncedWindowDelegate* window) {
@@ -316,11 +355,11 @@ bool SessionModelAssociator::AssociateTabs(
   return true;
 }
 
-bool SessionModelAssociator::AssociateTab(const SyncedTabDelegate& tab,
+bool SessionModelAssociator::AssociateTab(SyncedTabDelegate& tab,
                                           syncer::SyncError* error) {
   DCHECK(CalledOnValidThread());
   int64 sync_id;
-  SessionID::id_type tab_id = tab.GetSessionId();
+  SessionID::id_type tab_id = tab.GetSessionId().id();
   if (tab.IsBeingDestroyed()) {
     // This tab is closing.
     TabLinksMap::iterator tab_iter = tab_map_.find(tab_id);
@@ -339,8 +378,17 @@ bool SessionModelAssociator::AssociateTab(const SyncedTabDelegate& tab,
   TabLinksMap::iterator tab_map_iter = tab_map_.find(tab_id);
   TabLink* tab_link = NULL;
   if (tab_map_iter == tab_map_.end()) {
-    // This is a new tab, get a sync node for it.
-    sync_id = tab_pool_.GetFreeTabNode();
+    // This is a new tab, get a sync node for it. Check if there is an old sync
+    // node for it.
+    sync_id = tab_pool_.GetOldSyncNode(tab.GetSyncSessionId());
+    if (sync_id < 0) {
+      sync_id = tab_pool_.GetFreeTabNode();
+    }
+    if (tab.GetSyncSessionId() != sync_id) {
+      tab.SetSyncSessionId(sync_id);
+      NotifySyncIdGenerated(tab);

[Nicolas Zea, 2013/05/16 22:59:03]

would it be better to have the equivalency check and the notify call part of the
SetSyncSessionId method?

[shashi, 2013/05/17 00:29:16]

Yes, I can replace it with an equivalency check, any maybe generate notification
directly from SetSyncSessionId.

On 2013/05/16 22:59:03, Nicolas Zea wrote:

+    }
+
     if (sync_id == syncer::kInvalidId) {
       if (error) {
         *error = error_handler_->CreateAndUploadError(
@@ -363,7 +411,7 @@ bool SessionModelAssociator::AssociateTab(const SyncedTabDelegate& tab,
   DCHECK_NE(tab_link->sync_id(), syncer::kInvalidId);
 
   DVLOG(1) << "Reloading tab " << tab_id << " from window "
-           << tab.GetWindowId();
+           << tab.GetWindowId().id();
   return WriteTabContentsToSyncModel(tab_link, error);
 }
 
@@ -397,21 +445,21 @@ bool SessionModelAssociator::WriteTabContentsToSyncModel(
     TabLink* tab_link,
     syncer::SyncError* error) {
   DCHECK(CalledOnValidThread());
+  DCHECK(tab_link->tab());
   const SyncedTabDelegate& tab_delegate = *(tab_link->tab());
   int64 sync_id = tab_link->sync_id();
   GURL old_tab_url = tab_link->url();
 
   // Load the last stored version of this tab so we can compare changes. If this
   // is a new tab, session_tab will be a blank/newly created SessionTab object.
-  SessionTab* session_tab =
-      synced_session_tracker_.GetTab(GetCurrentMachineTag(),
-                                     tab_delegate.GetSessionId());
+  SessionTab* session_tab = synced_session_tracker_.GetTab(
+      GetCurrentMachineTag(), tab_delegate.GetSessionId().id());
 
   SetSessionTabFromDelegate(tab_delegate, base::Time::Now(), session_tab);
 
   const GURL new_url = GetCurrentVirtualURL(tab_delegate);
-  DVLOG(1) << "Local tab " << tab_delegate.GetSessionId()
-           << " now has URL " << new_url.spec();
+  VLOG(1) << "Local tab " << tab_delegate.GetSessionId().id() << " now has URL "
+          << new_url.spec();
 
   // Trigger the favicon load if needed. We do this before opening the write
   // transaction to avoid jank.
@@ -424,7 +472,6 @@ bool SessionModelAssociator::WriteTabContentsToSyncModel(
   // Update our last modified time.
   synced_session_tracker_.GetSession(GetCurrentMachineTag())->modified_time =
       base::Time::Now();
-
   syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
   syncer::WriteNode tab_node(&trans);
   if (tab_node.InitByIdLookup(sync_id) != syncer::BaseNode::INIT_OK) {
@@ -436,7 +483,6 @@ bool SessionModelAssociator::WriteTabContentsToSyncModel(
     }
     return false;
   }
-
   sync_pb::SessionTab tab_s = session_tab->ToSyncData();
   sync_pb::SessionSpecifics specifics = tab_node.GetSessionSpecifics();
   if (new_url == old_tab_url) {
@@ -449,7 +495,6 @@ bool SessionModelAssociator::WriteTabContentsToSyncModel(
   // Retain the base SessionSpecifics data (tag, tab_node_id, etc.), and just
   // write the new SessionTabSpecifics.
   specifics.mutable_tab()->CopyFrom(tab_s);
-
   // Write into the actual sync model.
   tab_node.SetSessionSpecifics(specifics);
 
@@ -462,12 +507,13 @@ void SessionModelAssociator::SetSessionTabFromDelegate(
     base::Time mtime,
     SessionTab* session_tab) {
   DCHECK(session_tab);
-  session_tab->window_id.set_id(tab_delegate.GetWindowId());
-  session_tab->tab_id.set_id(tab_delegate.GetSessionId());
+  session_tab->window_id.set_id(tab_delegate.GetWindowId().id());
+  session_tab->tab_id.set_id(tab_delegate.GetSessionId().id());
   session_tab->tab_visual_index = 0;
   session_tab->current_navigation_index = tab_delegate.GetCurrentEntryIndex();
   session_tab->pinned = tab_delegate.IsPinned();
   session_tab->extension_app_id = tab_delegate.GetExtensionAppId();
+  session_tab->sync_session_id = tab_delegate.GetSyncSessionId();
   session_tab->user_agent_override.clear();
   session_tab->timestamp = mtime;
   const int current_index = tab_delegate.GetCurrentEntryIndex();
@@ -757,7 +803,8 @@ bool SessionModelAssociator::UpdateAssociationsFromSyncModel(
         // TODO(zea): fix this once we add support for reassociating
         // pre-existing tabs with pre-existing tab nodes. We'll need to load
         // the tab_node_id and ensure the tab_pool_ keeps track of them.
-        sync_node.Tombstone();
+        if (specifics.has_tab())
+          tab_pool_.AddTabNode(id, specifics.tab_node_id());

[Nicolas Zea, 2013/05/16 22:59:03]

else tombstone? (it would be the above error case where more than one session
header was found or the node was just corrupted somehow)

[shashi, 2013/05/17 00:29:16]

Good point, I should  handle the case for multiple header nodes/corrupted nodes.
Will fix this.
On 2013/05/16 22:59:03, Nicolas Zea wrote:

       }
     }
     id = next_id;
@@ -1098,7 +1145,7 @@ bool SessionModelAssociator::IsValidTab(const SyncedTabDelegate& tab) const {
   }
   const SyncedWindowDelegate* window =
       SyncedWindowDelegate::FindSyncedWindowDelegateWithId(
-          tab.GetWindowId());
+          tab.GetWindowId().id());
   if (!window && !setup_for_test_)
     return false;
   return true;