[Sync] Fix Session's handling of windows.

chrome/browser/sync/glue/synced_session_tracker.h

 // Copyright (c) 2011 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.
 
 #ifndef CHROME_BROWSER_SYNC_GLUE_SYNCED_SESSION_TRACKER_H_
 #define CHROME_BROWSER_SYNC_GLUE_SYNCED_SESSION_TRACKER_H_
 #pragma once
 
 #include <map>
 #include <set>
 #include <string>
+#include <utility>

[akalin, 2011/09/29 21:59:55]

no need for this anymore

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_vector.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sync/glue/synced_session.h"
 
 namespace browser_sync {
 
 // Class to manage synced sessions. The tracker will own all SyncedSession
 // and SessionTab objects it creates, and deletes them appropriately on
 // destruction.
 // Note: SyncedSession objects are created for all synced sessions, including
 // the local session (whose tag we maintain separately).
 class SyncedSessionTracker {
  public:
   SyncedSessionTracker();
   ~SyncedSessionTracker();
 
   // We track and distinguish the local session from foreign sessions.
   void SetLocalSessionTag(const std::string& local_session_tag);
 
   // Fill a preallocated vector with all foreign sessions we're tracking (skips
   // the local session object).
   // Returns true if we had foreign sessions to fill it with, false otherwise.

[akalin, 2011/09/29 21:59:55]

Comment that ownership of the SyncedSessions stays with the SST.

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

   bool LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions);

[akalin, 2011/09/29 21:59:55]

make const?

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

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

[akalin, 2011/09/29 21:59:55]

Comment that ownership of the SessionWindows stays with the SST.

[akalin, 2011/09/29 21:59:55]

make const?

also, do the returned SessionWindow pointers have to be const?

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

                             std::vector<SessionWindow*>* windows);
 
   // Attempts to look up the tab associated with the given tag and tab id.
   // If lookup succeeds:
   // - Sets tab to point to the SessionTab, and returns true.

[akalin, 2011/09/29 21:59:55]

Comment that ownership of the SessionTab stays with the SST.

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

   // Else
   // - Returns false, tab is set to NULL.
   bool LookupSessionTab(const std::string& session_tag,
                         SessionID::id_type tab_id,
                         const SessionTab** tab);

[akalin, 2011/09/29 21:59:55]

make const?

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

 
-  // 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.

[akalin, 2011/09/29 21:59:55]

Given the changes below, is this function needed anymore?  Can it be changed to
simply:

void PutSession(...);

That creates the session if it's not already created?

[Nicolas Zea, 2011/09/30 22:52:59]

We still need to get the session itself to set some of it's values (name and
device type).

   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|.
+  // Once reset, all calls to GetWindow and GetTabForWindow will denote
+  // that the requested windows and tabs are in use (by setting the boolean
+  // in their SessionWindowWrapper/SessionTabWrapper to true). The next call to
+  // CleanupSession(...) will delete those windows and tabs not in use.
+  void ResetSessionTracking(const std::string& session_tag);
+
+  // Deletes those windows and tabs associated with |session_tag| that are no
+  // longer in use from memory (and removes unused windows from their
+  // SyncedSession object).
+  // See ResetSessionTracking(...).
+  void CleanupSession(const std::string& session_tag);
+
+  // Returns a pointer to the SessionWindow object associated with |window_id|
+  // for the session specified with |session_tag|. If none exists, creates one.
+  SessionWindow* GetWindow(const std::string& session_tag,

[akalin, 2011/09/29 21:59:55]

Change this to something like:

PutWindowInSession(session_tag, window_id);

and do the
foreign_session->windows[window_id] = window_ptr;
dance inside

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

+                           SessionID::id_type window_id);
+
+  // Same as GetTab, but internally notes that window |window_id| is now
+  // holding tab |tab_id|. As a result, when CleanupSession(...) is called,
+  // the tab will not be destroyed.
+  SessionTab* GetTabForWindow(const std::string& session_tag,

[akalin, 2011/09/29 21:59:55]

Similarly, change this to something like:

PutTabInWindow(session_tag, window_id, tab_id);

and do the:
window->tabs[tab_id] = tab_ptr;
dance inside

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

+                              SessionID::id_type window_id,
+                              SessionID::id_type tab_id);
+
   // 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.
+  SessionTab* GetTab(const std::string& session_tag,

[akalin, 2011/09/29 21:59:55]

Comment that ownership of the SessionTab stays with its owning window/SST.

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

+                     SessionID::id_type tab_id);
 
   // Free the memory for all dynamically allocated objects and clear the
   // tracking structures.
   void clear();

[akalin, 2011/09/29 21:59:55]

nit: Clear() is Google style

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

 
   inline bool empty() {

[akalin, 2011/09/29 21:59:55]

nit: -inline, make const (if it's in the class definition, it's already inline)

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

     return synced_tab_map_.empty() && synced_session_map_.empty();
   }
 
   // Includes both foreign sessions and the local session.
   inline size_t num_synced_sessions() {

[akalin, 2011/09/29 21:59:55]

make const, -inline

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

     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) {

[akalin, 2011/09/29 21:59:55]

make const

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

     if (synced_tab_map_.find(session_tag) != synced_tab_map_.end()) {

[akalin, 2011/09/29 21:59:55]

probably need to store the iterator in a var and deref it to keep constness

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

-      return synced_tab_map_[session_tag]->size();
+      return synced_tab_map_[session_tag].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;
+  // Note, we pair pointers with bools so that we can track what is in use and
+  // what can be deleted (see ResetSessionTracking(..) and CleanupSession(..)
+  // above).

[akalin, 2011/09/29 21:59:55]

comment that SessionTabWrapper doesn't take ownership; the tab is owned either
by its contained window or by unmapped_tabs_

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

+  struct SessionTabWrapper {
+    SessionTabWrapper() : tab_ptr(NULL), used(false) {}
+    SessionTabWrapper(SessionTab* tab_ptr, bool used)
+        : tab_ptr(tab_ptr),
+          used(used) {}
+    SessionTab* tab_ptr;
+    bool used;
+  };
+  typedef std::map<SessionID::id_type, SessionTabWrapper> IDToSessionTabMap;
+  typedef std::map<std::string, IDToSessionTabMap> SyncedTabMap;
+
+  struct SessionWindowWrapper {

[akalin, 2011/09/29 21:59:55]

comment that SessionWindowWrapper doesn't take ownership; the window is owned by
its containing session

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

+    SessionWindowWrapper() : window_ptr(NULL), used(false) {}

[akalin, 2011/09/29 21:59:55]

used -> owner?

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

+    SessionWindowWrapper(SessionWindow* window_ptr, bool used)
+        : window_ptr(window_ptr),
+          used(used) {}
+    SessionWindow* window_ptr;
+    bool used;
+  };
+  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.
+  bool ClearUnusedSessionWindow(SessionWindowWrapper window_wrapper,
+                                SyncedSession* session);
+  bool ClearUnusedSessionTab(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.
   SyncedSessionMap synced_session_map_;
 
   // 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_.

[akalin, 2011/09/29 21:59:55]

say that these are owned by the SST itself

[Nicolas Zea, 2011/09/30 22:52:59]

Done.

   std::set<SessionTab*> unmapped_tabs_;
 
   // The tag for this machine's local session, so we can distinguish the foreign
   // sessions.
   std::string local_session_tag_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncedSessionTracker);
 };
 
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_GLUE_SYNCED_SESSION_TRACKER_H_