TabDiscarder: Expand PageMetrics to get rid of struct TabMetrics.

This work is for separating TabFeatures calculation from ukm logging.

(1) PageMetrics is expanded to also have page_transition.

(2) TabMetrics is not needed any more.

(3) More tests are added to confirm that each TabFeature is populated
    correctly. (There will be another cl that confirms features in
    TabActivityWatcher are all populated correctly.)


Change-Id: Ie7f718155c03259513670cd0edce870f07a9c7cd
Bug: 945699
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1538069
Commit-Queue: Charles . <charleszhao@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#646627}

chrome/browser/resource_coordinator/tab_activity_watcher.cc

@@ -14,7 +14,6 @@
 #include "chrome/browser/resource_coordinator/tab_metrics_logger.h"
 #include "chrome/browser/resource_coordinator/tab_ranker/mru_features.h"
 #include "chrome/browser/resource_coordinator/tab_ranker/tab_features.h"
-#include "chrome/browser/resource_coordinator/tab_ranker/window_features.h"
 #include "chrome/browser/resource_coordinator/time.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -109,8 +108,7 @@ class TabActivityWatcher::WebContentsData
     backgrounded_time_ = replaced_tab.backgrounded_time_;
 
     // Copy the replaced tab's stats.
-    tab_metrics_.page_metrics = replaced_tab.tab_metrics_.page_metrics;
-    tab_metrics_.page_transition = replaced_tab.tab_metrics_.page_transition;
+    page_metrics_ = replaced_tab.page_metrics_;
 
     // Record previous ukm_source_id from the |replaced_tab|.
     previous_ukm_source_id_ = replaced_tab.ukm_source_id_;
@@ -190,7 +188,6 @@ class TabActivityWatcher::WebContentsData
   explicit WebContentsData(content::WebContents* web_contents)
       : WebContentsObserver(web_contents) {
     DCHECK(!web_contents->GetBrowserContext()->IsOffTheRecord());
-    tab_metrics_.web_contents = web_contents;
     web_contents->GetRenderViewHost()->GetWidget()->AddInputEventObserver(this);
 
     creation_time_ = NowTicks();
@@ -243,7 +240,7 @@ class TabActivityWatcher::WebContentsData
     foregrounded_time_ = NowTicks();
     creation_time_ = NowTicks();
 
-    tab_metrics_.page_metrics.num_reactivations++;
+    page_metrics_.num_reactivations++;
   }
 
   // content::WebContentsObserver:
@@ -274,10 +271,10 @@ class TabActivityWatcher::WebContentsData
     navigation_time_ = navigation_handle->NavigationStart();
 
     // Reset the per-page data.
-    tab_metrics_.page_metrics = {};
+    page_metrics_ = {};
 
     // Update navigation info.
-    tab_metrics_.page_transition = navigation_handle->GetPageTransition();
+    page_metrics_.page_transition = navigation_handle->GetPageTransition();
   }
 
   // Logs metrics for the tab when it stops loading instead of immediately
@@ -343,11 +340,11 @@ class TabActivityWatcher::WebContentsData
   // content::RenderWidgetHost::InputEventObserver:
   void OnInputEvent(const blink::WebInputEvent& event) override {
     if (blink::WebInputEvent::IsMouseEventType(event.GetType()))
-      tab_metrics_.page_metrics.mouse_event_count++;
+      page_metrics_.mouse_event_count++;
     else if (blink::WebInputEvent::IsKeyboardEventType(event.GetType()))
-      tab_metrics_.page_metrics.key_event_count++;
+      page_metrics_.key_event_count++;
     else if (blink::WebInputEvent::IsTouchEventType(event.GetType()))
-      tab_metrics_.page_metrics.touch_event_count++;
+      page_metrics_.touch_event_count++;
   }
 
   // Iterates through tabstrips to determine the index of |contents| in
