Move two metrics from TabActivityWatcher to TabStatsTracker.

UMA metrics RecordVisibilityHistogram and UKM metrics
TabManager_TabLifetime are moved from TabActivityWatcher to
TabStatsTracker without changing its logic.

Bug: 1131349
Change-Id: Iee06e9ff87fb43293e62fd5d7b9d83af9437f205
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2437087Reviewed-by: Thanh Nguyen <thanhdng@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Charles . <charleszhao@chromium.org>
Commit-Queue: Charles . <charleszhao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814018}

chrome/browser/metrics/tab_stats_tracker.cc

@@ -15,6 +15,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/resource_coordinator/lifecycle_unit.h"
 #include "chrome/browser/resource_coordinator/lifecycle_unit_observer.h"
@@ -28,6 +29,9 @@
 #include "components/metrics/daily_event.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "components/ukm/content/source_url_recorder.h"
+#include "services/metrics/public/cpp/ukm_builders.h"
+#include "services/metrics/public/cpp/ukm_source_id.h"
 
 #if BUILDFLAG(ENABLE_BACKGROUND_MODE)
 #include "chrome/browser/background/background_mode_manager.h"
@@ -240,7 +244,9 @@ class TabStatsTracker::WebContentsUsageObserver
   WebContentsUsageObserver(content::WebContents* web_contents,
                            TabStatsTracker* tab_stats_tracker)
       : content::WebContentsObserver(web_contents),
-        tab_stats_tracker_(tab_stats_tracker) {}
+        tab_stats_tracker_(tab_stats_tracker),
+        visibility_(web_contents->GetVisibility()),
+        ukm_source_id_(ukm::GetSourceIdForWebContentsDocument(web_contents)) {}
 
   // content::WebContentsObserver:
   void DidStartNavigation(
@@ -252,25 +258,89 @@ class TabStatsTracker::WebContentsUsageObserver
     }
   }
 
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    if (!navigation_handle->HasCommitted() ||
+        !navigation_handle->IsInMainFrame() ||
+        navigation_handle->IsSameDocument()) {
+      return;
+    }
+    // Update navigation time for UKM reporting.
+    navigation_time_ = navigation_handle->NavigationStart();
+    ukm_source_id_ = ukm::ConvertToSourceId(
+        navigation_handle->GetNavigationId(), ukm::SourceIdType::NAVIGATION_ID);
+  }
+
   void DidGetUserInteraction(const blink::WebInputEvent& event) override {
     tab_stats_tracker_->tab_stats_data_store()->OnTabInteraction(
         web_contents());
   }
 
   void OnVisibilityChanged(content::Visibility visibility) override {
+    // Record Tab.Visibility.* histogram and do associated bookkeeping.
+    // Recording is done at every visibility state change rather than just when
+    // the WebContents is destroyed to reduce data loss on session end.
+    RecordVisibilityHistogram(visibility);
+
     if (visibility == content::Visibility::VISIBLE)
       tab_stats_tracker_->tab_stats_data_store()->OnTabVisible(web_contents());
   }
 
   void WebContentsDestroyed() override {
-    tab_stats_tracker_->OnWebContentsDestroyed(web_contents());
+    RecordVisibilityHistogram(visibility_);
 
+    if (ukm_source_id_) {
+      ukm::builders::TabManager_TabLifetime(ukm_source_id_)
+          .SetTimeSinceNavigation(
+              (base::TimeTicks::Now() - navigation_time_).InMilliseconds())
+          .Record(ukm::UkmRecorder::Get());
+    }
+
+    tab_stats_tracker_->OnWebContentsDestroyed(web_contents());
     // The call above will free |this| and so nothing should be done on this
     // object starting from here.
   }
 
  private:
   TabStatsTracker* tab_stats_tracker_;
+  // Current tab visibility.
+  content::Visibility visibility_;
+  // The last time at which |visibility_| changed.
+  base::TimeTicks last_visibility_change_time_ = base::TimeTicks::Now();
+  // The last navigation time associated with this tab.
+  base::TimeTicks navigation_time_ = base::TimeTicks::Now();
+  // Updated when a navigation is finished.
+  ukm::SourceId ukm_source_id_ = 0;
+
+  void RecordVisibilityHistogram(content::Visibility new_visibility) {
+    const base::TimeTicks now = base::TimeTicks::Now();
+    const base::TimeDelta duration = now - last_visibility_change_time_;
+    switch (visibility_) {
+      case content::Visibility::VISIBLE: {
+        UMA_HISTOGRAM_CUSTOM_TIMES("Tab.Visibility.Visible", duration,
+                                   base::TimeDelta::FromMilliseconds(1),
+                                   base::TimeDelta::FromDays(1), 50);
+        break;
+      }
+
+      case content::Visibility::OCCLUDED: {
+        UMA_HISTOGRAM_CUSTOM_TIMES("Tab.Visibility.Occluded", duration,
+                                   base::TimeDelta::FromMilliseconds(1),
+                                   base::TimeDelta::FromDays(1), 50);
+        break;
+      }
+
+      case content::Visibility::HIDDEN: {
+        UMA_HISTOGRAM_CUSTOM_TIMES("Tab.Visibility.Hidden", duration,
+                                   base::TimeDelta::FromMilliseconds(1),
+                                   base::TimeDelta::FromDays(1), 50);
+        break;
+      }
+    }
+
+    visibility_ = new_visibility;
+    last_visibility_change_time_ = now;
+  }
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsUsageObserver);
 };

