Log TabMetric UKM when background tab loads

We normally log a TabMetric UKM when an existing tab is backgrounded.
If a new tab is opened in the background, we should log that as well so
we have an entry to correlate it with when it is later closed or
foregrounded.

Similarly, if a tab navigates to a new URL while in the background (e.g.
via JavaScript), we should log a new TabMetric to capture that URL.

To support testing this, add a version of WebContentsTester::Create that
takes a CreateParams so we can specify whether a WebContents (tab)
starts hidden. This enables other tests to also override CreateParams
fields when necessary.

Bug: 784639
Change-Id: I189af4535a238767bb257f39ee97301bac2a4171
Reviewed-on: https://chromium-review.googlesource.com/797930
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520716}

chrome/browser/ui/tabs/tab_activity_watcher.cc

@@ -98,6 +98,12 @@ class TabActivityWatcher::WebContentsData
     // Reset the per-page data.
     tab_metrics_.page_metrics = {};
   }
+  void DidStopLoading() override {
+    // Log metrics for the tab when it stops loading instead of immediately
+    // after a navigation commits, so we can have some idea of its status and
+    // contents.
+    TabActivityWatcher::GetInstance()->OnDidStopLoading(web_contents());
+  }
   void WasHidden() override {
     TabActivityWatcher::GetInstance()->OnWasHidden(web_contents());
   }
@@ -161,6 +167,14 @@ void TabActivityWatcher::OnWasHidden(content::WebContents* web_contents) {
   MaybeLogTab(web_contents);
 }
 
+void TabActivityWatcher::OnDidStopLoading(content::WebContents* web_contents) {
+  // Ignore load events in foreground tabs. The tab state of a foreground tab
+  // will be logged if/when it is backgrounded.
+  if (web_contents->IsVisible())
+    return;
+  MaybeLogTab(web_contents);
+}
+
 void TabActivityWatcher::MaybeLogTab(content::WebContents* web_contents) {
   // Don't log when the WebContents is being closed or replaced.
   if (web_contents->IsBeingDestroyed())

chrome/browser/ui/tabs/tab_activity_watcher.h

@@ -47,9 +47,12 @@ class TabActivityWatcher : public TabStripModelObserver,
   // BrowserTabStripTrackerDelegate:
   bool ShouldTrackBrowser(Browser* browser) override;
 
-  // Called from WebContentsData.
+  // Called from WebContentsData when a tab is being hidden.
   void OnWasHidden(content::WebContents* web_contents);
 
+  // Called from WebContentsData when a tab has stopped loading.
+  void OnDidStopLoading(content::WebContents* web_contents);
+
   // Logs the tab with |web_contents| if the tab hasn't been logged for the same
   // source ID within a timeout window.
   void MaybeLogTab(content::WebContents* web_contents);

chrome/browser/ui/tabs/tab_activity_watcher_unittest.cc

@@ -98,13 +98,16 @@ class TabActivityWatcherTest : public ChromeRenderViewHostTestHarness {
   }
 
   // Creates a new WebContents suitable for testing, adds it to the tab strip
-  // and navigates it to |initial_url|. The result is owned by the
-  // TabStripModel, so its tab must be closed later (e.g. via CloseAllTabs()).
+  // and commits a navigation to |initial_url|. The WebContents is owned by the
+  // TabStripModel, so its tab must be closed later, e.g. via CloseAllTabs().
   content::WebContents* AddWebContentsAndNavigate(
       TabStripModel* tab_strip_model,
       const GURL& initial_url) {
+    content::WebContents::CreateParams params(profile(), nullptr);
+    // Create as a background tab if there are other tabs in the tab strip.
+    params.initially_hidden = tab_strip_model->count() > 0;
     content::WebContents* test_contents =
-        WebContentsTester::CreateTestWebContents(profile(), nullptr);
+        WebContentsTester::CreateTestWebContents(params);
 
     // Create the TestWebContentsObserver to observe |test_contents|. When the
     // WebContents is destroyed, the observer will be reset automatically.
@@ -181,13 +184,21 @@ TEST_F(TabActivityWatcherTest, Basic) {
   auto browser = CreateBrowserWithTestWindowForParams(&params);
 
   TabStripModel* tab_strip_model = browser->tab_strip_model();
-  AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[0]));
-  AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[1]));
-
-  // Start with the leftmost tab activated.
+  content::WebContents* fg_contents =
+      AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[0]));
   tab_strip_model->ActivateTabAt(0, false);
