Changes session restore callback to always happen when tabs are created

There is no reason to differentiate between sync/async
case. Additionally we may get rid of loading entirely, in which case
the async case would have to change.

TEST=covered by tests
BUG=468458

Review URL: https://codereview.chromium.org/1022583003

Cr-Commit-Position: refs/heads/master@{#321582}

chrome/browser/sessions/session_restore.cc

@@ -111,11 +111,6 @@ class TabLoader : public content::NotificationObserver,
   // TabLoaderCallback:
   void SetTabLoadingEnabled(bool enable_tab_loading) override;
 
-  void set_on_session_restored_callbacks(
-      SessionRestore::CallbackList* callbacks) {
-    on_session_restored_callbacks_ = callbacks;
-  }
-
  private:
   friend class base::RefCounted<TabLoader>;
 
@@ -210,9 +205,6 @@ class TabLoader : public content::NotificationObserver,
   // Max number of tabs that were loaded in parallel (for metrics).
   size_t max_parallel_tab_loads_;
 
-  // Callback list for sending a session restore notification.
-  SessionRestore::CallbackList* on_session_restored_callbacks_;
-
   // For keeping TabLoader alive while it's loading even if no
   // SessionRestoreImpls reference it.
   scoped_refptr<TabLoader> this_retainer_;
@@ -283,15 +275,14 @@ void TabLoader::SetTabLoadingEnabled(bool enable_tab_loading) {
 
 TabLoader::TabLoader(base::TimeTicks restore_started)
     : memory_pressure_listener_(
-        base::Bind(&TabLoader::OnMemoryPressure, base::Unretained(this))),
+          base::Bind(&TabLoader::OnMemoryPressure, base::Unretained(this))),
       force_load_delay_multiplier_(1),
       loading_enabled_(true),
       got_first_foreground_load_(false),
       got_first_paint_(false),
       tab_count_(0),
       restore_started_(restore_started),
-      max_parallel_tab_loads_(0),
-      on_session_restored_callbacks_(nullptr) {
+      max_parallel_tab_loads_(0) {
 }
 
 TabLoader::~TabLoader() {
@@ -331,12 +322,6 @@ void TabLoader::LoadNextTab() {
 
   if (!tabs_to_load_.empty())
     StartTimer();
-
-  // When the session restore is done synchronously, notification is sent from
-  // SessionRestoreImpl::Restore .
-  if (tabs_to_load_.empty() && !SessionRestore::IsRestoringSynchronously()) {
-    on_session_restored_callbacks_->Notify(tab_count_);
-  }
 }
 
 void TabLoader::StartTimer() {
@@ -660,13 +645,8 @@ class SessionRestoreImpl : public content::NotificationObserver {
         loop.Run();
         quit_closure_for_sync_restore_ = base::Closure();
       }
-      // Count the total number of tabs in |windows_|.
-      int total_num_tabs = 0;
-      for (int i = 0; i < static_cast<int>(windows_.size()); ++i)
-        total_num_tabs += windows_[i]->tabs.size();
-
-      Browser* browser = ProcessSessionWindows(&windows_, active_window_id_);
-      on_session_restored_callbacks_->Notify(total_num_tabs);
+      Browser* browser =
+          ProcessSessionWindowsAndNotify(&windows_, active_window_id_);
       delete this;
       return browser;
     }
@@ -685,6 +665,7 @@ class SessionRestoreImpl : public content::NotificationObserver {
       std::vector<const sessions::SessionWindow*>::const_iterator end) {
     StartTabCreation();
     std::vector<Browser*> browsers;
+    std::vector<WebContents*> created_contents;
     // Create a browser instance to put the restored tabs in.
     for (std::vector<const sessions::SessionWindow*>::const_iterator i = begin;
          i != end; ++i) {
@@ -702,12 +683,16 @@ class SessionRestoreImpl : public content::NotificationObserver {
           std::min((*i)->selected_tab_index,
                    static_cast<int>((*i)->tabs.size()) - 1));
       RestoreTabsToBrowser(*(*i), browser, initial_tab_count,
-                           selected_tab_index);
+                           selected_tab_index, &created_contents);
       NotifySessionServiceOfRestoredTabs(browser, initial_tab_count);
     }
 
-    // Always create in a new window
+    // Always create in a new window.
     FinishedTabCreation(true, true);
+
+    on_session_restored_callbacks_->Notify(
+        static_cast<int>(created_contents.size()));
+
     return browsers;
   }
 
@@ -770,6 +755,9 @@ class SessionRestoreImpl : public content::NotificationObserver {
     // Since FinishedTabCreation() is not called here, |this| will leak if we
     // are not in sychronous mode.
     DCHECK(synchronous_);
+
+    on_session_restored_callbacks_->Notify(1);
+
     return web_contents;
   }
 
@@ -805,8 +793,6 @@ class SessionRestoreImpl : public content::NotificationObserver {
   // Invoked when beginning to create new tabs. Resets the |tab_loader_|.
   void StartTabCreation() {
     tab_loader_ = TabLoader::GetTabLoader(restore_started_);
-    tab_loader_->set_on_session_restored_callbacks(
-        on_session_restored_callbacks_);
   }
 
   // Invoked when done with creating all the tabs/browsers.
@@ -883,11 +869,22 @@ class SessionRestoreImpl : public content::NotificationObserver {
       return;
     }
 
-    ProcessSessionWindows(&windows.get(), active_window_id);
+    ProcessSessionWindowsAndNotify(&windows.get(), active_window_id);
+  }
+
+  Browser* ProcessSessionWindowsAndNotify(
+      std::vector<sessions::SessionWindow*>* windows,
+      SessionID::id_type active_window_id) {
+    std::vector<WebContents*> contents;
+    Browser* result =
+        ProcessSessionWindows(windows, active_window_id, &contents);
+    on_session_restored_callbacks_->Notify(static_cast<int>(contents.size()));
+    return result;
   }
 
   Browser* ProcessSessionWindows(std::vector<sessions::SessionWindow*>* windows,
-                                 SessionID::id_type active_window_id) {
+                                 SessionID::id_type active_window_id,
+                                 std::vector<WebContents*>* created_contents) {
     DVLOG(1) << "ProcessSessionWindows " << windows->size();
     base::TimeDelta time_to_process_sessions =
         base::TimeTicks::Now() - restore_started_;
@@ -984,7 +981,7 @@ class SessionRestoreImpl : public content::NotificationObserver {
         browser_to_activate = browser;
 
       RestoreTabsToBrowser(*(*i), browser, initial_tab_count,
-                           selected_tab_index);
+                           selected_tab_index, created_contents);
       NotifySessionServiceOfRestoredTabs(browser, initial_tab_count);
       // This needs to be done after restore because closing the last tab will
       // close the whole window.
@@ -1043,9 +1040,10 @@ class SessionRestoreImpl : public content::NotificationObserver {
   // If there are no existing tabs, the tab at |selected_tab_index| will be
   // selected. Otherwise, the tab selection will remain untouched.
   void RestoreTabsToBrowser(const sessions::SessionWindow& window,
-                           Browser* browser,
-                           int initial_tab_count,
-                           int selected_tab_index) {
+                            Browser* browser,
+                            int initial_tab_count,
+                            int selected_tab_index,
+                            std::vector<WebContents*>* created_contents) {
     DVLOG(1) << "RestoreTabsToBrowser " << window.tabs.size();
     DCHECK(!window.tabs.empty());
     if (initial_tab_count == 0) {
@@ -1062,6 +1060,8 @@ class SessionRestoreImpl : public content::NotificationObserver {
         if (!restored_tab)
           continue;
 
+        created_contents->push_back(restored_tab);
+
         // If this isn't the selected tab, there's nothing else to do.
         if (!is_selected_tab)
           continue;
@@ -1084,7 +1084,10 @@ class SessionRestoreImpl : public content::NotificationObserver {
       for (int i = 0; i < static_cast<int>(window.tabs.size()); ++i) {
         const sessions::SessionTab& tab = *(window.tabs[i]);
         // Always schedule loads as we will not be calling ShowBrowser().
-        RestoreTab(tab, tab_index_offset + i, browser, false);
+        WebContents* contents =
+            RestoreTab(tab, tab_index_offset + i, browser, false);
+        if (contents)
+          created_contents->push_back(contents);
       }
     }
   }

chrome/browser/sessions/session_restore.h

@@ -91,19 +91,10 @@ class SessionRestore {
   // Returns true if synchronously restoring a session.
   static bool IsRestoringSynchronously();
 
-  // Register callbacks for session restore events. These callbacks are stored
-  // in |on_session_restored_callbacks_|.
-  // The callback is supplied an integer arg representing a tab count. The exact
-  // meaning and timing depend upon the restore type:
-  // - SessionRestore::SYNCHRONOUS: the parameter is the number of tabs that
-  // were created. Additionally the callback is invoked immediately after the
-  // tabs have been created. That is, the tabs are not necessarily loading.
-  // - For all other restore types the parameter is the number of tabs that were
-  // restored and is sent after all tabs have started loading. Additionally if a
-  // request to restore tabs comes in while a previous request to restore tabs
-  // has not yet completed (loading tabs is throttled), then the callback is
-  // only notified once both sets of tabs have started loading and with the
-  // total number of tabs for both restores.
+  // Registers a callback that is notified every time session restore completes.
+  // Note that 'complete' means all the browsers and tabs have been created but
+  // have not necessarily finished loading. The integer supplied to the callback
+  // indicates the number of tabs that were created.
   static CallbackSubscription RegisterOnSessionRestoredCallback(
       const base::Callback<void(int)>& callback);
 

chrome/browser/sessions/session_restore_browsertest.cc

@@ -156,6 +156,10 @@ class SessionRestoreTest : public InProcessBrowserTest {
           base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL);
     }
     restore_observer.Wait();
+
+    if (no_memory_pressure)
+      WaitForTabsToLoad(new_browser);
+
     g_browser_process->ReleaseModule();
 
     return new_browser;
@@ -192,6 +196,15 @@ class SessionRestoreTest : public InProcessBrowserTest {
     return count;
   }
 
+  void WaitForTabsToLoad(Browser* browser) {
+    for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
+      content::WebContents* contents =
+          browser->tab_strip_model()->GetWebContentsAt(i);
+      contents->GetController().LoadIfNecessary();
+      content::WaitForLoadStop(contents);
+    }
+  }
+
   GURL url1_;
   GURL url2_;
   GURL url3_;