[Sync] Fix Session's handling of windows.

chrome/browser/sync/glue/session_model_associator.cc

 // 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.
 
 #include "chrome/browser/sync/glue/session_model_associator.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/sys_info.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sessions/session_id.h"
 #include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/sync/api/sync_error.h"
 #include "chrome/browser/sync/glue/synced_session.h"
 #include "chrome/browser/sync/glue/synced_tab_delegate.h"
 #include "chrome/browser/sync/glue/synced_window_delegate.h"
 #include "chrome/browser/sync/internal_api/read_node.h"
 #include "chrome/browser/sync/internal_api/read_transaction.h"
 #include "chrome/browser/sync/internal_api/write_node.h"
 #include "chrome/browser/sync/internal_api/write_transaction.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ skipping 96 matching lines @@
   NOTREACHED();
   return false;
 }
 
 void SessionModelAssociator::ReassociateWindows(bool reload_tabs) {
   DCHECK(CalledOnValidThread());
   std::string local_tag = GetCurrentMachineTag();
   sync_pb::SessionSpecifics specifics;
   specifics.set_session_tag(local_tag);
   sync_pb::SessionHeader* header_s = specifics.mutable_header();
   SyncedSession* current_session =

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

i think with the changes suggested below there's no need to get the session
explicitly

       synced_session_tracker_.GetSession(local_tag);
   header_s->set_client_name(current_session_name_);
 #if defined(OS_LINUX)
   header_s->set_device_type(sync_pb::SessionHeader_DeviceType_TYPE_LINUX);
 #elif defined(OS_MACOSX)
   header_s->set_device_type(sync_pb::SessionHeader_DeviceType_TYPE_MAC);
 #elif defined(OS_WIN)
   header_s->set_device_type(sync_pb::SessionHeader_DeviceType_TYPE_WIN);
 #elif defined(OS_CHROMEOS)
   header_s->set_device_type(sync_pb::SessionHeader_DeviceType_TYPE_CROS);
 #else
   header_s->set_device_type(sync_pb::SessionHeader_DeviceType_TYPE_OTHER);
 #endif
 
-  size_t window_num = 0;
+  synced_session_tracker_.ResetSessionTracking(local_tag);
   std::set<SyncedWindowDelegate*> windows =
       SyncedWindowDelegate::GetSyncedWindowDelegates();
-  current_session->windows.reserve(windows.size());
   for (std::set<SyncedWindowDelegate*>::const_iterator i =
        windows.begin(); i != windows.end(); ++i) {
     // Make sure the window has tabs and a viewable window. The viewable window
     // check is necessary because, for example, when a browser is closed the
     // destructor is not necessarily run immediately. This means its possible
     // for us to get a handle to a browser that is about to be removed. If
     // the tab count is 0 or the window is NULL, the browser is about to be
     // deleted, so we ignore it.
-    if (ShouldSyncWindow(*i) && (*i)->GetTabCount() &&
-        (*i)->HasWindow()) {
+    if (ShouldSyncWindow(*i) && (*i)->GetTabCount() && (*i)->HasWindow()) {
       sync_pb::SessionWindow window_s;
       SessionID::id_type window_id = (*i)->GetSessionId();
       VLOG(1) << "Reassociating window " << window_id << " with " <<
           (*i)->GetTabCount() << " tabs.";
       window_s.set_window_id(window_id);
       window_s.set_selected_tab_index((*i)->GetActiveIndex());
       if ((*i)->IsTypeTabbed()) {
         window_s.set_browser_type(
             sync_pb::SessionWindow_BrowserType_TYPE_TABBED);
       } else {
@@ skipping 13 matching lines @@
             ReassociateTab(*tab);
           }
         }
       }
       // Only add a window if it contains valid tabs.
       if (found_tabs) {
         sync_pb::SessionWindow* header_window = header_s->add_window();
         *header_window = window_s;
 
         // Update this window's representation in the synced session tracker.
-        if (window_num >= current_session->windows.size()) {
-          // This a new window, create it.
-          current_session->windows.push_back(new SessionWindow());
-        }
+        synced_session_tracker_.PutWindowInSession(local_tag, window_id);

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

see comment below about making PutWindowInSession also fill in info from
specifics

         PopulateSessionWindowFromSpecifics(
             local_tag,
             window_s,
             base::Time::Now(),
-            current_session->windows[window_num++],
+            current_session->windows[window_id],
             &synced_session_tracker_);
       }
     }
   }