+  WebContentsTester::For(fg_contents)->TestSetIsLoading(false);
+
+  // Adding, loading and activating a foreground tab doesn't trigger logging.
   EXPECT_FALSE(WasNewEntryRecorded());
 
+  // The second web contents is added as a background tab, so it logs an entry
+  // when it stops loading.
+  content::WebContents* bg_contents =
+      AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[1]));
+  WebContentsTester::For(bg_contents)->TestSetIsLoading(false);
+  ExpectNewEntry(kTestUrls[1], kBasicMetricValues);
+
   // Activating a tab logs the deactivated tab.
   SwitchToTabAt(tab_strip_model, 1);
   {
@@ -215,19 +226,30 @@ TEST_F(TabActivityWatcherTest, TabEvents) {
   TabStripModel* tab_strip_model = browser->tab_strip_model();
   content::WebContents* test_contents_1 =
       AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[0]));
+  tab_strip_model->ActivateTabAt(0, false);
+
+  // Opening the background tab triggers logging once the page finishes loading.
   content::WebContents* test_contents_2 =
       AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[1]));
-  tab_strip_model->ActivateTabAt(0, false);
+  EXPECT_FALSE(WasNewEntryRecorded());
+  WebContentsTester::For(test_contents_2)->TestSetIsLoading(false);
+  {
+    SCOPED_TRACE("");
+    ExpectNewEntry(GURL(kTestUrls[1]), kBasicMetricValues);
+  }
 
   // Navigating the active tab doesn't trigger logging.
   WebContentsTester::For(test_contents_1)->NavigateAndCommit(kTestUrls[2]);
+  WebContentsTester::For(test_contents_1)->TestSetIsLoading(false);
   EXPECT_FALSE(WasNewEntryRecorded());
 
   // Pinning the active tab doesn't trigger logging.
   tab_strip_model->SetTabPinned(0, true);
   EXPECT_FALSE(WasNewEntryRecorded());
 
+  LOG(ERROR) << "this is 247";
   // Pinning and unpinning the background tab triggers logging.
+  LOG(ERROR) << "this is 249";
   tab_strip_model->SetTabPinned(1, true);
   UkmMetricMap expected_metrics(kBasicMetricValues);
   expected_metrics[TabManager_TabMetrics::kIsPinnedName] = 1;
@@ -242,10 +264,15 @@ TEST_F(TabActivityWatcherTest, TabEvents) {
     ExpectNewEntry(GURL(kTestUrls[1]), kBasicMetricValues);
   }
 
-  // Navigating the background tab doesn't trigger logging.
-  // TODO(michaelpg): Logging should occur once the page loads.
+  // Navigating the background tab triggers logging once the page finishes
+  // loading.
   WebContentsTester::For(test_contents_2)->NavigateAndCommit(kTestUrls[0]);
   EXPECT_FALSE(WasNewEntryRecorded());
+  WebContentsTester::For(test_contents_2)->TestSetIsLoading(false);
+  {
+    SCOPED_TRACE("");
+    ExpectNewEntry(GURL(kTestUrls[0]), kBasicMetricValues);
+  }
 
   tab_strip_model->CloseAllTabs();
 }
@@ -258,14 +285,20 @@ TEST_F(TabActivityWatcherTest, TabMetrics) {
   TabStripModel* tab_strip_model = browser->tab_strip_model();
   content::WebContents* test_contents_1 =
       AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[0]));
-  content::WebContents* test_contents_2 =
-      AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[1]));
   tab_strip_model->ActivateTabAt(0, false);
-  EXPECT_FALSE(WasNewEntryRecorded());
 
   // Expected metrics for tab event.
   UkmMetricMap expected_metrics(kBasicMetricValues);
 
