[Session restore] Add MRU logic to loading of background pages.

chrome/browser/sessions/session_restore_browsertest.cc

 // Copyright 2012 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 <algorithm>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/memory/memory_pressure_listener.h"
 #include "base/process/launch.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 190 matching lines @@
     }
   }
 
   GURL url1_;
   GURL url2_;
   GURL url3_;
 
   const BrowserList* active_browser_list_;
 };
 
-// Activates the smart restore behaviour and can track the loading of tabs.
-class SmartSessionRestoreTest : public SessionRestoreTest,
-                                public content::NotificationObserver {
+// Activates the smart restore behaviour in "simple" mode and tracks the loading
+// of tabs.
+class SmartSessionRestoreSimpleTest : public SessionRestoreTest,
+                                      public content::NotificationObserver {
  public:
-  SmartSessionRestoreTest() {}
+  SmartSessionRestoreSimpleTest() {}
   void StartObserving(int num_tabs) {
+    // Start by clearing everything so it can be reused in the same test.
+    web_contents_.clear();
+    registrar_.RemoveAll();
     num_tabs_ = num_tabs;
     registrar_.Add(this, content::NOTIFICATION_LOAD_START,
                    content::NotificationService::AllSources());
   }
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override {
     switch (type) {
       case content::NOTIFICATION_LOAD_START: {
         content::NavigationController* controller =
@@ skipping 23 matching lines @@
         "IntelligentSessionRestore.TestGroup:PrioritizeTabs/simple");
   }
 
  private:
   content::NotificationRegistrar registrar_;
   // Ordered by load start order.
   std::vector<content::WebContents*> web_contents_;
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
   int num_tabs_;
 
-  DISALLOW_COPY_AND_ASSIGN(SmartSessionRestoreTest);
+  DISALLOW_COPY_AND_ASSIGN(SmartSessionRestoreSimpleTest);
+};
+
+class SmartSessionRestoreMRUTest : public SmartSessionRestoreSimpleTest {
+ public:
+  SmartSessionRestoreMRUTest() {}
+
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kForceFieldTrials, "IntelligentSessionRestore/TestGroup/");
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kForceFieldTrialParams,
+        "IntelligentSessionRestore.TestGroup:PrioritizeTabs/mru");

[sky, 2015/05/12 21:25:30]

Ouch. Is the old one dumb?

[Georges Khalil, 2015/05/15 16:55:30]

No comments :)

