Separate TabMetrics data from logging

TabMetricsLogger logs the TabMetrics UKM event for a background tab.
This event includes the state of the tab, as well as some slightly
tricky or nullable fields.

Add a new TabFeatures struct that contains all this information, and
generate the struct in a separate function. The UKM logging function
now simply forwards info from the TabFeatures struct to the UKM event.

Later we will use the same TabFeatures to calculate tab scores with an
ML model. Encapsulating the logic for these features in a single
function ensures that we use the same logic for logging as we will for
inference.

Bug: 784639, 783989
Change-Id: I11e64119ce30465f98409f54f7e97af0251ce4b3
Reviewed-on: https://chromium-review.googlesource.com/1028562
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554694}

chrome/browser/BUILD.gn

@@ -2628,6 +2628,8 @@ jumbo_split_static_library("browser") {
       "resource_coordinator/site_characteristics_data_store.h",
       "resource_coordinator/tab_activity_watcher.cc",
       "resource_coordinator/tab_activity_watcher.h",
+      "resource_coordinator/tab_features.cc",
+      "resource_coordinator/tab_features.h",
       "resource_coordinator/tab_lifecycle_observer.h",
       "resource_coordinator/tab_lifecycle_unit.cc",
       "resource_coordinator/tab_lifecycle_unit.h",

chrome/browser/resource_coordinator/tab_features.cc

+// Copyright 2018 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 "chrome/browser/resource_coordinator/tab_features.h"
+
+namespace resource_coordinator {
+
+TabFeatures::TabFeatures() = default;
+
+TabFeatures::~TabFeatures() = default;
+
+TabFeatures::TabFeatures(const TabFeatures& other) = default;
+
+}  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_features.h

+// Copyright 2018 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.
+
+#ifndef CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_FEATURES_H_
+#define CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_FEATURES_H_
+
+#include <stdint.h>
+#include <string>
+
+#include "base/optional.h"
+#include "chrome/browser/resource_coordinator/tab_metrics_event.pb.h"
+#include "ui/base/page_transition_types.h"
+
+namespace resource_coordinator {
+
+// Tab features used for logging a Tab Ranker example to UKM or calculating a
+// Tab Ranker score.
+struct TabFeatures {
+  TabFeatures();
+  ~TabFeatures();
+
+  TabFeatures(const TabFeatures& other);
+
+  // Keep properties in alphabetical order to match the order in
+  // TabMetricsLogger::LogBackgroundTab() and make it easier to check which
+  // properties are sent via UKM.
+  metrics::TabMetricsEvent::ContentType content_type =
+      metrics::TabMetricsEvent::CONTENT_TYPE_UNKNOWN;
+  bool has_before_unload_handler = false;
+  bool has_form_entry = false;
+  bool is_extension_protected = false;
+  bool is_pinned = false;
+  int32_t key_event_count = 0;
+  int32_t mouse_event_count = 0;
+  int32_t navigation_entry_count = 0;
+  // Null if the value is not one of the core values logged to UKM.
+  base::Optional<ui::PageTransition> page_transition_core_type;
+  bool page_transition_from_address_bar = false;
+  bool page_transition_is_redirect = false;
+  // Null if the SiteEngagementService is disabled.
+  base::Optional<int32_t> site_engagement_score;
+  int32_t touch_event_count = 0;
+  bool was_recently_audible = false;
+};
+
+}  // namespace resource_coordinator
+
+#endif  // CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_FEATURES_H_

chrome/browser/resource_coordinator/tab_metrics_logger.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/custom_handlers/protocol_handler_registry_factory.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/resource_coordinator/tab_features.h"
 #include "chrome/browser/resource_coordinator/tab_lifecycle_unit_external.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -89,8 +90,8 @@ void PopulateProtocolHandlers(content::WebContents* web_contents,
 }
 
 // Populates navigation-related metrics.
