Reland v3 of Session refactor

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 35123d76a9ce54e67999f31071da0feac5128723..abdf16f80716b6bf400f2201552dd60984634693 100644
--- a/components/sync_sessions/synced_session_tracker.h
+++ b/components/sync_sessions/synced_session_tracker.h
@@ -40,8 +40,7 @@ class SyncedSessionTracker {
   explicit SyncedSessionTracker(SyncSessionsClient* sessions_client);
   ~SyncedSessionTracker();
 
-  // We track and distinguish the local session from foreign sessions.
-  void SetLocalSessionTag(const std::string& local_session_tag);
+  // **** Synced session/tab query methods. ****
 
   // Fill a preallocated vector with all foreign sessions we're tracking (skips
   // the local session object). SyncedSession ownership remains within the
@@ -51,6 +50,11 @@ class SyncedSessionTracker {
   bool LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions,
                                 SessionLookup lookup) const;
 
+  // Fills |tab_node_ids| with the tab node ids (see GetTab) for all the tabs*
+  // associated with the session having tag |session_tag|.
+  void LookupForeignTabNodeIds(const std::string& session_tag,
+                               std::set<int>* tab_node_ids) const;
+
   // Attempts to look up the session windows associatd with the session given
   // by |session_tag|. Ownership of SessionWindows stays within the
   // SyncedSessionTracker.
@@ -76,15 +80,13 @@ class SyncedSessionTracker {
   // this won't create-if-not-present.
   bool LookupLocalSession(const SyncedSession** output) const;
 
+  // **** Methods for manipulating synced sessions and tabs. ****
+
   // Returns a pointer to the SyncedSession object associated with
   // |session_tag|. If none exists, creates one. Ownership of the
   // SyncedSession remains within the SyncedSessionTracker.
   SyncedSession* GetSession(const std::string& session_tag);
 
-  // Deletes the session associated with |session_tag| if it exists.
-  // Returns true if the session existed and was deleted, false otherwise.
-  bool DeleteSession(const std::string& session_tag);
-
   // Resets the tracking information for the session specified by |session_tag|.
   // This involves clearing all the windows and tabs from the session, while
   // keeping pointers saved in the synced_window_map_ and synced_tab_map_. Once
@@ -94,19 +96,6 @@ class SyncedSessionTracker {
   // tabs not owned.
   void ResetSessionTracking(const std::string& session_tag);
 
-  // Tracks the deletion of a foreign tab by removing the given |tab_node_id|
-  // from the parent session. 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(...).
-  void CleanupSession(const std::string& session_tag);
-
   // Adds the window with id |window_id| to the session specified by
   // |session_tag|. If none existed for that session, creates one. Similarly, if
   // the session did not exist yet, creates it. Ownership of the SessionWindow
@@ -122,21 +111,70 @@ class SyncedSessionTracker {
   // ensure we having mapping information for this session.
   void PutTabInWindow(const std::string& session_tag,
                       SessionID::id_type window_id,
-                      SessionID::id_type tab_id,
-                      size_t tab_index);
-
-  // Returns a pointer to the SessionTab object associated with |tab_id| for
-  // the session specified with |session_tag|. If none exists, creates one.
-  // Ownership of the SessionTab remains within the SyncedSessionTracker.
-  // |tab_node_id| must be a valid node id for the node backing this tab.
+                      SessionID::id_type tab_id);
+
+  // Adds |tab_node_id| to the session specified by |session_tag|, creating that
+  // session if necessary. This is necessary to ensure that each session has an
+  // up to date list of tab nodes linked to it for session deletion purposes.
+  // Note that this won't update the local tab pool, even if the local session
+  // tag is passed. The tab pool is only updated with new tab nodes when they're
+  // associated with a tab id (see ReassociateLocalTabNode or GetTabNodeForTab).
+  void OnTabNodeSeen(const std::string& session_tag, int tab_node_id);
+
+  // Returns a pointer to the SessionTab object associated with
+  // |tab_id| for the session specified with |session_tag|.
+  // Note: Ownership of the SessionTab remains within the SyncedSessionTracker.
+  // TODO(zea): Replace SessionTab with a Sync specific wrapper.
+  // crbug.com/662597
   sessions::SessionTab* GetTab(const std::string& session_tag,
-                               SessionID::id_type tab_id,
-                               int tab_node_id);
+                               SessionID::id_type tab_id);
 
-  // Fills |tab_node_ids| with the tab node ids (see GetTab) for all the tabs*
-  // associated with the session having tag |session_tag|.
-  void LookupTabNodeIds(const std::string& session_tag,
-                        std::set<int>* tab_node_ids);
+  // **** Methods specific to foreign sessions. ****
+
+  // Tracks the deletion of a foreign tab by removing the given |tab_node_id|
+  // from the parent session. 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 the session associated with |session_tag| if it exists.
+  // Returns true if the session existed and was deleted, false otherwise.
+  bool DeleteForeignSession(const std::string& session_tag);
+
+  // Deletes those windows and tabs associated with |session_tag| that are no
+  // longer owned. See ResetSessionTracking(...)..
+  void CleanupForeignSession(const std::string& session_tag);
+
+  // **** Methods specific to the local session. ****
+
+  // Set the local session tag. Must be called before any other local session
+  // methods are invoked.
+  void SetLocalSessionTag(const std::string& local_session_tag);
+
+  // Similar to CleanupForeignSession, but also marks any unmapped tabs as free
+  // in the tab node pool and fills |deleted_node_ids| with the set of locally
+  // free tab nodes to be deleted.
+  void CleanupLocalTabs(std::set<int>* deleted_node_ids);
+
+  // Fills |tab_node_id| with a tab node for |tab_id|. Returns true if an
+  // existing tab node was found, false if there was none and one had to be
+  // created.
+  bool GetTabNodeForLocalTab(int tab_id, int* tab_node_id);
+
+  // Returns whether |tab_node_id| refers to a tab node that's already been
+  // seen.
+  bool IsLocalTabNodeValid(int tab_node_id);

[skym, 2017/01/24 20:46:19]

"Valid" is kind of confusing. Why is it invalid if it hasn't been seen? Our
kInvalidXxId constants use a different definition of valid, it would seem. Maybe
one of seen/stale/fresh would be better?

Also, should this thing get unit tests?

[Nicolas Zea, 2017/01/25 23:55:05]

Updated to IsLocalTabNodeAssociated

+
+  // Reassociates the tab denoted by |tab_node_id| with a new tab id, preserving
+  // any previous SessionTab object the node was associated with. This is useful
+  // on restart when sync needs to reassociate tabs from a previous session with
+  // newly restored tabs (and can be used in conjunction with PutTabInWindow).
+  void ReassociateLocalTab(int tab_node_id, SessionID::id_type new_tab_id);
+
+  // **** Methods for querying/manipulating overall state ****.
 
   // Free the memory for all dynamically allocated objects and clear the
   // tracking structures.
@@ -160,20 +198,21 @@ class SyncedSessionTracker {
   }
 
  private:
-  // Implementation for GetTab(...) above, permits invalid tab_node_id.
-  sessions::SessionTab* GetTabImpl(const std::string& session_tag,
-                                   SessionID::id_type tab_id,
-                                   int tab_node_id);
+  friend class SessionsSyncManagerTest;
+  friend class SyncedSessionTrackerTest;
+
+  // Implementation of CleanupForeignSession/CleanupLocalTabs.
+  void CleanupSessionImpl(const std::string& session_tag);
 
   // The client of the sync sessions datatype.
   SyncSessionsClient* const sessions_client_;
 
-  // The mapping of tab/window ids to their SessionTab/SessionWindow objects.
+  // The mapping of tab/window to their SessionTab/SessionWindow objects.
   // The SessionTab/SessionWindow objects referred to may be owned either by the
   // session in the |synced_session_map_| or be temporarily unmapped and live in
   // the |unmapped_tabs_|/|unmapped_windows_| collections.
   //
-  // Map: session tag -> (tab/window id -> SessionTab*/SessionWindow*)
+  // Map: session tag -> (tab/window -> SessionTab*/SessionWindow*)
   std::map<std::string, std::map<SessionID::id_type, sessions::SessionTab*>>
       synced_tab_map_;
   std::map<std::string, std::map<SessionID::id_type, sessions::SessionWindow*>>
@@ -203,6 +242,9 @@ class SyncedSessionTracker {
   // sessions.
   std::string local_session_tag_;
 
+  // Pool of used/available sync nodes associated with local tabs.
+  TabNodePool local_tab_pool_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncedSessionTracker);
 };