Popup metrics: track visible time for all tabs

This will help as a reference point for other visibility metrics at the
tab layer (namely popups and tab-unders).

This CL contains a minor refactor, so that the ScopedVisibilityTrackers
no longer own their tick clocks.

Bug: 661629
Change-Id: I879fe751661876442b609bad9008d16af52ba2f1
Reviewed-on: https://chromium-review.googlesource.com/694741
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505886}

chrome/browser/ui/blocked_content/popup_opener_tab_helper.cc

@@ -4,9 +4,11 @@
 
 #include "chrome/browser/ui/blocked_content/popup_opener_tab_helper.h"
 
+#include <utility>
+
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/time/default_tick_clock.h"
+#include "base/time/tick_clock.h"
 #include "chrome/browser/ui/blocked_content/scoped_visibility_tracker.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
@@ -15,36 +17,52 @@
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(PopupOpenerTabHelper);
 
+// static
+void PopupOpenerTabHelper::CreateForWebContents(
+    content::WebContents* contents,
+    std::unique_ptr<base::TickClock> tick_clock) {
+  DCHECK(contents);
+  if (!FromWebContents(contents)) {
+    contents->SetUserData(UserDataKey(),
+                          base::WrapUnique(new PopupOpenerTabHelper(
+                              contents, std::move(tick_clock))));
+  }
+}
+
 PopupOpenerTabHelper::~PopupOpenerTabHelper() {
-  if (!visibility_tracker_)
-    return;
-  base::TimeDelta foreground_duration =
-      visibility_tracker_->GetForegroundDuration();
-  UMA_HISTOGRAM_LONG_TIMES("Tab.VisibleTimeAfterCrossOriginRedirect",
-                           foreground_duration);
-  if (!last_popup_open_time_before_redirect_.is_null()) {
-    UMA_HISTOGRAM_LONG_TIMES(
-        "Tab.OpenedPopup.VisibleTimeAfterCrossOriginRedirect",
-        foreground_duration);
+  if (visibility_tracker_after_redirect_) {
+    base::TimeDelta foreground_duration =
+        visibility_tracker_after_redirect_->GetForegroundDuration();
+    UMA_HISTOGRAM_LONG_TIMES("Tab.VisibleTimeAfterCrossOriginRedirect",
+                             foreground_duration);
+    if (!last_popup_open_time_before_redirect_.is_null()) {
+      UMA_HISTOGRAM_LONG_TIMES(
+          "Tab.OpenedPopup.VisibleTimeAfterCrossOriginRedirect",
+          foreground_duration);
+    }
   }
+  DCHECK(visibility_tracker_);
+  UMA_HISTOGRAM_LONG_TIMES("Tab.VisibleTime",
+                           visibility_tracker_->GetForegroundDuration());
 }
 
 void PopupOpenerTabHelper::OnOpenedPopup(PopupTracker* popup_tracker) {
-  if (!visibility_tracker_) {
-    // Should still have clock ownership, since it is passed to the visibility
-    // tracker when that is instantiated.
-    DCHECK(tick_clock_);
+  if (!visibility_tracker_after_redirect_)
     last_popup_open_time_before_redirect_ = tick_clock_->NowTicks();
-  }
 }
 
-PopupOpenerTabHelper::PopupOpenerTabHelper(content::WebContents* web_contents)
+PopupOpenerTabHelper::PopupOpenerTabHelper(
+    content::WebContents* web_contents,
+    std::unique_ptr<base::TickClock> tick_clock)
     : content::WebContentsObserver(web_contents),
-      tick_clock_(base::MakeUnique<base::DefaultTickClock>()) {}
+      tick_clock_(std::move(tick_clock)) {
+  visibility_tracker_ = base::MakeUnique<ScopedVisibilityTracker>(
+      tick_clock_.get(), web_contents->IsVisible());
+}
 
 void PopupOpenerTabHelper::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (visibility_tracker_)
+  if (visibility_tracker_after_redirect_)
     return;
 
   if (navigation_handle->IsInMainFrame() && !web_contents()->IsVisible())
@@ -55,7 +73,7 @@ void PopupOpenerTabHelper::DidStartNavigation(
 
 void PopupOpenerTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (visibility_tracker_ || !navigation_handle->IsInMainFrame())
+  if (visibility_tracker_after_redirect_ || !navigation_handle->IsInMainFrame())
     return;
 
   size_t num_erased = pending_background_navigations_.erase(navigation_handle);
@@ -83,7 +101,6 @@ void PopupOpenerTabHelper::DidFinishNavigation(
     return;
   }
 
-  // Use the tick clock before passing ownership.
   if (!last_popup_open_time_before_redirect_.is_null()) {
     // If long times doesn't have enough resolution, consider switching the
     // macro.
@@ -92,17 +109,20 @@ void PopupOpenerTabHelper::DidFinishNavigation(
         tick_clock_->NowTicks() - last_popup_open_time_before_redirect_);
   }
 
-  visibility_tracker_ = base::MakeUnique<ScopedVisibilityTracker>(
-      std::move(tick_clock_), false /* is_visible */);
+  visibility_tracker_after_redirect_ =
+      base::MakeUnique<ScopedVisibilityTracker>(tick_clock_.get(),
+                                                false /* is_visible */);
   pending_background_navigations_.clear();
 }
 
 void PopupOpenerTabHelper::WasShown() {
-  if (visibility_tracker_)
-    visibility_tracker_->OnShown();
+  if (visibility_tracker_after_redirect_)
+    visibility_tracker_after_redirect_->OnShown();
+  visibility_tracker_->OnShown();
 }
 
 void PopupOpenerTabHelper::WasHidden() {
-  if (visibility_tracker_)
-    visibility_tracker_->OnHidden();
+  if (visibility_tracker_after_redirect_)
+    visibility_tracker_after_redirect_->OnHidden();
+  visibility_tracker_->OnHidden();
 }

chrome/browser/ui/blocked_content/popup_opener_tab_helper.h

@@ -6,7 +6,6 @@
 #define CHROME_BROWSER_UI_BLOCKED_CONTENT_POPUP_OPENER_TAB_HELPER_H_
 
 #include <memory>
-#include <utility>
 
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
@@ -15,6 +14,10 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
+namespace base {
+class TickClock;
+}
+
 namespace content {
 class NavigationHandle;
 class WebContents;
@@ -29,18 +32,17 @@ class PopupOpenerTabHelper
     : public content::WebContentsObserver,
       public content::WebContentsUserData<PopupOpenerTabHelper> {
  public:
+  static void CreateForWebContents(content::WebContents* contents,
+                                   std::unique_ptr<base::TickClock> tick_clock);
   ~PopupOpenerTabHelper() override;
 
-  void set_tick_clock_for_testing(std::unique_ptr<base::TickClock> tick_clock) {
-    tick_clock_ = std::move(tick_clock);
-  }
-
   void OnOpenedPopup(PopupTracker* popup_tracker);
 
  private:
   friend class content::WebContentsUserData<PopupOpenerTabHelper>;
 
-  explicit PopupOpenerTabHelper(content::WebContents* web_contents);
+  explicit PopupOpenerTabHelper(content::WebContents* web_contents,
+                                std::unique_ptr<base::TickClock> tick_clock);
 
   // content::WebContentsObserver:
   void DidStartNavigation(
@@ -54,12 +56,15 @@ class PopupOpenerTabHelper
   // WebContents is not visible.
   base::flat_set<content::NavigationHandle*> pending_background_navigations_;
 
-  // The clock which gets passed to the |visibility_tracker_|.
+  // The clock which is used by the visibility trackers.
   std::unique_ptr<base::TickClock> tick_clock_;
 
-  // The |visibility_tracker| tracks the time this WebContents is in the
-  // foreground. Will be nullptr until we redirect cross-origin in the
-  // background.
+  // The |visibility_tracker_after_redirect_| tracks the time this WebContents
+  // is in the foreground, after the tab does a cross site redirect in the
+  // background. Will be nullptr until that point in time.
+  std::unique_ptr<ScopedVisibilityTracker> visibility_tracker_after_redirect_;
+
+  // Keeps track of the total foreground time for this tab.
   std::unique_ptr<ScopedVisibilityTracker> visibility_tracker_;
 
   // Measures the time this WebContents opened a popup before

chrome/browser/ui/blocked_content/popup_opener_tab_helper_unittest.cc

@@ -34,12 +34,10 @@ class PopupOpenerTabHelperTest : public ChromeRenderViewHostTestHarness {
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
-    PopupOpenerTabHelper::CreateForWebContents(web_contents());
-
     auto tick_clock = base::MakeUnique<base::SimpleTestTickClock>();
     raw_clock_ = tick_clock.get();
-    PopupOpenerTabHelper::FromWebContents(web_contents())
-        ->set_tick_clock_for_testing(std::move(tick_clock));
+    PopupOpenerTabHelper::CreateForWebContents(web_contents(),
+                                               std::move(tick_clock));
 
     // The tick clock needs to be advanced manually so it isn't set to null,
     // which the code uses to determine if it is set yet.
@@ -87,6 +85,8 @@ class PopupOpenerTabHelperTest : public ChromeRenderViewHostTestHarness {
 
 TEST_F(PopupOpenerTabHelperTest, BackgroundNavigation_LogsMetrics) {
   NavigateAndCommitWithoutGesture(GURL("https://first.test/"));
+  raw_clock()->Advance(base::TimeDelta::FromMinutes(1));
+
   web_contents()->WasHidden();
   NavigateAndCommitWithoutGesture(GURL("https://example.test/"));
 
@@ -100,6 +100,10 @@ TEST_F(PopupOpenerTabHelperTest, BackgroundNavigation_LogsMetrics) {
   histogram_tester()->ExpectUniqueSample(
       kTabVisibleTimeAfterRedirect,
       base::TimeDelta::FromSeconds(1).InMilliseconds(), 1);
+
+  histogram_tester()->ExpectUniqueSample(
+      "Tab.VisibleTime", base::TimeDelta::FromSeconds(60 + 1).InMilliseconds(),
+      1);
 }
 
 TEST_F(PopupOpenerTabHelperTest, FirstNavigation_NoLogging) {

chrome/browser/ui/blocked_content/popup_tracker.cc

@@ -34,7 +34,8 @@ PopupTracker::~PopupTracker() {
 
 PopupTracker::PopupTracker(content::WebContents* contents,
                            content::WebContents* opener)
-    : content::WebContentsObserver(contents) {
+    : content::WebContentsObserver(contents),
+      tick_clock_(base::MakeUnique<base::DefaultTickClock>()) {
   if (auto* popup_opener = PopupOpenerTabHelper::FromWebContents(opener))
     popup_opener->OnOpenedPopup(this);
 }
@@ -50,8 +51,7 @@ void PopupTracker::DidFinishNavigation(
   // we've committed the first navigation in this WebContents.
   if (!first_load_visibility_tracker_) {
     first_load_visibility_tracker_ = base::MakeUnique<ScopedVisibilityTracker>(
-        base::MakeUnique<base::DefaultTickClock>(),
-        web_contents()->IsVisible());
+        tick_clock_.get(), web_contents()->IsVisible());
   } else {
     web_contents()->RemoveUserData(UserDataKey());
     // Destroys this object.

chrome/browser/ui/blocked_content/popup_tracker.h

@@ -12,6 +12,10 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
+namespace base {
+class DefaultTickClock;
+}
+
 namespace content {
 class WebContents;
 }
@@ -40,6 +44,9 @@ class PopupTracker : public content::WebContentsObserver,
   void WasShown() override;
   void WasHidden() override;
 
+  // The clock which is used by the visibility tracker.
+  std::unique_ptr<base::DefaultTickClock> tick_clock_;
+
   // The |first_load_visibility_tracker_| tracks the time this WebContents is in
   // the foreground for the duration of the first page load.
   std::unique_ptr<ScopedVisibilityTracker> first_load_visibility_tracker_;

chrome/browser/ui/blocked_content/scoped_visibility_tracker.cc

@@ -8,10 +8,9 @@
 
 #include "base/time/tick_clock.h"
 
-ScopedVisibilityTracker::ScopedVisibilityTracker(
-    std::unique_ptr<base::TickClock> tick_clock,
-    bool is_shown)
-    : tick_clock_(std::move(tick_clock)) {
+ScopedVisibilityTracker::ScopedVisibilityTracker(base::TickClock* tick_clock,
+                                                 bool is_shown)
+    : tick_clock_(tick_clock) {
   DCHECK(tick_clock_);
   if (is_shown)
     OnShown();

chrome/browser/ui/blocked_content/scoped_visibility_tracker.h

@@ -18,8 +18,8 @@ class TickClock;
 // OnShown/OnHidden notifications, which are logically idempotent.
 class ScopedVisibilityTracker {
  public:
-  ScopedVisibilityTracker(std::unique_ptr<base::TickClock> tick_clock,
-                          bool is_shown);
+  // |tick_clock| must outlive this object.
+  ScopedVisibilityTracker(base::TickClock* tick_clock, bool is_shown);
   ~ScopedVisibilityTracker();
 
   void OnShown();
@@ -30,7 +30,7 @@ class ScopedVisibilityTracker {
  private:
   void Update(bool in_foreground);
 
-  std::unique_ptr<base::TickClock> tick_clock_;
+  base::TickClock* tick_clock_;
 
   base::TimeTicks last_time_shown_;
   base::TimeDelta foreground_duration_;

chrome/browser/ui/blocked_content/scoped_visibility_tracker_unittest.cc

@@ -15,70 +15,65 @@ class ScopedVisibilityTrackerTest : public testing::Test {};
 
 TEST_F(ScopedVisibilityTrackerTest, NeverVisible) {
   auto tick_clock = base::MakeUnique<base::SimpleTestTickClock>();
-  auto* raw_clock = tick_clock.get();
-  ScopedVisibilityTracker tracker(std::move(tick_clock), false /* is_shown */);
+  ScopedVisibilityTracker tracker(tick_clock.get(), false /* is_shown */);
 
-  raw_clock->Advance(base::TimeDelta::FromMinutes(10));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(10));
   EXPECT_EQ(base::TimeDelta::FromMinutes(0), tracker.GetForegroundDuration());
 }
 
 TEST_F(ScopedVisibilityTrackerTest, SimpleVisibility) {
   auto tick_clock = base::MakeUnique<base::SimpleTestTickClock>();
-  auto* raw_clock = tick_clock.get();
-  ScopedVisibilityTracker tracker(std::move(tick_clock), true /* is_shown */);
+  ScopedVisibilityTracker tracker(tick_clock.get(), true /* is_shown */);
 
-  raw_clock->Advance(base::TimeDelta::FromMinutes(10));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(10));
   EXPECT_EQ(base::TimeDelta::FromMinutes(10), tracker.GetForegroundDuration());
 }
 
 TEST_F(ScopedVisibilityTrackerTest, HiddenThenShown) {
   auto tick_clock = base::MakeUnique<base::SimpleTestTickClock>();
-  auto* raw_clock = tick_clock.get();
-  ScopedVisibilityTracker tracker(std::move(tick_clock), true /* is_shown */);
+  ScopedVisibilityTracker tracker(tick_clock.get(), true /* is_shown */);
 
-  raw_clock->Advance(base::TimeDelta::FromMinutes(1));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(base::TimeDelta::FromMinutes(1), tracker.GetForegroundDuration());
 
   tracker.OnHidden();
-  raw_clock->Advance(base::TimeDelta::FromMinutes(2));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(2));
   EXPECT_EQ(base::TimeDelta::FromMinutes(1), tracker.GetForegroundDuration());
 
   tracker.OnShown();
-  raw_clock->Advance(base::TimeDelta::FromMinutes(3));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(3));
   EXPECT_EQ(base::TimeDelta::FromMinutes(4), tracker.GetForegroundDuration());
 }
 
 TEST_F(ScopedVisibilityTrackerTest, InitiallyHidden) {
   auto tick_clock = base::MakeUnique<base::SimpleTestTickClock>();
-  auto* raw_clock = tick_clock.get();
-  ScopedVisibilityTracker tracker(std::move(tick_clock), false /* is_shown */);
+  ScopedVisibilityTracker tracker(tick_clock.get(), false /* is_shown */);
 
-  raw_clock->Advance(base::TimeDelta::FromMinutes(1));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(base::TimeDelta::FromMinutes(0), tracker.GetForegroundDuration());
 
   tracker.OnShown();
-  raw_clock->Advance(base::TimeDelta::FromMinutes(2));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(2));
   EXPECT_EQ(base::TimeDelta::FromMinutes(2), tracker.GetForegroundDuration());
 }
 
 // The object should be robust to double hidden and shown notification
 TEST_F(ScopedVisibilityTrackerTest, DoubleNotifications) {
   auto tick_clock = base::MakeUnique<base::SimpleTestTickClock>();
-  auto* raw_clock = tick_clock.get();
-  ScopedVisibilityTracker tracker(std::move(tick_clock), false /* is_shown */);
+  ScopedVisibilityTracker tracker(tick_clock.get(), false /* is_shown */);
 
-  raw_clock->Advance(base::TimeDelta::FromMinutes(1));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(base::TimeDelta::FromMinutes(0), tracker.GetForegroundDuration());
 
   tracker.OnHidden();
-  raw_clock->Advance(base::TimeDelta::FromMinutes(1));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(1));
   EXPECT_EQ(base::TimeDelta::FromMinutes(0), tracker.GetForegroundDuration());
 
   tracker.OnShown();
-  raw_clock->Advance(base::TimeDelta::FromMinutes(2));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(2));
   EXPECT_EQ(base::TimeDelta::FromMinutes(2), tracker.GetForegroundDuration());
 
   tracker.OnShown();
-  raw_clock->Advance(base::TimeDelta::FromMinutes(2));
+  tick_clock->Advance(base::TimeDelta::FromMinutes(2));
   EXPECT_EQ(base::TimeDelta::FromMinutes(4), tracker.GetForegroundDuration());
 }

chrome/browser/ui/tab_helpers.cc

@@ -10,6 +10,7 @@
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
+#include "base/time/default_tick_clock.h"
 #include "build/build_config.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browser_process.h"
@@ -223,7 +224,8 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
   PDFPluginPlaceholderObserver::CreateForWebContents(web_contents);
   PermissionRequestManager::CreateForWebContents(web_contents);
   PopupBlockerTabHelper::CreateForWebContents(web_contents);
-  PopupOpenerTabHelper::CreateForWebContents(web_contents);
+  PopupOpenerTabHelper::CreateForWebContents(
+      web_contents, base::MakeUnique<base::DefaultTickClock>());
   PrefsTabHelper::CreateForWebContents(web_contents);
   prerender::PrerenderTabHelper::CreateForWebContents(web_contents);
   PreviewsInfoBarTabHelper::CreateForWebContents(web_contents);

tools/metrics/histograms/histograms.xml

@@ -83217,6 +83217,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Tab.VisibleTime" units="ms">
+  <owner>csharrison@chromium.org</owner>
+  <summary>
+    Measures the total time a tab is visible. Measured at the WebContents layer
+    so this metric does not attempt to account for things like session restore.
+    Logged at WebContents destruction.
+  </summary>
+</histogram>
+
 <histogram name="Tab.VisibleTimeAfterCrossOriginRedirect" units="ms">
   <owner>csharrison@chromium.org</owner>
   <summary>