@@ -402,27 +399,25 @@ class TabActivityWatcher::WebContentsData
 
   // Returns the tabfeatures of current tab by combining TabMetrics,
   // WindowFeatures and MRUFeatures.
+  // TODO(charleszhao): refactor TabMetricsLogger::GetTabFeatures to return a
+  // full TabFeatures instead of a partial TabFeatures.
   base::Optional<tab_ranker::TabFeatures> GetTabFeatures(
       const tab_ranker::MRUFeatures& mru = tab_ranker::MRUFeatures()) {
-    const Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
-    if (!browser)
-      return base::nullopt;
 
     // For tab features.
-    tab_ranker::TabFeatures tab = TabMetricsLogger::GetTabFeatures(
-        browser, tab_metrics_, NowTicks() - backgrounded_time_);
+    base::Optional<tab_ranker::TabFeatures> tab =
+        TabMetricsLogger::GetTabFeatures(page_metrics_, web_contents());
+    if (!tab.has_value())
+      return tab;
 
-    // For window features.
-    tab_ranker::WindowFeatures window =
-        TabMetricsLogger::CreateWindowFeatures(browser);
-    tab.window_is_active = window.is_active;
-    tab.window_show_state = window.show_state;
-    tab.window_tab_count = window.tab_count;
-    tab.window_type = window.type;
+    tab->time_from_backgrounded =
+        backgrounded_time_.is_null()
+            ? 0
+            : (NowTicks() - backgrounded_time_).InMilliseconds();
 
     // For mru features.
-    tab.mru_index = mru.index;
-    tab.total_tab_count = mru.total;
+    tab->mru_index = mru.index;
+    tab->total_tab_count = mru.total;
     return tab;
   }
 
@@ -475,8 +470,8 @@ class TabActivityWatcher::WebContentsData
   // The last navigation time associated with this tab.
   base::TimeTicks navigation_time_;
 
-  // Stores current stats for the tab.
-  TabMetricsLogger::TabMetrics tab_metrics_;
+  // Stores current page stats for the tab.
+  TabMetricsLogger::PageMetrics page_metrics_;
 
   // Set to true when the WebContents has been detached from its tab.
   bool is_detached_ = false;

chrome/browser/resource_coordinator/tab_metrics_logger.cc