-void PopulatePageTransitionMetrics(ukm::builders::TabManager_TabMetrics* entry,
-                                   ui::PageTransition page_transition) {
+void PopulatePageTransitionFeatures(resource_coordinator::TabFeatures* tab,
+                                    ui::PageTransition page_transition) {
   // We only report the following core types.
   // Note: Redirects unrelated to clicking a link still get the "link" type.
   if (ui::PageTransitionCoreTypeIs(page_transition, ui::PAGE_TRANSITION_LINK) ||
@@ -100,14 +101,14 @@ void PopulatePageTransitionMetrics(ukm::builders::TabManager_TabMetrics* entry,
                                    ui::PAGE_TRANSITION_FORM_SUBMIT) ||
       ui::PageTransitionCoreTypeIs(page_transition,
                                    ui::PAGE_TRANSITION_RELOAD)) {
-    entry->SetPageTransitionCoreType(
-        ui::PageTransitionStripQualifier(page_transition));
+    tab->page_transition_core_type =
+        ui::PageTransitionStripQualifier(page_transition);
   }
 
-  entry->SetPageTransitionFromAddressBar(
-      (page_transition & ui::PAGE_TRANSITION_FROM_ADDRESS_BAR) != 0);
-  entry->SetPageTransitionIsRedirect(
-      ui::PageTransitionIsRedirect(page_transition));
+  tab->page_transition_from_address_bar =
+      (page_transition & ui::PAGE_TRANSITION_FROM_ADDRESS_BAR) != 0;
+  tab->page_transition_is_redirect =
+      ui::PageTransitionIsRedirect(page_transition);
 }
 
 // Logs the TabManager.Background.ForegroundedOrClosed event.
@@ -184,6 +185,51 @@ int TabMetricsLogger::GetSiteEngagementScore(
   return rounded_score;
 }
 
+// static
+resource_coordinator::TabFeatures TabMetricsLogger::GetTabFeatures(
+    const Browser* browser,
+    const TabMetricsLogger::TabMetrics& tab_metrics) {
+  DCHECK(browser);
+  const TabStripModel* tab_strip_model = browser->tab_strip_model();
+  content::WebContents* web_contents = tab_metrics.web_contents;
+
+  resource_coordinator::TabFeatures tab;
+
+  TabMetricsEvent::ContentType content_type =
+      GetContentTypeFromMimeType(web_contents->GetContentsMimeType());
+  tab.content_type = content_type;
+  tab.has_before_unload_handler =
+      web_contents->GetMainFrame()->GetSuddenTerminationDisablerState(
+          blink::kBeforeUnloadHandler);
+  tab.has_form_entry =
+      web_contents->GetPageImportanceSignals().had_form_interaction;
+  tab.is_extension_protected =
+      !resource_coordinator::TabLifecycleUnitExternal::FromWebContents(
+           web_contents)
+           ->IsAutoDiscardable();
+
+  int index = tab_strip_model->GetIndexOfWebContents(web_contents);
+  DCHECK_NE(index, TabStripModel::kNoTab);
+  tab.is_pinned = tab_strip_model->IsTabPinned(index);
+
+  tab.key_event_count = tab_metrics.page_metrics.key_event_count;
+  tab.mouse_event_count = tab_metrics.page_metrics.mouse_event_count;
+  tab.navigation_entry_count = web_contents->GetController().GetEntryCount();
+
+  PopulatePageTransitionFeatures(&tab, tab_metrics.page_transition);
+
+  if (SiteEngagementService::IsEnabled()) {
+    tab.site_engagement_score = GetSiteEngagementScore(web_contents);
+  }
+
+  tab.touch_event_count = tab_metrics.page_metrics.touch_event_count;
+
+  // This checks if the tab was audible within the past two seconds, same as the
+  // audio indicator in the tab strip.
+  tab.was_recently_audible = web_contents->WasRecentlyAudible();
+  return tab;
+}
+
 void TabMetricsLogger::LogBackgroundTab(ukm::SourceId ukm_source_id,
                                         const TabMetrics& tab_metrics) {
   if (!ukm_source_id)
@@ -211,46 +257,36 @@ void TabMetricsLogger::LogBackgroundTab(ukm::SourceId ukm_source_id,
   DCHECK_NE(index, TabStripModel::kNoTab);
 
   ukm::builders::TabManager_TabMetrics entry(ukm_source_id);
-
-  // The browser window logs its own usage UKMs with its session ID.
-  entry.SetWindowId(browser->session_id().id());
-
-  entry.SetKeyEventCount(tab_metrics.page_metrics.key_event_count)
-      .SetMouseEventCount(tab_metrics.page_metrics.mouse_event_count)
-      .SetTouchEventCount(tab_metrics.page_metrics.touch_event_count);
+  resource_coordinator::TabFeatures tab = GetTabFeatures(browser, tab_metrics);
 
   PopulateProtocolHandlers(web_contents, &entry);
 
-  const int engagement_score = GetSiteEngagementScore(web_contents);
-  if (engagement_score >= 0) {
-    entry.SetSiteEngagementScore(engagement_score);
-  }
-
-  TabMetricsEvent::ContentType content_type =
-      GetContentTypeFromMimeType(web_contents->GetContentsMimeType());
-  entry.SetContentType(static_cast<int>(content_type));
+  // Keep these Set functions in alphabetical order so they're easy to check
+  // against the list of metrics in the UKM event.
   // TODO(michaelpg): Add PluginType field if mime type matches "application/*"
   // using PluginUMAReporter.
+  entry.SetContentType(tab.content_type);
+  entry.SetHasBeforeUnloadHandler(tab.has_before_unload_handler);
+  entry.SetHasFormEntry(tab.has_form_entry);
+  entry.SetIsExtensionProtected(tab.is_extension_protected);
+  entry.SetIsPinned(tab.is_pinned);
+  entry.SetKeyEventCount(tab.key_event_count);
+  entry.SetMouseEventCount(tab.mouse_event_count);
+  entry.SetNavigationEntryCount(tab.navigation_entry_count);
+  if (tab.page_transition_core_type.has_value())
+    entry.SetPageTransitionCoreType(tab.page_transition_core_type.value());
+  entry.SetPageTransitionFromAddressBar(tab.page_transition_from_address_bar);
+  entry.SetPageTransitionIsRedirect(tab.page_transition_is_redirect);
+  entry.SetSequenceId(++sequence_id_);
+  if (tab.site_engagement_score.has_value())
+    entry.SetSiteEngagementScore(tab.site_engagement_score.value());
+  entry.SetTouchEventCount(tab.touch_event_count);
+  entry.SetWasRecentlyAudible(tab.was_recently_audible);
 
-  // This checks if the tab was audible within the past two seconds, same as the
-  // audio indicator in the tab strip.
-  entry.SetWasRecentlyAudible(web_contents->WasRecentlyAudible());
-
-  PopulatePageTransitionMetrics(&entry, tab_metrics.page_transition);
-  entry
-      .SetHasBeforeUnloadHandler(
-          web_contents->GetMainFrame()->GetSuddenTerminationDisablerState(
-              blink::kBeforeUnloadHandler))
-      .SetHasFormEntry(
-          web_contents->GetPageImportanceSignals().had_form_interaction)
-      .SetIsExtensionProtected(
-          !resource_coordinator::TabLifecycleUnitExternal::FromWebContents(
-               web_contents)
-               ->IsAutoDiscardable())
-      .SetIsPinned(tab_strip_model->IsTabPinned(index))
-      .SetNavigationEntryCount(web_contents->GetController().GetEntryCount())
-      .SetSequenceId(++sequence_id_)
-      .Record(ukm::UkmRecorder::Get());
+  // The browser window logs its own usage UKMs with its session ID.
+  entry.SetWindowId(browser->session_id().id());
+
+  entry.Record(ukm::UkmRecorder::Get());
 }
 
 void TabMetricsLogger::LogBackgroundTabShown(ukm::SourceId ukm_source_id,

chrome/browser/resource_coordinator/tab_metrics_logger.h

@@ -10,6 +10,8 @@
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "ui/base/page_transition_types.h"
 
+class Browser;
+
 namespace base {
 class TimeDelta;
 }  // namespace base
@@ -18,6 +20,10 @@ namespace content {
 class WebContents;
 }  // namespace content
 
+namespace resource_coordinator {
+struct TabFeatures;
+}  // namespace resource_coordinator
+
 // Logs metrics for a tab and its WebContents when requested.
 // Must be used on the UI thread.
 class TabMetricsLogger {
@@ -90,6 +96,14 @@ class TabMetricsLogger {
   // to limit granularity. Returns -1 if site engagement service is disabled.
   static int GetSiteEngagementScore(const content::WebContents* web_contents);
 
+  // Creates TabFeatures for logging or scoring tabs.
+  // A common function for populating these features ensures that the same
+  // values are used for logging training examples to UKM and for locally
+  // scoring tabs.
+  static resource_coordinator::TabFeatures GetTabFeatures(
+      const Browser* browser,
+      const TabMetrics& tab_metrics);
+
  private:
   // A counter to be incremented and logged with each UKM entry, used to
   // indicate the order that events within the same report were logged.

chrome/browser/resource_coordinator/tab_metrics_logger_unittest.cc

@@ -4,16 +4,125 @@
 
 #include "chrome/browser/resource_coordinator/tab_metrics_logger.h"
 
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/engagement/site_engagement_service.h"
+#include "chrome/browser/resource_coordinator/tab_features.h"
+#include "chrome/browser/resource_coordinator/tab_manager.h"
 #include "chrome/browser/resource_coordinator/tab_metrics_event.pb.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_activity_simulator.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/test_browser_window.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/web_contents_tester.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using metrics::TabMetricsEvent;
+using content::WebContentsTester;
 
 // Sanity checks for functions in TabMetricsLogger.
 // See TabActivityWatcherTest for more thorough tab usage UKM tests.
+using TabMetricsLoggerTest = ChromeRenderViewHostTestHarness;
+
+// Tests creating a flat TabFeatures structure for logging a tab and its
+// TabMetrics state.
+TEST_F(TabMetricsLoggerTest, TabFeatures) {
+  TabActivitySimulator tab_activity_simulator;
+  Browser::CreateParams params(profile(), true);
+  std::unique_ptr<Browser> browser =
+      CreateBrowserWithTestWindowForParams(&params);
+  TabStripModel* tab_strip_model = browser->tab_strip_model();
+
+  // Add a foreground tab.
+  tab_activity_simulator.AddWebContentsAndNavigate(tab_strip_model,
+                                                   GURL("about://blank"));
+  tab_strip_model->ActivateTabAt(0, false);
+
+  // Add a background tab to test.
+  content::WebContents* bg_contents =
+      tab_activity_simulator.AddWebContentsAndNavigate(
+          tab_strip_model, GURL("http://example.com/test.html"));
+  WebContentsTester::For(bg_contents)->TestSetIsLoading(false);
+
+  {
+    TabMetricsLogger::TabMetrics bg_metrics;
+    bg_metrics.web_contents = bg_contents;
+    bg_metrics.page_transition = ui::PAGE_TRANSITION_FORM_SUBMIT;
+
+    resource_coordinator::TabFeatures bg_features =
+        TabMetricsLogger::GetTabFeatures(browser.get(), bg_metrics);
+    EXPECT_EQ(TabMetricsEvent::CONTENT_TYPE_TEXT_HTML,
+              bg_features.content_type);
+    EXPECT_EQ(bg_features.has_before_unload_handler, false);
+    EXPECT_EQ(bg_features.has_form_entry, false);
+    EXPECT_EQ(bg_features.is_extension_protected, false);
+    EXPECT_EQ(bg_features.is_pinned, false);
+    EXPECT_EQ(bg_features.key_event_count, 0);
+    EXPECT_EQ(bg_features.mouse_event_count, 0);
+    EXPECT_EQ(bg_features.navigation_entry_count, 1);
+    ASSERT_TRUE(bg_features.page_transition_core_type.has_value());
+    EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs(
+        ui::PAGE_TRANSITION_FORM_SUBMIT,
+        bg_features.page_transition_core_type.value()));
+    EXPECT_EQ(bg_features.page_transition_from_address_bar, false);
+    EXPECT_EQ(bg_features.page_transition_is_redirect, false);
+    ASSERT_TRUE(bg_features.site_engagement_score.has_value());
+    EXPECT_EQ(bg_features.site_engagement_score.value(), 0);
+    EXPECT_EQ(bg_features.touch_event_count, 0);
+    EXPECT_EQ(bg_features.was_recently_audible, false);
+  }
+
+  // Update tab features.
+  ui::PageTransition page_transition = static_cast<ui::PageTransition>(
+      ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR);
+  tab_activity_simulator.Navigate(bg_contents, GURL("https://www.chromium.org"),
+                                  page_transition);
+  tab_strip_model->SetTabPinned(1, true);
+  // Simulate an extension protecting a tab.
+  g_browser_process->GetTabManager()->SetTabAutoDiscardableState(bg_contents,
+                                                                 false);
+  SiteEngagementService::Get(profile())->ResetBaseScoreForURL(
+      GURL("https://www.chromium.org"), 91);
+
+  {
+    TabMetricsLogger::TabMetrics bg_metrics;
+    bg_metrics.web_contents = bg_contents;
+    bg_metrics.page_transition = page_transition;
+    bg_metrics.page_metrics.key_event_count = 3;
+    bg_metrics.page_metrics.mouse_event_count = 42;
+    bg_metrics.page_metrics.touch_event_count = 10;
+
+    resource_coordinator::TabFeatures bg_features =
+        TabMetricsLogger::GetTabFeatures(browser.get(), bg_metrics);
+    EXPECT_EQ(TabMetricsEvent::CONTENT_TYPE_TEXT_HTML,
+              bg_features.content_type);
+    EXPECT_EQ(bg_features.has_before_unload_handler, false);
+    EXPECT_EQ(bg_features.has_form_entry, false);
+    EXPECT_EQ(bg_features.is_extension_protected, true);
+    EXPECT_EQ(bg_features.is_pinned, true);
+    EXPECT_EQ(bg_features.key_event_count, 3);
+    EXPECT_EQ(bg_features.mouse_event_count, 42);
+    EXPECT_EQ(bg_features.navigation_entry_count, 2);
+    ASSERT_TRUE(bg_features.page_transition_core_type.has_value());
+    EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs(
+        ui::PAGE_TRANSITION_LINK,
+        bg_features.page_transition_core_type.value()));
+    EXPECT_EQ(bg_features.page_transition_from_address_bar, true);
+    EXPECT_EQ(bg_features.page_transition_is_redirect, false);
+    ASSERT_TRUE(bg_features.site_engagement_score.has_value());
+    // Site engagement score should round down to the nearest 10.
+    EXPECT_EQ(bg_features.site_engagement_score.value(), 90);
+    EXPECT_EQ(bg_features.touch_event_count, 10);
+    EXPECT_EQ(bg_features.was_recently_audible, false);
+  }
+
+  tab_strip_model->CloseAllTabs();
+}
 
 // Tests that protocol schemes are mapped to the correct enumerators.
-TEST(TabMetricsLoggerTest, Schemes) {
+TEST_F(TabMetricsLoggerTest, Schemes) {
   EXPECT_EQ(TabMetricsEvent::PROTOCOL_HANDLER_SCHEME_BITCOIN,
             TabMetricsLogger::GetSchemeValueFromString("bitcoin"));
   EXPECT_EQ(TabMetricsEvent::PROTOCOL_HANDLER_SCHEME_GEO,
@@ -64,7 +173,7 @@ TEST(TabMetricsLoggerTest, Schemes) {
 }
 
 // Tests non-whitelisted protocol schemes.
-TEST(TabMetricsLoggerTest, NonWhitelistedSchemes) {
+TEST_F(TabMetricsLoggerTest, NonWhitelistedSchemes) {
   // Native (non-web-based) handler.
   EXPECT_EQ(TabMetricsEvent::PROTOCOL_HANDLER_SCHEME_OTHER,
             TabMetricsLogger::GetSchemeValueFromString("foo"));