[Sync] Fix Session's handling of windows.

chrome/browser/sync/glue/synced_session_tracker.h

Index: chrome/browser/sync/glue/synced_session_tracker.h
diff --git a/chrome/browser/sync/glue/synced_session_tracker.h b/chrome/browser/sync/glue/synced_session_tracker.h
index 5140f9000f6a48516346bc57341ce6a175dc2903..3d27a829e9c734da4ec94ee7a73775673ded9ad0 100644
--- a/chrome/browser/sync/glue/synced_session_tracker.h
+++ b/chrome/browser/sync/glue/synced_session_tracker.h
@@ -33,77 +33,146 @@ class SyncedSessionTracker {
   void SetLocalSessionTag(const std::string& local_session_tag);
 
   // Fill a preallocated vector with all foreign sessions we're tracking (skips
-  // the local session object).
+  // the local session object). SyncedSession ownership remains within the
+  // SyncedSessionTracker.
   // Returns true if we had foreign sessions to fill it with, false otherwise.
-  bool LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions);
+  bool LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions)

[akalin, 2011/10/01 01:54:14]

There's no need to differentiate between this returning false and this returning
true but giving an empty vector, right?  If so, I think it's cleaner to make it
return a vector directly.

From a cursory search, is this used outside of test?  If not, I'd like to suffix
it with ForTest.

[akalin, 2011/10/01 01:57:15]

Ah, this is used in the foreign_session_handler, of course.  Nevermind my
comments about appending ForTest.

+      const;
 
   // Attempts to look up the session windows associatd with the session given
-  // by |session_tag|.
+  // by |session_tag|. Ownership Of SessionWindows stays within the
+  // SyncedSessionTracker.
   // If lookup succeeds:
   // - Fills windows with the SessionWindow pointers, returns true.
   // Else
   // - Returns false.
   bool LookupSessionWindows(const std::string& session_tag,

[akalin, 2011/10/01 01:54:14]

Ditto

-                            std::vector<SessionWindow*>* windows);
+                            std::vector<const SessionWindow*>* windows) const;
 
   // Attempts to look up the tab associated with the given tag and tab id.
+  // Ownership of the SessionTab's remains within the SyncedSessionTracker.

[akalin, 2011/10/01 01:54:14]

no "'s" since there's only one

[Nicolas Zea, 2011/10/03 18:36:50]

Done.

   // If lookup succeeds:
   // - Sets tab to point to the SessionTab, and returns true.
   // Else
   // - Returns false, tab is set to NULL.
   bool LookupSessionTab(const std::string& session_tag,

[akalin, 2011/10/01 01:54:14]

what about returning the tab directly, and NULL if it doesn't exist?

[Nicolas Zea, 2011/10/03 18:36:50]

I kind of prefer being consistent with the other "Lookup*" methods above and
returning a bool the way they do.

                         SessionID::id_type tab_id,
-                        const SessionTab** tab);
+                        const SessionTab** tab) const;
 
-  // Returns a pointer to the SyncedSession object associated with session_tag.
-  // If none exists, creates one and returns its pointer.
+  // 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);

[akalin, 2011/10/01 01:54:14]

WIth my comments in the model associator file, I think this can be changed to:

const SyncedSession* GetSessionForTest(...) const;

and maybe a private helper function: "SyncedSession GetSession(...)" used by
PutWindowInSession.

 
   // 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 reset, all calls to PutWindowInSession and PutTabInWindow will denote
+  // that the requested windows and tabs are owned (by setting the boolean
+  // in their SessionWindowWrapper/SessionTabWrapper to true) and add them back
+  // to their session. The next call to CleanupSession(...) will delete those
+  // windows and tabs not owned.
+  void ResetSessionTracking(const std::string& session_tag);
+
+  // 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|, and markes the window as being owned. If none existed for

[akalin, 2011/10/01 01:54:14]

Specify that the session is created as needed also.  Also see comments in model
associator about making this function also set info from specifics.

[Nicolas Zea, 2011/10/03 18:36:50]

Done.

