[Tab Metrics] Measure FP, FCP and FMP for Foreground Tab during Session Restore

chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc

Index: chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
diff --git a/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc b/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
index d69596d91d1237754423b7a0bef40dc08c2d9ca2..9a3221667b1af68c3f762cd8c281ed6c76c865e3 100644
--- a/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
+++ b/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
@@ -2,10 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/command_line.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
@@ -19,12 +25,14 @@
 #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/observers/session_restore_page_load_metrics_observer.h"
 #include "chrome/browser/page_load_metrics/observers/use_counter_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_restore.h"
 #include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/sessions/session_service_test_helper.h"
 #include "chrome/browser/ui/browser.h"
@@ -37,11 +45,14 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/prefs/pref_service.h"
+#include "components/sessions/core/serialized_navigation_entry.h"
+#include "components/sessions/core/serialized_navigation_entry_test_helper.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/referrer.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/download_test_observer.h"
 #include "net/base/net_errors.h"
@@ -53,6 +64,7 @@
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "third_party/WebKit/public/platform/web_feature.mojom.h"
+#include "url/gurl.h"
 
 namespace {
 
@@ -1142,6 +1154,23 @@ class SessionRestorePageLoadMetricsBrowserTest
     return window_observer.WaitForSingleNewBrowser();
   }
 
+  std::unique_ptr<PageLoadMetricsWaiter> CreatePageLoadMetricsWaiter(

[Charlie Harrison, 2017/08/07 14:30:08]

nit: I would just remove this method, it doesn't gain you much for the
additional layer of abstraction.

[ducbui, 2017/08/08 16:29:48]

Acknowledged. I will remove it.

+      content::WebContents* web_contents) const {
+    return base::MakeUnique<PageLoadMetricsWaiter>(web_contents);
+  }
+
+  void WaitForFirstMeaningfulPaintOfActiveTab(Browser* browser) const {
+    WaitForFirstMeaningfulPaintOfTab(
+        browser->tab_strip_model()->GetActiveWebContents());
+  }
+
+  void WaitForFirstMeaningfulPaintOfTab(
+      content::WebContents* web_contents) const {
+    auto waiter = CreatePageLoadMetricsWaiter(web_contents);

[Bryan McQuade, 2017/08/09 15:41:34]

generally we prefer to create the waiter before navigating, and call Wait()
after navigating. otherwise there's the potential for races. so i'm not sure i
recomment adding helpers like this - better to inline in each test method
instead.

[ducbui, 2017/08/09 19:43:06]

Ack. We should ensure to create the Waiter before any navigation. I added
SessionRestoreFirstMeaningfulPaintWaiter to create a Waiter right after
the creation of a restored tab and before any of its navigations.

+    waiter->AddPageExpectation(TimingField::FIRST_MEANINGFUL_PAINT);
+    waiter->Wait();
+  }
+
   void WaitForTabsToLoad(Browser* browser) {
     for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
       content::WebContents* contents =
@@ -1151,8 +1180,28 @@ class SessionRestorePageLoadMetricsBrowserTest
     }
   }
 