+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SmartSessionRestoreMRUTest);
 };
 
 // Verifies that restored tabs have a root window. This is important
 // otherwise the wrong information is communicated to the renderer.
 // (http://crbug.com/342672).
 IN_PROC_BROWSER_TEST_F(SessionRestoreTest, RestoredTabsShouldHaveWindow) {
   // Create tabs.
   ui_test_utils::NavigateToURLWithDisposition(
       browser(),
       GURL(url::kAboutBlankURL),
@@ skipping 1051 matching lines @@
       &browser()->tab_strip_model()->GetActiveWebContents()->GetController();
   EXPECT_TRUE(
       controller->GetDefaultSessionStorageNamespace()->should_persist());
 
   // Quit and restore. Check that no extra tabs were created.
   Browser* new_browser = QuitBrowserAndRestore(browser(), 1);
   ASSERT_EQ(1u, active_browser_list_->size());
   EXPECT_EQ(1, new_browser->tab_strip_model()->count());
 }
 
-IN_PROC_BROWSER_TEST_F(SmartSessionRestoreTest, CorrectLoadingOrder) {
+IN_PROC_BROWSER_TEST_F(SmartSessionRestoreSimpleTest, CorrectLoadingOrder) {
   ASSERT_EQ(SessionRestore::SMART_RESTORE_MODE_SIMPLE,
             SessionRestore::GetSmartRestoreMode());
 
   const int NumTabs = 6;
 
   // Start observing the loading of tabs, to make sure the order is correct.
   StartObserving(NumTabs);
 
   struct TabInfo {
     GURL url;
@@ skipping 25 matching lines @@
     sync_data.set_tab_visual_index(0);
     sync_data.set_current_navigation_index(0);
     sync_data.add_navigation()->CopyFrom(nav.ToSyncData());
     sync_data.set_pinned(tab_info[i].pinned);
     tab[i].SetFromSyncData(sync_data, base::Time::Now());
     window.tabs.push_back(tab + i);
   }
 
   session.push_back(&window);
   Profile* profile = browser()->profile();
-  ui_test_utils::BrowserAddedObserver window_observer;
   std::vector<Browser*> browsers = SessionRestore::RestoreForeignSessionWindows(
       profile, browser()->host_desktop_type(), session.begin(), session.end());
 
   ASSERT_EQ(1u, browsers.size());
   ASSERT_TRUE(browsers[0]);
   ASSERT_EQ(NumTabs, browsers[0]->tab_strip_model()->count());
 
   WaitForAllTabsToStartLoading();
 
   ASSERT_EQ(static_cast<size_t>(NumTabs), web_contents().size());
 
   // Make sure that contents are loaded in the correct order, ie. each tab rank
   // is higher that its preceding one.
   std::map<GURL, int> ranks;
   for (auto t : tab_info)
     ranks[t.url] = t.expected_load_order;
   for (size_t i = 1; i < web_contents().size(); i++) {
     int current_rank = ranks[web_contents()[i]->GetLastCommittedURL()];
     int previous_rank = ranks[web_contents()[i - 1]->GetLastCommittedURL()];
     ASSERT_LT(previous_rank, current_rank);
   }
 
   // The SessionWindow destructor deletes the tabs, so we have to clear them
   // here to avoid a crash.
   window.tabs.clear();
 }
+
+IN_PROC_BROWSER_TEST_F(SmartSessionRestoreMRUTest, CorrectLoadingOrder) {
+  const int NumTabs = 6;

[sky, 2015/05/12 21:25:30]

num_tabs

[Georges Khalil, 2015/05/15 16:55:30]

Oops, done. Fixed the other test too.

+
+  GURL urls[] = {GURL("http://google.com/1"),
+                 GURL("http://google.com/2"),
+                 GURL("http://google.com/3"),
+                 GURL("http://google.com/4"),
+                 GURL("http://google.com/5"),
+                 GURL("http://google.com/6")};
+
+  // Create a random activation order so that running the test enough times
+  // gives us complete coverage.
+  std::vector<int> activation_order;
+  for (int i = 0; i < NumTabs; i++)
+    activation_order.push_back(i);
+  std::random_shuffle(activation_order.begin(), activation_order.end());

[sky, 2015/05/12 21:25:30]

It's not a good idea to randomize test data. If there is flake then it can be
hard to repro. Much better if you order it in a specific way. Do multiple runs
of the test if appropriate.

[Georges Khalil, 2015/05/15 16:55:30]

Acknowledged. Changed to a fixed order.

+
+  // Replace the first tab and add the other tabs.
+  ui_test_utils::NavigateToURL(browser(), urls[0]);
+  for (int i = 1; i < NumTabs; i++) {
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(), urls[i], NEW_FOREGROUND_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  }
+
+  ASSERT_EQ(NumTabs, browser()->tab_strip_model()->count());
+
+  // Activate the tabs one by one following the random activation order.
+  for (auto i : activation_order) {

[sky, 2015/05/12 21:25:30]

nit: no {}

[Georges Khalil, 2015/05/15 16:55:30]

Done.

+    browser()->tab_strip_model()->ActivateTabAt(i, true);
+  }
+
+  // Close the browser.
+  g_browser_process->AddRefModule();
+  CloseBrowserSynchronously(browser());
+
+  StartObserving(NumTabs);
+
+  // Create a new window, which should trigger session restore.
+  ui_test_utils::BrowserAddedObserver window_observer;
+  chrome::NewEmptyWindow(browser()->profile(),
+                         chrome::HOST_DESKTOP_TYPE_NATIVE);
+  Browser* new_browser = window_observer.WaitForSingleNewBrowser();
+  WaitForAllTabsToStartLoading();
+  g_browser_process->ReleaseModule();
+
+  ASSERT_EQ(static_cast<size_t>(NumTabs), web_contents().size());
+  // Test that we have observed the tabs being loaded in the inverse order of
+  // their activation (MRU).
+  for (size_t i = 0; i < web_contents().size(); i++) {
+    GURL expected_url = urls[activation_order[NumTabs - i - 1]];
+    ASSERT_EQ(expected_url, web_contents()[i]->GetLastCommittedURL());
+  }
+
+  // Close the browser and open it again, to trigger another session restore.
+  // The goal is to ensure that activation time is persisted between session
+  // restores.
+
+  // Close the browser.
+  g_browser_process->AddRefModule();
+  CloseBrowserSynchronously(new_browser);
+
+  StartObserving(NumTabs);
+
+  // Create a new window, which should trigger session restore.
+  ui_test_utils::BrowserAddedObserver window_observer2;
+  chrome::NewEmptyWindow(browser()->profile(),
+                         chrome::HOST_DESKTOP_TYPE_NATIVE);
+  window_observer2.WaitForSingleNewBrowser();
+  WaitForAllTabsToStartLoading();
+  g_browser_process->ReleaseModule();
+
+  ASSERT_EQ(static_cast<size_t>(NumTabs), web_contents().size());
+  // Test that we have observed the tabs being loaded in the inverse order of
+  // their activation (MRU).
+  for (size_t i = 0; i < web_contents().size(); i++) {
+    GURL expected_url = urls[activation_order[NumTabs - i - 1]];
+    ASSERT_EQ(expected_url, web_contents()[i]->GetLastCommittedURL());
+  }
+}