+  // Free memory for closed windows and tabs.
+  synced_session_tracker_.CleanupSession(local_tag);
 
   sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
   sync_api::WriteNode header_node(&trans);
   if (!header_node.InitByIdLookup(local_session_syncid_)) {
     LOG(ERROR) << "Failed to load local session header node.";
     return;
   }
   header_node.SetSessionSpecifics(specifics);
   if (waiting_for_change_) QuitLoopForSubtleTesting();
 }
@@ skipping 110 matching lines @@
       sync_pb::TabNavigation* nav_s = tab_s->add_navigation();
       PopulateSessionSpecificsNavigation(&tab_nav, nav_s);
     }
   }
   tab_s->set_current_navigation_index(current_index);
 
   tab_node.SetSessionSpecifics(session_s);
 
   // Convert to a local representation and store in synced session tracker.
   SessionTab* session_tab =
-      synced_session_tracker_.GetSessionTab(GetCurrentMachineTag(),
-                                            tab_s->tab_id(),
-                                            false);
+      synced_session_tracker_.GetTab(GetCurrentMachineTag(),
+                                     tab_s->tab_id());
   PopulateSessionTabFromSpecifics(*tab_s,
                                   base::Time::Now(),
                                   session_tab);
   return true;
 }
 
 // Static
 // TODO(zea): perhaps sync state (scroll position, form entries, etc.) as well?
 // See http://crbug.com/67068.
 void SessionModelAssociator::PopulateSessionSpecificsNavigation(
@@ skipping 73 matching lines @@
 void SessionModelAssociator::Disassociate(int64 sync_id) {
   DCHECK(CalledOnValidThread());
   NOTIMPLEMENTED();
   // TODO(zea): we will need this once we support deleting foreign sessions.
 }
 
 bool SessionModelAssociator::AssociateModels(SyncError* error) {
   DCHECK(CalledOnValidThread());
 
   // Ensure that we disassociated properly, otherwise memory might leak.
-  DCHECK(synced_session_tracker_.empty());
+  DCHECK(synced_session_tracker_.Empty());
   DCHECK_EQ(0U, tab_pool_.capacity());
 
   local_session_syncid_ = sync_api::kInvalidId;
 
   // Read any available foreign sessions and load any session data we may have.
   // If we don't have any local session data in the db, create a header node.
   {
     sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
 
     sync_api::ReadNode root(&trans);
@@ skipping 36 matching lines @@
   // Check if anything has changed on the client side.
   UpdateSyncModelDataFromClient();
 
   VLOG(1) << "Session models associated.";
 
   return true;
 }
 
 bool SessionModelAssociator::DisassociateModels(SyncError* error) {
   DCHECK(CalledOnValidThread());
-  synced_session_tracker_.clear();
+  synced_session_tracker_.Clear();
   tab_map_.clear();
   tab_pool_.clear();
   local_session_syncid_ = sync_api::kInvalidId;
   current_machine_tag_ = "";
   current_session_name_ = "";
 
   // There is no local model stored with which to disassociate, just notify
   // foreign session handlers.
   NotificationService::current()->Notify(
       chrome::NOTIFICATION_FOREIGN_SESSION_DISABLED,
@@ skipping 126 matching lines @@
     return false;
 
   if (specifics.has_header()) {
     // Read in the header data for this foreign session.
     // Header data contains window information and ordered tab id's for each
     // window.
 
     // Load (or create) the SyncedSession object for this client.
     SyncedSession* foreign_session =
         synced_session_tracker_.GetSession(foreign_session_tag);
-
     const sync_pb::SessionHeader& header = specifics.header();
     PopulateSessionHeaderFromSpecifics(header, foreign_session);
-    foreign_session->windows.reserve(header.window_size());
-    VLOG(1) << "Associating " << foreign_session_tag << " with " <<
-        header.window_size() << " windows.";
-    size_t i;
-    for (i = 0; i < static_cast<size_t>(header.window_size()); ++i) {
-      if (i >= foreign_session->windows.size()) {
-        // This a new window, create it.
-        foreign_session->windows.push_back(new SessionWindow());
-      }
+
+    // Reset the tab/window tracking for this session (must do this before
+    // we start calling PutWindowInSessino and PutTabInWindow so that all

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

Sessino -> Session

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

Done.

+    // unused tabs/windows get cleared by the CleanupSession(...) call).
+    synced_session_tracker_.ResetSessionTracking(foreign_session_tag);
+
+    // Process all the windows and their tab information.
+    int num_windows = header.window_size();
+    VLOG(1) << "Associating " << foreign_session_tag << " with "
+            << num_windows << " windows.";
+    for (int i = 0; i < num_windows; ++i) {
       const sync_pb::SessionWindow& window_s = header.window(i);
+      SessionID::id_type window_id = window_s.window_id();
+      synced_session_tracker_.PutWindowInSession(foreign_session_tag,
+                                                 window_id);
       PopulateSessionWindowFromSpecifics(foreign_session_tag,

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

I think the PopulateSessionWindowFromSpecifics logic more properly belongs in
the SyncedSessionTracker, merged with PutWindowInSession.  So it'd look like:

synced_session_tracker_.PutWindowInSession(foreign_session_tag, window_id,
window_s, modification_time);

                                          window_s,
                                          modification_time,
-                                         foreign_session->windows[i],
+                                         foreign_session->windows[window_id],
                                          &synced_session_tracker_);
     }
-    // Remove any remaining windows (in case windows were closed)
-    for (; i < foreign_session->windows.size(); ++i) {
-      delete foreign_session->windows[i];
-    }
-    foreign_session->windows.resize(header.window_size());
+
+    // Delete any closed windows and unused tabs as necessary.
+    synced_session_tracker_.CleanupSession(foreign_session_tag);
   } else if (specifics.has_tab()) {
     const sync_pb::SessionTab& tab_s = specifics.tab();
     SessionID::id_type tab_id = tab_s.tab_id();
     SessionTab* tab =

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

I think there's a similar need to rename GetTab to, say, PutWindowslessTab() and
have it take the tab specifics

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

But it's not necessarily windowless. It's possible the meta node already
processed it before the tab node arrived, in which case this will get and fill
the previously created SessionTab. 

-        synced_session_tracker_.GetSessionTab(foreign_session_tag,
-                                              tab_id,
-                                              false);
+        synced_session_tracker_.GetTab(foreign_session_tag, tab_id);
     PopulateSessionTabFromSpecifics(tab_s, modification_time, tab);
   } else {
     NOTREACHED();
     return false;
   }
 
   return true;
 }
 
 void SessionModelAssociator::DisassociateForeignSession(
@@ skipping 45 matching lines @@
     session_window->selected_tab_index = specifics.selected_tab_index();
   if (specifics.has_browser_type()) {
     if (specifics.browser_type() ==
         sync_pb::SessionWindow_BrowserType_TYPE_TABBED) {
       session_window->type = 1;
     } else {
       session_window->type = 2;
     }
   }
   session_window->timestamp = mtime;
-  session_window->tabs.resize(specifics.tab_size());
+  session_window->tabs.resize(specifics.tab_size(), NULL);
   for (int i = 0; i < specifics.tab_size(); i++) {
     SessionID::id_type tab_id = specifics.tab(i);
-    session_window->tabs[i] =
-        tracker->GetSessionTab(session_tag, tab_id, true);
+    tracker->PutTabInWindow(session_tag,
+                            session_window->window_id.id(),
+                            tab_id,
+                            i);
   }
 }
 
 // Static
 void SessionModelAssociator::PopulateSessionTabFromSpecifics(
     const sync_pb::SessionTab& specifics,
     const base::Time& mtime,
     SessionTab* tab) {
+  DCHECK_EQ(tab->tab_id.id(), specifics.tab_id());
   if (specifics.has_tab_id())
     tab->tab_id.set_id(specifics.tab_id());
   if (specifics.has_window_id())
     tab->window_id.set_id(specifics.window_id());
   if (specifics.has_tab_visual_index())
     tab->tab_visual_index = specifics.tab_visual_index();
   if (specifics.has_current_navigation_index())
     tab->current_navigation_index = specifics.current_navigation_index();
   if (specifics.has_pinned())
     tab->pinned = specifics.pinned();
@@ skipping 150 matching lines @@
     return tab_syncid_pool_[static_cast<size_t>(tab_pool_fp_--)];
   }
 }
 
 void SessionModelAssociator::TabNodePool::FreeTabNode(int64 sync_id) {
   // Pool size should always match # of free tab nodes.
   DCHECK_LT(tab_pool_fp_, static_cast<int64>(tab_syncid_pool_.size()));
   tab_syncid_pool_[static_cast<size_t>(++tab_pool_fp_)] = sync_id;
 }
 
 bool SessionModelAssociator::GetLocalSession(

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

This is needed for test only, right?  Append "ForTest, change to return a const
pointer directly, also see comment re. GetSession in SyncedSessionTracker

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

This and the methods below are used in (or are meant to be available to) the
foreign session handler and the session UI.

     const SyncedSession* * local_session) {
   DCHECK(CalledOnValidThread());
   if (current_machine_tag_.empty())
     return false;
-  *local_session =
-      synced_session_tracker_.GetSession(GetCurrentMachineTag());
+  *local_session = synced_session_tracker_.GetSession(GetCurrentMachineTag());
   return true;
 }
 
 bool SessionModelAssociator::GetAllForeignSessions(

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

append ForTest, return vector directly

     std::vector<const SyncedSession*>* sessions) {
   DCHECK(CalledOnValidThread());
   return synced_session_tracker_.LookupAllForeignSessions(sessions);
 }
 
 bool SessionModelAssociator::GetForeignSession(

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

append ForTest, return vector directly

     const std::string& tag,
-    std::vector<SessionWindow*>* windows) {
+    std::vector<const SessionWindow*>* windows) {
   DCHECK(CalledOnValidThread());
   return synced_session_tracker_.LookupSessionWindows(tag, windows);
 }
 
 bool SessionModelAssociator::GetForeignTab(
     const std::string& tag,
     const SessionID::id_type tab_id,
     const SessionTab** tab) {
   DCHECK(CalledOnValidThread());
   return synced_session_tracker_.LookupSessionTab(tag, tab_id, tab);
 }
 
-// Static
-bool SessionModelAssociator::SessionWindowHasNoTabsToSync(
-    const SessionWindow& window) {
-  int num_populated = 0;
-  for (std::vector<SessionTab*>::const_iterator i = window.tabs.begin();
-      i != window.tabs.end(); ++i) {
-    const SessionTab* tab = *i;
-    if (IsValidSessionTab(*tab))
-      num_populated++;
-  }
-  if (num_populated == 0)
-    return true;
-  return false;
-}
-
 // Valid local tab?
-bool SessionModelAssociator::IsValidTab(const SyncedTabDelegate& tab) {
+bool SessionModelAssociator::IsValidTab(const SyncedTabDelegate& tab) const {
   DCHECK(CalledOnValidThread());
   if ((tab.profile() == sync_service_->profile() ||
        sync_service_->profile() == NULL)) {          // For tests.
     const SyncedWindowDelegate* window =
         SyncedWindowDelegate::FindSyncedWindowDelegateWithId(
             tab.GetWindowId());
     if (!window)
       return false;
     const NavigationEntry* entry = tab.GetActiveEntry();
     if (!entry)
       return false;
     if (entry->virtual_url().is_valid() &&
         (entry->virtual_url().GetOrigin() != GURL(chrome::kChromeUINewTabURL) ||
          tab.GetEntryCount() > 1)) {
       return true;
     }
   }
   return false;
 }
 
-// Static.
-bool SessionModelAssociator::IsValidSessionTab(const SessionTab& tab) {
-  if (tab.navigations.empty())
-    return false;
-  int selected_index = tab.current_navigation_index;
-  selected_index = std::max(
-      0,
-      std::min(selected_index,
-          static_cast<int>(tab.navigations.size() - 1)));
-  if (selected_index == 0 &&
-      tab.navigations.size() == 1 &&
-      tab.navigations.at(selected_index).virtual_url().GetOrigin() ==
-          GURL(chrome::kChromeUINewTabURL)) {
-    // This is a new tab with no further history, skip.
-    return false;
-  }
-  return true;
-}
-
-
 void SessionModelAssociator::QuitLoopForSubtleTesting() {
   if (waiting_for_change_) {
     VLOG(1) << "Quitting MessageLoop for test.";
     waiting_for_change_ = false;
     test_method_factory_.RevokeAll();
     MessageLoop::current()->Quit();
   }
 }
 
 void SessionModelAssociator::BlockUntilLocalChangeForTest(
@@ skipping 176 matching lines @@
   char computer_name[MAX_COMPUTERNAME_LENGTH + 1];
   DWORD size = sizeof(computer_name);
   if (GetComputerNameA(computer_name, &size)) {
     return computer_name;
   }
   return std::string();
 }
 #endif
 
 }  // namespace browser_sync