+  // The PageLoadMetricsWaiter can observe first meaningful paints on these test
+  // pages while not on other simple pages such as /title1.html.
   GURL GetTestURL() const {
-    return embedded_test_server()->GetURL("/title1.html");
+    return embedded_test_server()->GetURL(
+        "/page_load_metrics/page_with_css.html");

[Charlie Harrison, 2017/08/07 14:30:08]

page_with_css is not very aptly named, it does a lot of needless computation to
ensure that we spend more than 1ms of time parsing css.

Can we choose another site? Why doesn't title1.html work?

[ducbui, 2017/08/08 16:29:48]

Using "/title1.html" yields the first paint and first contentful paint, but no
first meaningful paint, so it makes PageLoadMetricsWaiter timeout. I checked
which timing is updated on PageLoadMetricsWaiter::OnTimingUpdated(). The first
meaningful paint does happen when I use more complicated pages like the page
with css.

Currently I do not have any clue to explain and will do further investigation
about this issue. I checked FirstMeaningfulPaintDetector but it seems to work
normally.

[Charlie Harrison, 2017/08/08 17:00:29]

Hm maybe FMP needs > 1 layout to work correctly? In any case I would still
prefer if we could use another URL, maybe main_frame_with_iframe with a query
param would work.

[ducbui, 2017/08/08 22:21:06]

Heuristics in FirstMeaningfulPaintDetector.cpp skip the first meaningful paint
candidate which is generally the first contentful paint, so the page might need
to have at least 2 paints to have the first meaningful paint.

I changed the testing URLs to main_frame_with_iframe.html and iframes.html
which also work.

+  }
+
+  GURL GetTestURL2() const {
+    return embedded_test_server()->GetURL(
+        "/page_load_metrics/main_frame_with_iframe.html");

[Bryan McQuade, 2017/08/09 15:41:34]

can we use title2.html to be consistent?

[ducbui, 2017/08/09 19:43:06]

Acknowledged. I will change this to title2.html.

+  }
+
+  void ExpectFirstPaintMetricsTotalCount(int expected_total_count) const {
+    histogram_tester_.ExpectTotalCount(
+        internal::kHistogramSessionRestoreForegroundTabFirstPaint,
+        expected_total_count);
+    histogram_tester_.ExpectTotalCount(
+        internal::kHistogramSessionRestoreForegroundTabFirstContentfulPaint,
+        expected_total_count);
+    histogram_tester_.ExpectTotalCount(
+        internal::kHistogramSessionRestoreForegroundTabFirstMeaningfulPaint,
+        expected_total_count);
   }
 
  private:
@@ -1201,3 +1250,217 @@ IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
   histogram_tester_.ExpectBucketCount(
       page_load_metrics::internal::kPageLoadStartedInForeground, false, 2);
 }
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       NoSessionRestore) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  ExpectFirstPaintMetricsTotalCount(0);
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       SingleTabSessionRestore) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  Browser* new_browser = QuitBrowserAndRestore(browser());
+  WaitForFirstMeaningfulPaintOfActiveTab(new_browser);
+  ExpectFirstPaintMetricsTotalCount(1);
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       MultipleTabsSessionRestore) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GetTestURL(), WindowOpenDisposition::NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  Browser* new_browser = QuitBrowserAndRestore(browser());
+
+  TabStripModel* tab_strip = new_browser->tab_strip_model();
+  ASSERT_TRUE(tab_strip);
+  ASSERT_EQ(2, tab_strip->count());
+
+  WaitForFirstMeaningfulPaintOfActiveTab(new_browser);
+  WaitForTabsToLoad(new_browser);
+
+  // Only metrics of the initial foreground tab are recorded.
+  ExpectFirstPaintMetricsTotalCount(1);
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       NavigationDuringSessionRestore) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  Browser* new_browser = QuitBrowserAndRestore(browser());
+
+  content::WebContents* active_web_contents =
+      new_browser->tab_strip_model()->GetActiveWebContents();
+  auto waiter = base::MakeUnique<PageLoadMetricsWaiter>(active_web_contents);
+  waiter->AddPageExpectation(TimingField::FIRST_MEANINGFUL_PAINT);
+
+  // Load another url in the initial tab before its first paints.
+  ExpectFirstPaintMetricsTotalCount(0);
+  // If the current web contents is not stopped, the next navigation
+  // will not yield any paints.
+  active_web_contents->Stop();

[Charlie Harrison, 2017/08/07 14:30:08]

I'm not quite sure why this is needed? Shouldn't loading a new URL in the
contents automatically stop the previous navigation?

How does it affect paint metrics?

[ducbui, 2017/08/08 16:29:48]

Acknowledged.
I tried to use ui_test_utils::NavigateToURL() but there are only first paint
and first contentful pain while not first meaningful paint (even with the page
with CSS).
I will do further debugging on this issue.

[Charlie Harrison, 2017/08/08 17:00:29]

I see. Don't let this block you too much if it turns out to be difficult. If
that's the case let's just beef up this comment and maybe add a TODO(ducbui)
next to it.