+  // that session, creates one. Ownership of the SessionWindow remains within
+  // the SyncedSessionTracker.
+  void PutWindowInSession(const std::string& session_tag,
+                          SessionID::id_type window_id);
+
+  // Adds the tab with id |tab_id| to the window |window_id|, and marks it as
+  // being owned. If none existed for that session, creates one. Ownership of
+  // the SessionTab remains within the SyncedSessionTracker.
+  void PutTabInWindow(const std::string& session_tag,

[akalin, 2011/10/01 01:54:14]

Would it make sense for PutTabInWindow to auto-create the window/session, too?

[Nicolas Zea, 2011/10/03 18:36:50]

The window/session should be created before this is called. I don't really see
any reason to call this without them having been made, at least at the moment.

+                      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 and
-  // returns its pointer.
-  // |has_window| determines if newly created tabs are added to the pool of
-  // orphaned tabs (those which can't be reached by traversing sessions).
-  SessionTab* GetSessionTab(const std::string& session_tag,
-                            SessionID::id_type tab_id,
-                            bool has_window);
+  // the session specified with |session_tag|. If none exists, creates one.
+  // Ownership of the SessionTab remains within the SyncedSessionTracker.
+  SessionTab* GetTab(const std::string& session_tag,
+                     SessionID::id_type tab_id);
 
   // Free the memory for all dynamically allocated objects and clear the
   // tracking structures.
-  void clear();
+  void Clear();
 
-  inline bool empty() {
+  bool Empty() const {
     return synced_tab_map_.empty() && synced_session_map_.empty();
   }
 
   // Includes both foreign sessions and the local session.
-  inline size_t num_synced_sessions() {
+  size_t num_synced_sessions() const {
     return synced_session_map_.size();
   }
 
   // Returns the number of tabs associated with the specified session tag.
-  inline size_t num_synced_tabs(const std::string& session_tag) {
-    if (synced_tab_map_.find(session_tag) != synced_tab_map_.end()) {
-      return synced_tab_map_[session_tag]->size();
+  size_t num_synced_tabs(const std::string& session_tag) const {
+    SyncedTabMap::const_iterator iter = synced_tab_map_.find(session_tag);
+    if (iter != synced_tab_map_.end()) {
+      return iter->second.size();
     } else {
       return 0;
     }
   }
  private:
-  // Datatypes for accessing session data.
-  typedef std::map<SessionID::id_type, SessionTab*> IDToSessionTabMap;
-  typedef std::map<std::string, IDToSessionTabMap*> SyncedTabMap;
+  // Datatypes for accessing session data. Neither of the *Wrappers actually
+  // have ownership of the Windows/Tabs, they just provide id-based access to
+  // them. The ownership remains within it's containing session (for windows and
+  // 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).
+  struct SessionTabWrapper {
+    SessionTabWrapper() : tab_ptr(NULL), owned(false) {}
+    SessionTabWrapper(SessionTab* tab_ptr, bool owned)
+        : tab_ptr(tab_ptr),
+          owned(owned) {}
+    SessionTab* tab_ptr;
+    bool owned;
+  };
+  typedef std::map<SessionID::id_type, SessionTabWrapper> IDToSessionTabMap;
+  typedef std::map<std::string, IDToSessionTabMap> SyncedTabMap;
+
+  struct SessionWindowWrapper {
+    SessionWindowWrapper() : window_ptr(NULL), owned(false) {}
+    SessionWindowWrapper(SessionWindow* window_ptr, bool owned)
+        : window_ptr(window_ptr),
+          owned(owned) {}
+    SessionWindow* window_ptr;
+    bool owned;
+  };
+  typedef std::map<SessionID::id_type, SessionWindowWrapper>
+      IDToSessionWindowMap;
+  typedef std::map<std::string, IDToSessionWindowMap> SyncedWindowMap;
+
   typedef std::map<std::string, SyncedSession*> SyncedSessionMap;
 
+  // Helper methods for deleting SessionWindows and SessionTabs without owners.
+  bool DeleteOldSessionWindowIfNecessary(SessionWindowWrapper window_wrapper);
+  bool DeleteOldSessionTabIfNecessary(SessionTabWrapper tab_wrapper);
+
   // Per client mapping of tab id's to their SessionTab objects.
   // Key: session tag.
-  // Value: Tab id to SessionTab map pointer.
+  // Value: Tab id to SessionTabWrapper map.
   SyncedTabMap synced_tab_map_;
 
+  // Per client mapping of the window id's to their SessionWindow objects.
+  // Key: session_tag
+  // Value: Window id to SessionWindowWrapper map.
+  SyncedWindowMap synced_window_map_;
+
   // Per client mapping synced session objects.
   // Key: session tag.
   // Value: SyncedSession object pointer.
@@ -111,7 +180,8 @@ class SyncedSessionTracker {
 
   // The set of tabs that we have seen, and created SessionTab objects for, but
   // have not yet mapped to SyncedSessions. These are temporarily orphaned
-  // tabs, and won't be deleted if we delete synced_session_map_.
+  // tabs, and won't be deleted if we delete synced_session_map_, but are still
+  // owned by the SyncedSessionTracker itself (and deleted on Clear()).
   std::set<SessionTab*> unmapped_tabs_;
 
   // The tag for this machine's local session, so we can distinguish the foreign