@@ -37,8 +37,8 @@ using metrics::WindowMetricsEvent;
 namespace {
 
 // Populates navigation-related metrics.
-void PopulatePageTransitionFeatures(tab_ranker::TabFeatures* tab,
-                                    ui::PageTransition page_transition) {
+void PopulatePageTransitionFeatures(ui::PageTransition page_transition,
+                                    tab_ranker::TabFeatures* tab) {
   // 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) ||
@@ -58,32 +58,70 @@ void PopulatePageTransitionFeatures(tab_ranker::TabFeatures* tab,
       ui::PageTransitionIsRedirect(page_transition);
 }
 
+// Populates TabFeatures from |page_metrics|.
+void PopulateTabFeaturesFromPageMetrics(
+    const TabMetricsLogger::PageMetrics& page_metrics,
+    tab_ranker::TabFeatures* tab) {
+  static_assert(sizeof(TabMetricsLogger::PageMetrics) ==
+                    sizeof(int) * 4 + sizeof(ui::PageTransition),
+                "Make sure all fields in PageMetrics are considered here.");
+  tab->key_event_count = page_metrics.key_event_count;
+  tab->mouse_event_count = page_metrics.mouse_event_count;
+  tab->num_reactivations = page_metrics.num_reactivations;
+  tab->touch_event_count = page_metrics.touch_event_count;
+
+  PopulatePageTransitionFeatures(page_metrics.page_transition, tab);
+}
+
+// Populates TabFeatures that can be calculated simply from |web_contents|.
+void PopulateTabFeaturesFromWebContents(content::WebContents* web_contents,
+                                        tab_ranker::TabFeatures* tab_features) {
+  tab_features->has_before_unload_handler =
+      web_contents->GetMainFrame()->GetSuddenTerminationDisablerState(
+          blink::kBeforeUnloadHandler);
+  tab_features->has_form_entry =
+      web_contents->GetPageImportanceSignals().had_form_interaction;
+  tab_features->host = web_contents->GetLastCommittedURL().host();
+  tab_features->navigation_entry_count =
+      web_contents->GetController().GetEntryCount();
+
+  if (SiteEngagementService::IsEnabled()) {
+    tab_features->site_engagement_score =
+        TabMetricsLogger::GetSiteEngagementScore(web_contents);
+  }
+
+  // This checks if the tab was audible within the past two seconds, same as the
+  // audio indicator in the tab strip.
+  tab_features->was_recently_audible =
+      RecentlyAudibleHelper::FromWebContents(web_contents)
+          ->WasRecentlyAudible();
+}
+
+// Populates TabFeatures that calculated from |browser| including WindowMetrics
+// and PinState.
+void PopulateTabFeaturesFromBrowser(const Browser* browser,
+                                    content::WebContents* web_contents,
+                                    tab_ranker::TabFeatures* tab_features) {
+  // For pin state.
+  const TabStripModel* tab_strip_model = browser->tab_strip_model();
+  int index = tab_strip_model->GetIndexOfWebContents(web_contents);
+  DCHECK_NE(index, TabStripModel::kNoTab);
+  tab_features->is_pinned = tab_strip_model->IsTabPinned(index);
+
+  // For window features.
+  tab_ranker::WindowFeatures window =
+      TabMetricsLogger::CreateWindowFeatures(browser);
+  tab_features->window_is_active = window.is_active;
+  tab_features->window_show_state = window.show_state;
+  tab_features->window_tab_count = window.tab_count;
+  tab_features->window_type = window.type;
+}
+
 }  // namespace
 
 TabMetricsLogger::TabMetricsLogger() = default;
 TabMetricsLogger::~TabMetricsLogger() = default;
 
-// static
-TabMetricsEvent::ContentType TabMetricsLogger::GetContentTypeFromMimeType(
-    const std::string& mime_type) {
-  // Test for special cases before testing wildcard types.
-  if (mime_type.empty())
-    return TabMetricsEvent::CONTENT_TYPE_UNKNOWN;
-  if (net::MatchesMimeType("text/html", mime_type))
-    return TabMetricsEvent::CONTENT_TYPE_TEXT_HTML;
-  if (net::MatchesMimeType("application/*", mime_type))
-    return TabMetricsEvent::CONTENT_TYPE_APPLICATION;
-  if (net::MatchesMimeType("audio/*", mime_type))
-    return TabMetricsEvent::CONTENT_TYPE_AUDIO;
-  if (net::MatchesMimeType("image/*", mime_type))
-    return TabMetricsEvent::CONTENT_TYPE_IMAGE;
-  if (net::MatchesMimeType("text/*", mime_type))
-    return TabMetricsEvent::CONTENT_TYPE_TEXT;
-  if (net::MatchesMimeType("video/*", mime_type))
-    return TabMetricsEvent::CONTENT_TYPE_VIDEO;
-  return TabMetricsEvent::CONTENT_TYPE_OTHER;
-}
-
 // static
 int TabMetricsLogger::GetSiteEngagementScore(
     content::WebContents* web_contents) {
@@ -104,44 +142,18 @@ int TabMetricsLogger::GetSiteEngagementScore(
 }
 
 // static
-tab_ranker::TabFeatures TabMetricsLogger::GetTabFeatures(
-    const Browser* browser,
-    const TabMetrics& tab_metrics,
-    base::TimeDelta inactive_duration) {
-  DCHECK(browser);
-  const TabStripModel* tab_strip_model = browser->tab_strip_model();
-  content::WebContents* web_contents = tab_metrics.web_contents;
+base::Optional<tab_ranker::TabFeatures> TabMetricsLogger::GetTabFeatures(
+    const PageMetrics& page_metrics,
+    content::WebContents* web_contents) {
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
+  if (!browser)
+    return base::nullopt;
 
   tab_ranker::TabFeatures tab;
+  PopulateTabFeaturesFromWebContents(web_contents, &tab);
+  PopulateTabFeaturesFromBrowser(browser, web_contents, &tab);
+  PopulateTabFeaturesFromPageMetrics(page_metrics, &tab);
 
-  tab.has_before_unload_handler =
-      web_contents->GetMainFrame()->GetSuddenTerminationDisablerState(
-          blink::kBeforeUnloadHandler);
-  tab.has_form_entry =
-      web_contents->GetPageImportanceSignals().had_form_interaction;
-  tab.host = web_contents->GetLastCommittedURL().host();
-
-  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();
-  tab.num_reactivations = tab_metrics.page_metrics.num_reactivations;
-
-  PopulatePageTransitionFeatures(&tab, tab_metrics.page_transition);
-
-  if (SiteEngagementService::IsEnabled())
-    tab.site_engagement_score = GetSiteEngagementScore(web_contents);
-
-  tab.time_from_backgrounded = inactive_duration.InMilliseconds();
-  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.
-  auto* audible_helper = RecentlyAudibleHelper::FromWebContents(web_contents);
-  tab.was_recently_audible = audible_helper->WasRecentlyAudible();
   return tab;
 }
 

chrome/browser/resource_coordinator/tab_metrics_logger.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_METRICS_LOGGER_H_
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "chrome/browser/resource_coordinator/tab_metrics_event.pb.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "ui/base/page_transition_types.h"
@@ -40,18 +41,8 @@ class TabMetricsLogger {
     int touch_event_count = 0;
     // Number of times this tab has been reactivated.
     int num_reactivations = 0;
-  };
-
-  // The state of a tab.
-  struct TabMetrics {
-    content::WebContents* web_contents = nullptr;
-
     // Source of the last committed navigation.
     ui::PageTransition page_transition = ui::PAGE_TRANSITION_FIRST;
-
-    // Per-page metrics of the state of the WebContents. Tracked since the
-    // tab's last top-level navigation.
-    PageMetrics page_metrics = {};
   };
 
   // A struct that contains metrics to be logged in ForegroundedOrClosed event.
@@ -84,10 +75,6 @@ class TabMetricsLogger {
   void LogTabLifetime(ukm::SourceId ukm_source_id,
                       base::TimeDelta time_since_navigation);
 
-  // Returns the ContentType that matches |mime_type|.
-  static metrics::TabMetricsEvent::ContentType GetContentTypeFromMimeType(
-      const std::string& mime_type);
-
   // Returns the site engagement score for the WebContents, rounded down to 10s
   // to limit granularity. Returns -1 if site engagement service is disabled.
   static int GetSiteEngagementScore(content::WebContents* web_contents);
@@ -96,10 +83,9 @@ class TabMetricsLogger {
   // 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 tab_ranker::TabFeatures GetTabFeatures(
-      const Browser* browser,
-      const TabMetrics& tab_metrics,
-      base::TimeDelta inactive_duration);
+  static base::Optional<tab_ranker::TabFeatures> GetTabFeatures(
+      const PageMetrics& page_metrics,
+      content::WebContents* web_contents);
 
   // Returns a populated WindowFeatures for the browser.
   static tab_ranker::WindowFeatures CreateWindowFeatures(

chrome/browser/resource_coordinator/tab_metrics_logger_interactive_uitest.cc

@@ -9,6 +9,7 @@
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/resource_coordinator/tab_metrics_event.pb.h"
+#include "chrome/browser/resource_coordinator/tab_ranker/tab_features.h"
 #include "chrome/browser/resource_coordinator/tab_ranker/window_features.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -22,12 +23,15 @@ using metrics::WindowMetricsEvent;
 using tab_ranker::WindowFeatures;
 
 const char* kTestUrl = "https://example.com/";
+constexpr char kBeforeUnloadHtml[] =
+    "data:text/html,<html><body><script>window.onbeforeunload=function(e) {}"
+    "</script></body></html>";
 
 // Tests WindowFeatures generated by TabMetricsLogger::CreateWindowFeatures due
 // to interactive changes to window state.
-class CreateWindowFeaturesTest : public InProcessBrowserTest {
+class TabMetricsLoggerTest : public InProcessBrowserTest {
  protected:
-  CreateWindowFeaturesTest() = default;
+  TabMetricsLoggerTest() = default;
 
   // InProcessBrowserTest:
   void PreRunTestOnMainThread() override {
@@ -49,12 +53,27 @@ class CreateWindowFeaturesTest : public InProcessBrowserTest {
 #endif
   }
 
+  // Returns TabFeatures of Tab at |index|.
+  tab_ranker::TabFeatures CurrentTabFeatures(const int index) {
+    auto* web_contents = browser()->tab_strip_model()->GetWebContentsAt(index);
+    return TabMetricsLogger::GetTabFeatures(TabMetricsLogger::PageMetrics(),
+                                            web_contents)
+        .value();
+  }
+
  private:
-  DISALLOW_COPY_AND_ASSIGN(CreateWindowFeaturesTest);
+  DISALLOW_COPY_AND_ASSIGN(TabMetricsLoggerTest);
 };
 
+// Tests before unload handler is calculated correctly.
+IN_PROC_BROWSER_TEST_F(TabMetricsLoggerTest, GetBeforeUnloadHandler) {
+  EXPECT_FALSE(CurrentTabFeatures(0).has_before_unload_handler);
+  ui_test_utils::NavigateToURL(browser(), GURL(kBeforeUnloadHtml));
+  EXPECT_TRUE(CurrentTabFeatures(0).has_before_unload_handler);
+}
+
 // Tests WindowMetrics of the current browser window.
-IN_PROC_BROWSER_TEST_F(CreateWindowFeaturesTest, Basic) {
+IN_PROC_BROWSER_TEST_F(TabMetricsLoggerTest, CreateWindowFeaturesTest) {
   WindowFeatures expected_metrics{WindowMetricsEvent::TYPE_TABBED,
                                   WindowMetricsEvent::SHOW_STATE_NORMAL, true,
                                   1};
@@ -84,7 +103,8 @@ IN_PROC_BROWSER_TEST_F(CreateWindowFeaturesTest, Basic) {
 // TODO(https://crbug.com/51364): Implement BrowserWindow::Deactivate() on Mac.
 #if !defined(OS_MACOSX)
 // Tests WindowMetrics by activating/deactivating the window.
-IN_PROC_BROWSER_TEST_F(CreateWindowFeaturesTest, WindowActivation) {
+IN_PROC_BROWSER_TEST_F(TabMetricsLoggerTest,
+                       CreateWindowFeaturesTestWindowActivation) {
   WindowFeatures expected_metrics{WindowMetricsEvent::TYPE_TABBED,
                                   WindowMetricsEvent::SHOW_STATE_NORMAL, false,
                                   1};
@@ -114,7 +134,8 @@ IN_PROC_BROWSER_TEST_F(CreateWindowFeaturesTest, WindowActivation) {
 }
 
 // Tests WindowMetrics when switching between windows.
-IN_PROC_BROWSER_TEST_F(CreateWindowFeaturesTest, MultipleWindows) {
+IN_PROC_BROWSER_TEST_F(TabMetricsLoggerTest,
+                       CreateWindowFeaturesTestMultipleWindows) {
   // Create a new browser window.
   Browser* browser_2 = CreateBrowser(browser()->profile());
   WindowFeatures expected_metrics{WindowMetricsEvent::TYPE_TABBED,