[ducbui, 2017/08/08 22:21:07]

I added a TODO for this issue.

I tried other APIs for loading URLs but they did not work. When I changed the
testing URL to "/page_load_metrics/iframes.html", it worked without the Stop()
without browser-side navigation but did not produce the first meaningful paint
with browser-side navigation enabled.

I think I need to debug all the first meaningful-paint code to resolve this
issue.

[Bryan McQuade, 2017/08/09 15:41:34]

agree - if you have to call this method, something else seems wrong here, and we
should figure out what's going on.

[ducbui, 2017/08/10 01:01:17]

I removed the Stop() and added some calculation to distinguish the first
paints of the first navigation and the second navigation to test that no metrics
of the second navigation are recorded.

+  active_web_contents->GetController().LoadURL(
+      GetTestURL2(), content::Referrer(), ui::PAGE_TRANSITION_TYPED,
+      std::string());
+
+  waiter->Wait();
+
+  // No metrics recorded because it navigates away during session restore.

[Charlie Harrison, 2017/08/07 14:30:08]

s/it/the tab

[ducbui, 2017/08/08 16:29:48]

Done.

+  ExpectFirstPaintMetricsTotalCount(0);
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       LoadingAfterSessionRestore) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  Browser* new_browser = QuitBrowserAndRestore(browser());
+
+  WaitForFirstMeaningfulPaintOfActiveTab(new_browser);
+  ExpectFirstPaintMetricsTotalCount(1);
+
+  // Load a new page after session restore.
+  auto waiter = CreatePageLoadMetricsWaiter(
+      new_browser->tab_strip_model()->GetActiveWebContents());
+  waiter->AddPageExpectation(TimingField::FIRST_MEANINGFUL_PAINT);
+  ui_test_utils::NavigateToURL(new_browser, GetTestURL2());
+  waiter->Wait();
+
+  // No more metrics because the navigation is after session restore.
+  ExpectFirstPaintMetricsTotalCount(1);
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       InitialForegroundTabChanged) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GetTestURL2(), WindowOpenDisposition::NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  Browser* new_browser = QuitBrowserAndRestore(browser());
+
+  // Change the foreground tab before the first meaningful paint.
+  TabStripModel* tab_strip = new_browser->tab_strip_model();
+  ASSERT_TRUE(tab_strip);
+  ASSERT_EQ(2, tab_strip->count());
+  ASSERT_EQ(0, tab_strip->active_index());
+  tab_strip->ActivateTabAt(1, true);
+
+  WaitForFirstMeaningfulPaintOfActiveTab(new_browser);
+
+  // No metrics were recorded because initial foreground tab was switched away.
+  ExpectFirstPaintMetricsTotalCount(0);
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       MultipleSessionRestores) {
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+
+  Browser* current_browser = browser();
+  const int num_session_restores = 3;
+  for (int i = 1; i <= num_session_restores; ++i) {
+    current_browser = QuitBrowserAndRestore(current_browser);
+    WaitForFirstMeaningfulPaintOfActiveTab(current_browser);
+    ExpectFirstPaintMetricsTotalCount(i);
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       RestoreForeignTab) {
+  sessions::SerializedNavigationEntry nav =
+      sessions::SerializedNavigationEntryTestHelper::CreateNavigation(
+          GetTestURL().spec(), "one");
+
+  // Set up the restore data.
+  sync_pb::SessionTab sync_data;
+  sync_data.set_tab_visual_index(0);
+  sync_data.set_current_navigation_index(1);
+  sync_data.set_pinned(false);
+  sync_data.add_navigation()->CopyFrom(nav.ToSyncData());
+
+  sessions::SessionTab tab;
+  tab.SetFromSyncData(sync_data, base::Time::Now());
+  ASSERT_EQ(1, browser()->tab_strip_model()->count());
+
+  // Restore in the current tab.
+  content::WebContents* tab_content = SessionRestore::RestoreForeignSessionTab(
+      browser()->tab_strip_model()->GetActiveWebContents(), tab,
+      WindowOpenDisposition::CURRENT_TAB);
+  ASSERT_EQ(1, browser()->tab_strip_model()->count());
+  ASSERT_TRUE(tab_content);
+  ASSERT_EQ(GetTestURL(), tab_content->GetURL());
+
+  WaitForFirstMeaningfulPaintOfTab(tab_content);
+  ExpectFirstPaintMetricsTotalCount(1);
+
+  // Restore in a new foreground tab.
+  tab_content = SessionRestore::RestoreForeignSessionTab(
+      browser()->tab_strip_model()->GetActiveWebContents(), tab,
+      WindowOpenDisposition::NEW_FOREGROUND_TAB);
+  ASSERT_EQ(2, browser()->tab_strip_model()->count());
+  ASSERT_EQ(1, browser()->tab_strip_model()->active_index());
+  ASSERT_TRUE(tab_content);
+  ASSERT_EQ(GetTestURL(), tab_content->GetURL());
+
+  WaitForFirstMeaningfulPaintOfTab(tab_content);
+  ExpectFirstPaintMetricsTotalCount(2);
+
+  // Restore in a new background tab.
+  tab_content = SessionRestore::RestoreForeignSessionTab(
+      browser()->tab_strip_model()->GetActiveWebContents(), tab,
+      WindowOpenDisposition::NEW_BACKGROUND_TAB);
+  ASSERT_EQ(3, browser()->tab_strip_model()->count());
+  ASSERT_EQ(1, browser()->tab_strip_model()->active_index());
+  ASSERT_TRUE(tab_content);
+  ASSERT_EQ(GetTestURL(), tab_content->GetURL());
+  WaitForTabsToLoad(browser());
+
+  // Do not record timings of initially background tabs.
+  ExpectFirstPaintMetricsTotalCount(2);
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestorePageLoadMetricsBrowserTest,
+                       RestoreForeignSession) {
+  Profile* profile = browser()->profile();
+
+  sessions::SerializedNavigationEntry nav1 =
+      sessions::SerializedNavigationEntryTestHelper::CreateNavigation(
+          GetTestURL().spec(), "one");
+  sessions::SerializedNavigationEntry nav2 =
+      sessions::SerializedNavigationEntryTestHelper::CreateNavigation(
+          GetTestURL2().spec(), "two");
+
+  // Set up the restore data: one window with two tabs.
+  std::vector<const sessions::SessionWindow*> session;
+  sessions::SessionWindow window;
+  auto tab1 = base::MakeUnique<sessions::SessionTab>();
+  {
+    sync_pb::SessionTab sync_data;
+    sync_data.set_tab_visual_index(0);
+    sync_data.set_current_navigation_index(0);
+    sync_data.set_pinned(true);
+    sync_data.add_navigation()->CopyFrom(nav1.ToSyncData());
+    tab1->SetFromSyncData(sync_data, base::Time::Now());
+  }
+  window.tabs.push_back(std::move(tab1));
+
+  auto tab2 = base::MakeUnique<sessions::SessionTab>();
+  {
+    sync_pb::SessionTab sync_data;
+    sync_data.set_tab_visual_index(1);
+    sync_data.set_current_navigation_index(0);
+    sync_data.set_pinned(false);
+    sync_data.add_navigation()->CopyFrom(nav2.ToSyncData());
+    tab2->SetFromSyncData(sync_data, base::Time::Now());
+  }
+  window.tabs.push_back(std::move(tab2));
+
+  // Restore the session window with 2 tabs.
+  session.push_back(static_cast<const sessions::SessionWindow*>(&window));
+  ui_test_utils::BrowserAddedObserver window_observer;
+  SessionRestore::RestoreForeignSessionWindows(profile, session.begin(),
+                                               session.end());
+  Browser* new_browser = window_observer.WaitForSingleNewBrowser();
+  ASSERT_TRUE(new_browser);
+  ASSERT_EQ(2, new_browser->tab_strip_model()->count());
+
+  WaitForFirstMeaningfulPaintOfActiveTab(new_browser);
+  ExpectFirstPaintMetricsTotalCount(1);
+}