chrome/browser/resource_coordinator/tab_activity_watcher.cc

@@ -33,14 +33,6 @@
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "third_party/blink/public/common/input/web_input_event.h"
 
-// Use a 1-day max for tab visibility histograms since it's not uncommon to keep
-// a tab in the same visibility state for a very long time (see Tab.VisibleTime
-// which has 5% of samples in the overflow bucket with a 1-hour max).
-#define UMA_TAB_VISIBILITY_HISTOGRAM(visibility, sample)           \
-  UMA_HISTOGRAM_CUSTOM_TIMES("Tab.Visibility." visibility, sample, \
-                             base::TimeDelta::FromMilliseconds(1), \
-                             base::TimeDelta::FromDays(1), 50)
-
 namespace resource_coordinator {
 namespace {
 using tab_ranker::TabFeatures;
@@ -284,9 +276,6 @@ class TabActivityWatcher::WebContentsData
         << "Expected a unique Source ID for the navigation";
     ukm_source_id_ = new_source_id;
 
-    // Update navigation time for UKM reporting.
-    navigation_time_ = navigation_handle->NavigationStart();
-
     // Reset the per-page data.
     page_metrics_ = {};
 
@@ -304,11 +293,6 @@ class TabActivityWatcher::WebContentsData
   }
 
   void OnVisibilityChanged(content::Visibility visibility) override {
-    // Record Tab.Visibility.* histogram and do associated bookkeeping.
-    // Recording is done at every visibility state change rather than just when
-    // the WebContents is destroyed to reduce data loss on session end.
-    RecordVisibilityHistogram(visibility);
-
     // Record background tab UKMs and do associated bookkepping.
     if (!web_contents()->IsBeingDestroyed()) {
       // TODO(michaelpg): Consider treating occluded tabs as hidden.
@@ -320,33 +304,7 @@ class TabActivityWatcher::WebContentsData
     }
   }
 
-  void RecordVisibilityHistogram(content::Visibility new_visibility) {
-    const base::TimeTicks now = NowTicks();
-    const base::TimeDelta duration = now - last_visibility_change_time_;
-    switch (visibility_) {
-      case content::Visibility::VISIBLE: {
-        UMA_TAB_VISIBILITY_HISTOGRAM("Visible", duration);
-        break;
-      }
-
-      case content::Visibility::OCCLUDED: {
-        UMA_TAB_VISIBILITY_HISTOGRAM("Occluded", duration);
-        break;
-      }
-
-      case content::Visibility::HIDDEN: {
-        UMA_TAB_VISIBILITY_HISTOGRAM("Hidden", duration);
-        break;
-      }
-    }
-
-    visibility_ = new_visibility;
-    last_visibility_change_time_ = now;
-  }
-
   void WebContentsDestroyed() override {
-    RecordVisibilityHistogram(visibility_);
-
     if (was_replaced_)
       return;
 
@@ -500,9 +458,6 @@ class TabActivityWatcher::WebContentsData
   // is activated.
   base::TimeTicks foregrounded_time_;
 
-  // The last navigation time associated with this tab.
-  base::TimeTicks navigation_time_;
-
   // Stores current page stats for the tab.
   TabMetricsLogger::PageMetrics page_metrics_;
 
@@ -512,12 +467,6 @@ class TabActivityWatcher::WebContentsData
   // If true, future events such as the tab being destroyed won't be logged.
   bool was_replaced_ = false;
 
-  // Current tab visibility.
-  content::Visibility visibility_ = web_contents()->GetVisibility();
-
-  // The last time at which |visibility_| changed.
-  base::TimeTicks last_visibility_change_time_ = NowTicks();
-
   // MRUFeatures of this WebContents, updated only before ForegroundedOrClosed
   // event is logged.
   tab_ranker::MRUFeatures mru_features_;
@@ -696,11 +645,6 @@ TabActivityWatcher* TabActivityWatcher::GetInstance() {
 }
 
 void TabActivityWatcher::OnTabClosed(WebContentsData* web_contents_data) {
-  // Log TabLifetime event.
-  tab_metrics_logger_->LogTabLifetime(
-      web_contents_data->ukm_source_id_,
-      NowTicks() - web_contents_data->navigation_time_);
-
   // Log ForegroundedOrClosed event.
   if (!web_contents_data->backgrounded_time_.is_null()) {
     web_contents_data->LogForegroundedOrClosedMetrics(