Fix: Prerendering was confusing SessionService to not save sessionStorage.

chrome/browser/sessions/session_service.cc

Index: chrome/browser/sessions/session_service.cc
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index eb406fcf225fc6110bc78af3b84e270b8ac66967..63c1292d391d74111c3446e471282be9a5e4ddaa 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -220,6 +220,11 @@ SessionService::SessionService(const FilePath& save_path)
 }
 
 SessionService::~SessionService() {
+  for (BrowserList::const_iterator iter = BrowserList::begin();
+       iter != BrowserList::end(); ++iter) {
+    OnBrowserRemoved(*iter);
+  }
+  BrowserList::RemoveObserver(this);
   Save();
 }
 
@@ -521,10 +526,6 @@ void SessionService::Save() {
 
 void SessionService::Init() {
   // Register for the notifications we're interested in.
-  registrar_.Add(this, chrome::NOTIFICATION_TAB_PARENTED,
-                 content::NotificationService::AllSources());
-  registrar_.Add(this, chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
-                 content::NotificationService::AllSources());
   registrar_.Add(this, content::NOTIFICATION_NAV_LIST_PRUNED,
                  content::NotificationService::AllSources());
   registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_CHANGED,
@@ -537,6 +538,11 @@ void SessionService::Init() {
   registrar_.Add(
       this, chrome::NOTIFICATION_TAB_CONTENTS_APPLICATION_EXTENSION_CHANGED,
       content::NotificationService::AllSources());
+  BrowserList::AddObserver(this);
+  for (BrowserList::const_iterator iter = BrowserList::begin();
+       iter != BrowserList::end(); ++iter) {
+    OnBrowserAdded(*iter);
+  }
 }
 
 bool SessionService::ShouldNewWindowStartSession() {
@@ -602,58 +608,6 @@ void SessionService::Observe(int type,
       break;
     }
 
-    case chrome::NOTIFICATION_TAB_PARENTED: {
-      WebContents* web_contents = content::Source<WebContents>(source).ptr();
-      if (web_contents->GetBrowserContext() != profile())
-        return;
-      SessionTabHelper* session_tab_helper =
-          SessionTabHelper::FromWebContents(web_contents);
-      SetTabWindow(session_tab_helper->window_id(),
-                   session_tab_helper->session_id());
-      extensions::TabHelper* extensions_tab_helper =
-          extensions::TabHelper::FromWebContents(web_contents);
-      if (extensions_tab_helper &&
-          extensions_tab_helper->extension_app()) {
-        SetTabExtensionAppID(
-            session_tab_helper->window_id(),
-            session_tab_helper->session_id(),
-            extensions_tab_helper->extension_app()->id());
-      }
-
-      // Record the association between the SessionStorageNamespace and the
-      // tab.
-      //
-      // TODO(ajwong): This should be processing the whole map rather than
-      // just the default. This in particular will not work for tabs with only
-      // isolated apps which won't have a default partition.
-      content::SessionStorageNamespace* session_storage_namespace =
-          web_contents->GetController().GetDefaultSessionStorageNamespace();
-      ScheduleCommand(CreateSessionStorageAssociatedCommand(
-          session_tab_helper->session_id(),
-          session_storage_namespace->persistent_id()));
-      session_storage_namespace->SetShouldPersist(true);
-      break;
-    }
-
-    case chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED: {
-      TabContents* tab = content::Source<TabContents>(source).ptr();
-      if (!tab || tab->profile() != profile())
-        return;
-      // Allow the associated sessionStorage to get deleted; it won't be needed
-      // in the session restore.
-      content::SessionStorageNamespace* session_storage_namespace =
-          tab->web_contents()->GetController().
-              GetDefaultSessionStorageNamespace();
-      session_storage_namespace->SetShouldPersist(false);
-      SessionTabHelper* session_tab_helper =
-          SessionTabHelper::FromWebContents(tab->web_contents());
-      TabClosed(session_tab_helper->window_id(),
-                session_tab_helper->session_id(),
-                tab->web_contents()->GetClosedByUserGesture());
-      RecordSessionUpdateHistogramData(type, &last_updated_tab_closed_time_);
-      break;
-    }
-
     case content::NOTIFICATION_NAV_LIST_PRUNED: {
       WebContents* web_contents =
           content::Source<content::NavigationController>(source).ptr()->
@@ -1763,3 +1717,77 @@ void SessionService::RecordUpdatedSaveTime(base::TimeDelta delta,
         50);
   }
 }
+
+void SessionService::OnBrowserAdded(Browser* browser) {

[sky, 2012/09/21 16:26:31]

Can we move the implementation of observing BrowserList and TabStripModel into a
separate class that invokes newly exposed methods on SessionService? That way
SessionService doesn't have as many dependencies.

[marja, 2012/09/24 09:23:09]

Ok, it seems that Browser already has this logic to monitor the tab creation,
and it was also calling SessionService funcs to notify it e.g., about window
closing, so I think notifying SessionService about tab creation / closing fits
in, too. (See the latest patch set.) Wdyt?

+  if (!profile() || !profile()->IsSameProfile(browser->profile()))
+    return;
+  browser->tab_strip_model()->AddObserver(this);
+}
+
+void SessionService::OnBrowserRemoved(Browser* browser) {
+  if (!profile() || !profile()->IsSameProfile(browser->profile()))
+    return;
+  browser->tab_strip_model()->RemoveObserver(this);
+}
+
+void SessionService::TabInsertedAt(TabContents* contents,
+                                   int index,
+                                   bool foreground) {
+  if (contents->profile() != profile())
+    return;
+  WebContents* web_contents = contents->web_contents();
+  SessionTabHelper* session_tab_helper =
+      SessionTabHelper::FromWebContents(web_contents);
+  SetTabWindow(session_tab_helper->window_id(),
+               session_tab_helper->session_id());
+  extensions::TabHelper* extensions_tab_helper =
+      extensions::TabHelper::FromWebContents(web_contents);
+  if (extensions_tab_helper &&
+      extensions_tab_helper->extension_app()) {
+    SetTabExtensionAppID(
+        session_tab_helper->window_id(),
+        session_tab_helper->session_id(),
+        extensions_tab_helper->extension_app()->id());
+  }
+
+  // Record the association between the SessionStorageNamespace and the
+  // tab.
+  //
+  // TODO(ajwong): This should be processing the whole map rather than
+  // just the default. This in particular will not work for tabs with only
+  // isolated apps which won't have a default partition.
+  content::SessionStorageNamespace* session_storage_namespace =
+      web_contents->GetController().GetDefaultSessionStorageNamespace();
+  ScheduleCommand(CreateSessionStorageAssociatedCommand(
+      session_tab_helper->session_id(),
+      session_storage_namespace->persistent_id()));
+  session_storage_namespace->SetShouldPersist(true);
+}
+
+void SessionService::TabClosingAt(TabStripModel* tab_strip_model,
+                                  TabContents* contents,
+                                  int index) {
+  if (!contents || contents->profile() != profile())
+    return;
+  WebContents* web_contents = contents->web_contents();
+  // Allow the associated sessionStorage to get deleted; it won't be needed
+  // in the session restore.
+  content::SessionStorageNamespace* session_storage_namespace =
+    web_contents->GetController().GetDefaultSessionStorageNamespace();
+  session_storage_namespace->SetShouldPersist(false);
+  SessionTabHelper* session_tab_helper =
+    SessionTabHelper::FromWebContents(web_contents);
+  TabClosed(session_tab_helper->window_id(),
+            session_tab_helper->session_id(),
+            web_contents->GetClosedByUserGesture());
+  RecordSessionUpdateHistogramData(chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
+                                   &last_updated_tab_closed_time_);
+}
+
+void SessionService::TabReplacedAt(TabStripModel* tab_strip_model,
+                                   TabContents* old_contents,
+                                   TabContents* new_contents,
+                                   int index) {
+  TabClosingAt(tab_strip_model, old_contents, index);
+  TabInsertedAt(new_contents, index, true /* foreground is not used */);
+}