Fix started_in_foreground for OnStart() in session restore.

started_in_foreground in PageLoadMetricsObserver::OnStart() is false for
foreground tabs in session restore.

This patches fixes 2 things:
1. Correct the in_foreground variable in MetricsWebContentsObserver constructor.
2. Pass the correct visibility status of tabs in session_restore.cc.

TBR=xiyuan@chromium.org
BUG=740252

Change-Id: Ic8ab868bc850b20070813ec659d64c60345cc160
Reviewed-on: https://chromium-review.googlesource.com/569578
Commit-Queue: Duc Bui <ducbui@google.com>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486809}

chrome/browser/page_load_metrics/metrics_web_contents_observer.cc

@@ -68,7 +68,7 @@ MetricsWebContentsObserver::MetricsWebContentsObserver(
     const base::Optional<content::WebContents::CreateParams>& create_params,
     std::unique_ptr<PageLoadMetricsEmbedderInterface> embedder_interface)
     : content::WebContentsObserver(web_contents),
-      in_foreground_(create_params ? !create_params->initially_hidden : false),
+      in_foreground_(web_contents->IsVisible()),
       embedder_interface_(std::move(embedder_interface)),
       has_navigated_(false),
       page_load_metrics_binding_(web_contents, this) {

chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc

@@ -11,16 +11,23 @@
 #include "base/test/histogram_tester.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
+#include "build/build_config.h"
+#include "chrome/browser/lifetime/keep_alive_types.h"
+#include "chrome/browser/lifetime/scoped_keep_alive.h"
 #include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"
 #include "chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h"
 #include "chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h"
 #include "chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h"
 #include "chrome/browser/page_load_metrics/observers/no_state_prefetch_page_load_metrics_observer.h"
 #include "chrome/browser/page_load_metrics/page_load_tracker.h"
+#include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/prerender/prerender_histograms.h"
 #include "chrome/browser/prerender/prerender_origin.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sessions/session_service_factory.h"
+#include "chrome/browser/sessions/session_service_test_helper.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_features.h"
@@ -1070,3 +1077,95 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
 
   histogram_tester_.ExpectUniqueSample(internal::kHistogramTotalBytes, 0, 1);
 }
+
+class SessionRestorePageLoadMetricsBrowserTest
+    : public PageLoadMetricsBrowserTest {
+ public:
+  SessionRestorePageLoadMetricsBrowserTest() {}
+
+  // PageLoadMetricsBrowserTest:
+  void SetUpOnMainThread() override {
+    PageLoadMetricsBrowserTest::SetUpOnMainThread();
+    SessionStartupPref::SetStartupPref(
+        browser()->profile(), SessionStartupPref(SessionStartupPref::LAST));
+    ASSERT_TRUE(embedded_test_server()->Start());
+#if defined(OS_CHROMEOS)
+    SessionServiceTestHelper helper(
+        SessionServiceFactory::GetForProfile(browser()->profile()));
+    helper.SetForceBrowserNotAliveWithNoWindows(true);
+    helper.ReleaseService();
+#endif
+  }
+
+  Browser* QuitBrowserAndRestore(Browser* browser) {
+    Profile* profile = browser->profile();
+
+    std::unique_ptr<ScopedKeepAlive> keep_alive(new ScopedKeepAlive(
+        KeepAliveOrigin::SESSION_RESTORE, KeepAliveRestartOption::DISABLED));
+    CloseBrowserSynchronously(browser);
+
+    // Create a new window, which should trigger session restore.
+    chrome::NewEmptyWindow(profile);
+    ui_test_utils::BrowserAddedObserver window_observer;
+    return window_observer.WaitForSingleNewBrowser();
+  }
+
+  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 GetTestURL() const {
+    return embedded_test_server()->GetURL("/title1.html");
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SessionRestorePageLoadMetricsBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       InitialVisibilityOfSingleRestoredTab) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  histogram_tester_.ExpectTotalCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, 1);
+  histogram_tester_.ExpectBucketCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, true, 1);
+
+  Browser* new_browser = QuitBrowserAndRestore(browser());
+  WaitForTabsToLoad(new_browser);
+
+  histogram_tester_.ExpectTotalCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, 2);
+  histogram_tester_.ExpectBucketCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, true, 2);
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       InitialVisibilityOfMultipleRestoredTabs) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GetTestURL(), WindowOpenDisposition::NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  histogram_tester_.ExpectTotalCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, 2);
+  histogram_tester_.ExpectBucketCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, false, 1);
+
+  Browser* new_browser = QuitBrowserAndRestore(browser());
+  WaitForTabsToLoad(new_browser);
+
+  TabStripModel* tab_strip = new_browser->tab_strip_model();
+  ASSERT_TRUE(tab_strip);
+  ASSERT_EQ(2, tab_strip->count());
+
+  histogram_tester_.ExpectTotalCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, 4);
+  histogram_tester_.ExpectBucketCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, true, 2);
+  histogram_tester_.ExpectBucketCount(
+      page_load_metrics::internal::kPageLoadStartedInForeground, false, 2);
+}

chrome/browser/page_load_metrics/page_load_tracker.h

@@ -44,6 +44,7 @@ extern const char kAbortChainSizeNewNavigation[];
 extern const char kAbortChainSizeNoCommit[];
 extern const char kAbortChainSizeSameURL[];
 extern const char kPageLoadCompletedAfterAppBackground[];
+extern const char kPageLoadStartedInForeground[];
 
 }  // namespace internal
 

chrome/browser/sessions/session_restore.cc

@@ -613,13 +613,12 @@ class SessionRestoreImpl : public content::NotificationObserver {
 
     WebContents* web_contents = chrome::AddRestoredTab(
         browser, tab.navigations, tab_index, selected_index,
-        tab.extension_app_id,
-        false,  // select
-        tab.pinned, true, session_storage_namespace.get(),
-        tab.user_agent_override);
-    // Regression check: check that the tab didn't start loading right away. The
+        tab.extension_app_id, is_selected_tab, tab.pinned, true,
+        session_storage_namespace.get(), tab.user_agent_override);
+    // Regression check: if the current tab |is_selected_tab|, it should load
+    // immediately, otherwise, tabs should not start loading right away. The
     // focused tab will be loaded by Browser, and TabLoader will load the rest.
-    DCHECK(web_contents->GetController().NeedsReload());
+    DCHECK(is_selected_tab || web_contents->GetController().NeedsReload());
 
     return web_contents;
   }

chrome/browser/sessions/session_restore_browsertest_chromeos.cc

@@ -198,8 +198,8 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTestChromeOS, RestoreMinimized) {
       ++minimized_count;
   }
   EXPECT_EQ(2u, total_count);
-  // Chrome OS always activates the last browser window on login, which results
-  // in one window being restored. This seems reasonable as it reminds users
-  // they have a browser running instead of just showing them an empty desktop.
-  EXPECT_EQ(1u, minimized_count);
+  // Chrome OS always activates the last browser windows on login to remind
+  // users they have a browser running instead of just showing them an empty
+  // desktop.
+  EXPECT_EQ(0u, minimized_count);
 }