+  // Load background contents and verify UKM entry.
+  content::WebContents* test_contents_2 =
+      AddWebContentsAndNavigate(tab_strip_model, GURL(kTestUrls[1]));
+  WebContentsTester::For(test_contents_2)->TestSetIsLoading(false);
+  {
+    SCOPED_TRACE("");
+    ExpectNewEntry(kTestUrls[1], expected_metrics);
+  }
+
   // Site engagement score should round down to the nearest 10.
   SiteEngagementService::Get(profile())->ResetBaseScoreForURL(kTestUrls[1], 45);
   expected_metrics[TabManager_TabMetrics::kSiteEngagementScoreName] = 40;
@@ -281,19 +314,15 @@ TEST_F(TabActivityWatcherTest, TabMetrics) {
   // Navigate the background tab to a new domain.
   // Site engagement score for the new domain is 0.
   WebContentsTester::For(test_contents_2)->NavigateAndCommit(kTestUrls[2]);
-  EXPECT_FALSE(WasNewEntryRecorded());
+  WebContentsTester::For(test_contents_2)->TestSetIsLoading(false);
   expected_metrics[TabManager_TabMetrics::kSiteEngagementScoreName] = 0;
-
-  // Unpin the background tab to log an event.
-  tab_strip_model->SetTabPinned(0, false);
-  expected_metrics[TabManager_TabMetrics::kIsPinnedName] = 0;
   {
     SCOPED_TRACE("");
     ExpectNewEntry(kTestUrls[2], expected_metrics);
   }
 
   // Navigate the active tab and switch away from it. The entry should reflect
-  // the new URL.
+  // the new URL (even when the page hasn't finished loading).
   WebContentsTester::For(test_contents_1)->NavigateAndCommit(kTestUrls[2]);
   SwitchToTabAt(tab_strip_model, 0);
   {

content/public/test/web_contents_tester.cc

@@ -30,4 +30,9 @@ WebContents* WebContentsTester::CreateTestWebContents(
 }
 
 // static
+WebContents* WebContentsTester::CreateTestWebContents(
+    const WebContents::CreateParams& params) {
+  return TestWebContents::Create(params);
+}
+
 }  // namespace content

content/public/test/web_contents_tester.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "content/public/browser/site_instance.h"
+#include "content/public/browser/web_contents.h"
 #include "ui/base/page_transition_types.h"
 
 class GURL;
@@ -28,7 +29,6 @@ class BrowserContext;
 class NavigationData;
 class NavigationHandle;
 class RenderFrameHost;
-class WebContents;
 
 // This interface allows embedders of content/ to write tests that depend on a
 // test version of WebContents.  This interface can be retrieved from any
@@ -67,6 +67,10 @@ class WebContentsTester {
       BrowserContext* browser_context,
       scoped_refptr<SiteInstance> instance);
 
+  // Creates a WebContents enabled for testing with the given params.
+  static WebContents* CreateTestWebContents(
+      const WebContents::CreateParams& params);
+
   // Simulates the appropriate RenderView (pending if any, current otherwise)
   // sending a navigate notification for the NavigationController pending entry.
   virtual void CommitPendingNavigation() = 0;

content/test/test_web_contents.cc

@@ -44,8 +44,14 @@ TestWebContents::TestWebContents(BrowserContext* browser_context)
 TestWebContents* TestWebContents::Create(BrowserContext* browser_context,
                                          scoped_refptr<SiteInstance> instance) {
   TestWebContents* test_web_contents = new TestWebContents(browser_context);
-  test_web_contents->Init(
-      WebContents::CreateParams(browser_context, std::move(instance)));
+  test_web_contents->Init(CreateParams(browser_context, std::move(instance)));
+  return test_web_contents;
+}
+
+TestWebContents* TestWebContents::Create(const CreateParams& params) {
+  TestWebContents* test_web_contents =
+      new TestWebContents(params.browser_context);
+  test_web_contents->Init(params);
   return test_web_contents;
 }
 

content/test/test_web_contents.h

@@ -46,6 +46,7 @@ class TestWebContents : public WebContentsImpl, public WebContentsTester {
 
   static TestWebContents* Create(BrowserContext* browser_context,
                                  scoped_refptr<SiteInstance> instance);
+  static TestWebContents* Create(const CreateParams& params);
 
   // WebContentsImpl overrides (returning the same values, but in Test* types)
   TestRenderFrameHost* GetMainFrame() const override;