[Sync] Moved tab_node_id tracking to session object and improved foreign session garbage collection.

components/sync_sessions/synced_session_tracker.h

Index: components/sync_sessions/synced_session_tracker.h
diff --git a/components/sync_sessions/synced_session_tracker.h b/components/sync_sessions/synced_session_tracker.h
index a45a80ee8493bd47074b08ccdd6100f27c30541a..27d3bca1601cc18937c438d798d04c3d3aa2d1b2 100644
--- a/components/sync_sessions/synced_session_tracker.h
+++ b/components/sync_sessions/synced_session_tracker.h
@@ -40,10 +40,13 @@ class SyncedSessionTracker {
 
   // Fill a preallocated vector with all foreign sessions we're tracking (skips
   // the local session object). SyncedSession ownership remains within the
-  // SyncedSessionTracker.
+  // SyncedSessionTracker. Can specify if you want only presentable sessions,
+  // which means they have at least one window with at least one tab that has
+  // syncable content.
   // Returns true if we had foreign sessions to fill it with, false otherwise.
   bool LookupAllForeignSessions(
-      std::vector<const sync_driver::SyncedSession*>* sessions) const;
+      std::vector<const sync_driver::SyncedSession*>* sessions,
+      bool presentable) const;
 
   // Attempts to look up the session windows associatd with the session given
   // by |session_tag|. Ownership Of SessionWindows stays within the
@@ -71,7 +74,7 @@ class SyncedSessionTracker {
   bool LookupLocalSession(const sync_driver::SyncedSession** output) const;
 
   // Returns a pointer to the SyncedSession object associated with
-  // |session_tag|.  If none exists, creates one. Ownership of the
+  // |session_tag|. If none exists, creates one. Ownership of the
   // SyncedSession remains within the SyncedSessionTracker.
   sync_driver::SyncedSession* GetSession(const std::string& session_tag);
 
@@ -89,9 +92,16 @@ class SyncedSessionTracker {
   // windows and tabs not owned.
   void ResetSessionTracking(const std::string& session_tag);
 
+  // Tracks the deletion of a foreign tab by removing the given |tab_node_id|.
+  // Doesn't actually remove any tab objects because the header may have or may
+  // not have already been updated to no longer parent this tab. Regardless,
+  // when the header is updated then cleanup will remove the actual tab data.
+  // However, this method always needs to be called upon foreign tab deletion,
+  // otherwise LookupTabNodeIds(...) may return already deleted tab node ids.
+  void DeleteForeignTab(const std::string& session_tag, int tab_node_id);
+
   // Deletes those windows and tabs associated with |session_tag| that are no
   // longer owned.
-  // See ResetSessionTracking(...).

[Nicolas Zea, 2016/04/12 20:38:04]

Should this remain?

[skym, 2016/04/12 22:12:14]

Yes. Done.

   void CleanupSession(const std::string& session_tag);
 
   // Adds the window with id |window_id| to the session specified by
@@ -122,15 +132,7 @@ class SyncedSessionTracker {
 
   // Fills |tab_node_ids| with the tab node ids (see GetTab) for all the tabs*
   // associated with the session having tag |session_tag|.
-  // Returns false if we don't have any record of the session.  If no tabs were
-  // found as part of the session, the return value will be true but
-  // |tab_node_ids| will be empty.
-  //
-  // * - note that this only returns the ids we're aware of; it's possible we
-  // don't have the latest tab state from a foreign session and it's also
-  // possible we just haven't updated the tab_node_id for a tab yet, so the
-  // result list should not be treated as authoritative.
-  bool LookupTabNodeIds(const std::string& session_tag,
+  void LookupTabNodeIds(const std::string& session_tag,
                         std::set<int>* tab_node_ids);
 
   // Free the memory for all dynamically allocated objects and clear the
@@ -176,21 +178,13 @@ class SyncedSessionTracker {
   // mapped tabs, unmapped tabs are owned by the unmapped_tabs_ container).
   // Note, we pair pointers with bools so that we can track what is owned and
   // what can be deleted (see ResetSessionTracking(..) and CleanupSession(..)
-  // above).
-  // The wrappers also serve as a convenient place to augment state stored in
-  // SessionTab for sync purposes, such as |tab_node_id|.
-  // IsOwned is used as a wrapper constructor parameter for readability.
+  // above). IsOwned is used as a wrapper constructor parameter for readability.
   struct SessionTabWrapper {
-    SessionTabWrapper()
-        : tab_ptr(NULL),
-          owned(false),
-          tab_node_id(TabNodePool::kInvalidTabNodeID) {}
-    SessionTabWrapper(sessions::SessionTab* tab_ptr,
-                      OwnedState owned,
-                      int tab_node_id)
-        : tab_ptr(tab_ptr),
-          owned(owned == IS_OWNED),
-          tab_node_id(tab_node_id) {}
+    SessionTabWrapper() : tab_ptr(NULL), owned(false) {}
+
+    SessionTabWrapper(sessions::SessionTab* tab_ptr, OwnedState owned)
+        : tab_ptr(tab_ptr), owned(owned == IS_OWNED) {}
+
     sessions::SessionTab* tab_ptr;
 
     // This is used as part of a mark-and-sweep approach to garbage
@@ -198,11 +192,6 @@ class SyncedSessionTracker {
     // ResetSessionTracking will clear |owned| bits, and if it is not claimed
     // by a window by the time CleanupSession is called it will be deleted.
     bool owned;
-
-    // This lets us identify the sync node that is "backing" this tab in the
-    // sync model, whether it is part of a local or foreign session. The
-    // "tab node id" is described in session_specifics.proto.
-    int tab_node_id;
   };
   typedef std::map<SessionID::id_type, SessionTabWrapper> IDToSessionTabMap;
   typedef std::map<std::string, IDToSessionTabMap> SyncedTabMap;
@@ -221,8 +210,9 @@ class SyncedSessionTracker {
   typedef std::map<std::string, sync_driver::SyncedSession*> SyncedSessionMap;
 
   // Helper methods for deleting SessionWindows and SessionTabs without owners.
-  bool DeleteOldSessionWindowIfNecessary(SessionWindowWrapper window_wrapper);
-  bool DeleteOldSessionTabIfNecessary(SessionTabWrapper tab_wrapper);
+  bool DeleteOldSessionWindowIfNecessary(
+      const SessionWindowWrapper& window_wrapper);
+  bool DeleteOldSessionTabIfNecessary(const SessionTabWrapper& tab_wrapper);
 
   // Implementation for GetTab(...) above, permits invalid tab_node_id.
   sessions::SessionTab* GetTabImpl(const std::string& session_tag,