[Tab Metrics] Adding TabManagerStatsCollector to TabManager

This CL creates a separate class that is in charge of recording UMAs on behalf
of TabManager for tab and system-related properties and events during session
restore and background tab loading. The RecordSwitchTo metric method has been
moved out of TabManager and into TabManagerStatsCollector.

In this implementation, the TabManagerStatsCollector is scoped to the lifetime
and owned by the TabManager, and the TabManagerStatsCollector holds a pointer
to the TabManager.

Bug: 740625
Change-Id: Ic3c77d5d99563ceefedab3d417721a7624d02ffe
Reviewed-on: https://chromium-review.googlesource.com/565353Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Zhen Wang <zhenw@chromium.org>
Commit-Queue: Scott Haseley <shaseley@google.com>
Cr-Commit-Position: refs/heads/master@{#487311}

chrome/browser/BUILD.gn

@@ -3398,6 +3398,8 @@ split_static_library("browser") {
       "resource_coordinator/tab_manager_grc_tab_signal_observer.h",
       "resource_coordinator/tab_manager_observer.cc",
       "resource_coordinator/tab_manager_observer.h",
+      "resource_coordinator/tab_manager_stats_collector.cc",
+      "resource_coordinator/tab_manager_stats_collector.h",
       "resource_coordinator/tab_manager_web_contents_data.cc",
       "resource_coordinator/tab_manager_web_contents_data.h",
       "resource_coordinator/tab_stats.cc",

chrome/browser/resource_coordinator/tab_manager.cc

@@ -33,6 +33,7 @@
 #include "chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h"
 #include "chrome/browser/resource_coordinator/tab_manager_grc_tab_signal_observer.h"
 #include "chrome/browser/resource_coordinator/tab_manager_observer.h"
+#include "chrome/browser/resource_coordinator/tab_manager_stats_collector.h"
 #include "chrome/browser/resource_coordinator/tab_manager_web_contents_data.h"
 #include "chrome/browser/sessions/session_restore.h"
 #include "chrome/browser/ui/browser.h"
@@ -155,6 +156,7 @@ TabManager::TabManager()
   if (resource_coordinator::IsResourceCoordinatorEnabled()) {
     grc_tab_signal_observer_.reset(new GRCTabSignalObserver());
   }
+  tab_manager_stats_collector_.reset(new TabManagerStatsCollector(this));
 }
 
 TabManager::~TabManager() {
@@ -825,6 +827,11 @@ void TabManager::ActiveTabChanged(content::WebContents* old_contents,
                                   content::WebContents* new_contents,
                                   int index,
                                   int reason) {
+  // An active tab is not purged.
+  // Calling GetWebContentsData() early ensures that the WebContentsData is
+  // created for |new_contents|, which |tab_manager_stats_collector_| expects.
+  GetWebContentsData(new_contents)->set_is_purged(false);
+
   // If |old_contents| is set, that tab has switched from being active to
   // inactive, so record the time of that transition.
   if (old_contents) {
@@ -835,12 +842,9 @@ void TabManager::ActiveTabChanged(content::WebContents* old_contents,
             GetTimeToPurge(min_time_to_purge_, max_time_to_purge_));
     // Only record switch-to-tab metrics when a switch happens, i.e.
     // |old_contents| is set.
-    RecordSwitchToTab(new_contents);
+    tab_manager_stats_collector_->RecordSwitchToTab(new_contents);
   }
 
-  // An active tab is not purged.
-  GetWebContentsData(new_contents)->set_is_purged(false);
-
   // Reload |web_contents| if it is in an active browser and discarded.
   if (IsActiveWebContentsInActiveBrowser(new_contents)) {
     ReloadWebContentsIfDiscarded(new_contents,
@@ -999,14 +1003,6 @@ std::vector<TabManager::BrowserInfo> TabManager::GetBrowserInfoList() const {
   return browser_info_list;
 }
 
-void TabManager::RecordSwitchToTab(content::WebContents* contents) const {
-  if (is_session_restore_loading_tabs_) {
-    UMA_HISTOGRAM_ENUMERATION("TabManager.SessionRestore.SwitchToTab",
-                              GetWebContentsData(contents)->tab_loading_state(),
-                              TAB_LOADING_STATE_MAX);
-  }
-}
-
 content::NavigationThrottle::ThrottleCheckResult
 TabManager::MaybeThrottleNavigation(
     content::NavigationHandle* navigation_handle) {

chrome/browser/resource_coordinator/tab_manager.h

@@ -47,6 +47,7 @@ namespace resource_coordinator {
 #if defined(OS_CHROMEOS)
 class TabManagerDelegate;
 #endif
+class TabManagerStatsCollector;
 
 // The TabManager periodically updates (see
 // |kAdjustmentIntervalSeconds| in the source) the status of renderers
@@ -209,13 +210,14 @@ class TabManager : public TabStripModelObserver,
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, FastShutdownSingleTabProcess);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest,
                            GetUnsortedTabStatsIsInVisibleWindow);
-  FRIEND_TEST_ALL_PREFIXES(TabManagerTest, HistogramsSessionRestoreSwitchToTab);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, DiscardTabWithNonVisibleTabs);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, MaybeThrottleNavigation);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, OnDidFinishNavigation);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, OnDidStopLoading);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, OnWebContentsDestroyed);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, OnDelayedTabSelected);
+  FRIEND_TEST_ALL_PREFIXES(TabManagerStatsCollectorTest,
+                           HistogramsSessionRestoreSwitchToTab);
 
   // Information about a Browser.
   struct BrowserInfo {
@@ -367,10 +369,6 @@ class TabManager : public TabStripModelObserver,
   void OnSessionRestoreStartedLoadingTabs();
   void OnSessionRestoreFinishedLoadingTabs();
 
-  // Records UMA histograms for the tab state when switching to a different tab
-  // during session restore.
-  void RecordSwitchToTab(content::WebContents* contents) const;
-
   // Returns true if the navigation should be delayed.
   bool ShouldDelayNavigation(
       content::NavigationHandle* navigation_handle) const;
@@ -476,6 +474,10 @@ class TabManager : public TabStripModelObserver,
   // GRC tab signal observer, receives tab scoped signal from GRC.
   std::unique_ptr<GRCTabSignalObserver> grc_tab_signal_observer_;
 
+  // Records UMAs for tab and system-related events and properties during
+  // session restore.
+  std::unique_ptr<TabManagerStatsCollector> tab_manager_stats_collector_;
+
   // Weak pointer factory used for posting delayed tasks.
   base::WeakPtrFactory<TabManager> weak_ptr_factory_;
 

chrome/browser/resource_coordinator/tab_manager_stats_collector.cc

+// Copyright 2017 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_manager_stats_collector.h"
+
+#include "base/metrics/histogram_macros.h"
+#include "chrome/browser/resource_coordinator/tab_manager.h"
+#include "chrome/browser/resource_coordinator/tab_manager_web_contents_data.h"
+
+namespace resource_coordinator {
+
+TabManagerStatsCollector::TabManagerStatsCollector(TabManager* tab_manager)
+    : tab_manager_(tab_manager) {}
+
+TabManagerStatsCollector::~TabManagerStatsCollector() = default;
+
+void TabManagerStatsCollector::RecordSwitchToTab(
+    content::WebContents* contents) const {
+  if (tab_manager_->IsSessionRestoreLoadingTabs()) {
+    auto* data = TabManager::WebContentsData::FromWebContents(contents);
+    DCHECK(data);
+    UMA_HISTOGRAM_ENUMERATION("TabManager.SessionRestore.SwitchToTab",
+                              data->tab_loading_state(), TAB_LOADING_STATE_MAX);
+  }
+}
+
+}  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_manager_stats_collector.h

+// Copyright 2017 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_MANAGER_STATS_COLLECTOR_H_
+#define CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_MANAGER_STATS_COLLECTOR_H_
+
+namespace content {
+class WebContents;
+}  // namespace content
+
+namespace resource_coordinator {
+
+class TabManager;
+
+// TabManagerStatsCollector records UMAs on behalf of TabManager for tab and
+// system-related events and properties during session restore.
+class TabManagerStatsCollector {
+ public:
+  explicit TabManagerStatsCollector(TabManager* tab_manager);
+  ~TabManagerStatsCollector();
+
+  // Records UMA histograms for the tab state when switching to a different tab
+  // during session restore.
+  void RecordSwitchToTab(content::WebContents* contents) const;
+
+ private:
+  TabManager* tab_manager_;
+};
+
+}  // namespace resource_coordinator
+
+#endif  // CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_MANAGER_STATS_COLLECTOR_H_

chrome/browser/resource_coordinator/tab_manager_stats_collector_unittest.cc

+// Copyright 2017 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_manager_stats_collector.h"
+
+#include <memory>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/test/histogram_tester.h"
+#include "chrome/browser/resource_coordinator/tab_manager_web_contents_data.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/web_contents.h"
+#include "content/test/test_web_contents.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::WebContents;
+
+namespace resource_coordinator {
+
+class TabManagerStatsCollectorTest : public ChromeRenderViewHostTestHarness {
+ public:
+  WebContents* CreateWebContents() {
+    return content::TestWebContents::Create(profile(), nullptr);
+  }
+};
+
+TEST_F(TabManagerStatsCollectorTest, HistogramsSessionRestoreSwitchToTab) {
+  const char kHistogramName[] = "TabManager.SessionRestore.SwitchToTab";
+
+  TabManager tab_manager;
+  std::unique_ptr<WebContents> tab(CreateWebContents());
+
+  auto* data = tab_manager.GetWebContentsData(tab.get());
+  auto* stats_collector = tab_manager.tab_manager_stats_collector_.get();
+
+  base::HistogramTester histograms;
+  histograms.ExpectTotalCount(kHistogramName, 0);
+
+  data->SetTabLoadingState(TAB_IS_LOADING);
+  stats_collector->RecordSwitchToTab(tab.get());
+  stats_collector->RecordSwitchToTab(tab.get());
+
+  // Nothing should happen until we're in a session restore.
+  histograms.ExpectTotalCount(kHistogramName, 0);
+
+  tab_manager.OnSessionRestoreStartedLoadingTabs();
+
+  data->SetTabLoadingState(TAB_IS_NOT_LOADING);
+  stats_collector->RecordSwitchToTab(tab.get());
+  stats_collector->RecordSwitchToTab(tab.get());
+  histograms.ExpectTotalCount(kHistogramName, 2);
+  histograms.ExpectBucketCount(kHistogramName, TAB_IS_NOT_LOADING, 2);
+
+  data->SetTabLoadingState(TAB_IS_LOADING);
+  stats_collector->RecordSwitchToTab(tab.get());
+  stats_collector->RecordSwitchToTab(tab.get());
+  stats_collector->RecordSwitchToTab(tab.get());
+
+  histograms.ExpectTotalCount(kHistogramName, 5);
+  histograms.ExpectBucketCount(kHistogramName, TAB_IS_NOT_LOADING, 2);
+  histograms.ExpectBucketCount(kHistogramName, TAB_IS_LOADING, 3);
+
+  data->SetTabLoadingState(TAB_IS_LOADED);
+  stats_collector->RecordSwitchToTab(tab.get());
+  stats_collector->RecordSwitchToTab(tab.get());
+  stats_collector->RecordSwitchToTab(tab.get());
+  stats_collector->RecordSwitchToTab(tab.get());
+
+  histograms.ExpectTotalCount(kHistogramName, 9);
+  histograms.ExpectBucketCount(kHistogramName, TAB_IS_NOT_LOADING, 2);
+  histograms.ExpectBucketCount(kHistogramName, TAB_IS_LOADING, 3);
+  histograms.ExpectBucketCount(kHistogramName, TAB_IS_LOADED, 4);
+}
+
+}  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_manager_unittest.cc

@@ -14,7 +14,6 @@
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/strings/string16.h"
-#include "base/test/histogram_tester.h"
 #include "base/test/mock_entropy_provider.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/time/time.h"
@@ -682,59 +681,6 @@ TEST_F(TabManagerTest, OnSessionRestoreStartedAndFinishedLoadingTabs) {
   EXPECT_FALSE(tab_manager->IsSessionRestoreLoadingTabs());
 }
 
-TEST_F(TabManagerTest, HistogramsSessionRestoreSwitchToTab) {
-  const char kHistogramName[] = "TabManager.SessionRestore.SwitchToTab";
-
-  TabManager tab_manager;
-  TabStripDummyDelegate delegate;
-  TabStripModel tab_strip(&delegate, profile());
-  WebContents* tab = CreateWebContents();
-  tab_strip.AppendWebContents(tab, true);
-
-  auto* data = tab_manager.GetWebContentsData(tab);
-
-  base::HistogramTester histograms;
-  histograms.ExpectTotalCount(kHistogramName, 0);
-
-  data->SetTabLoadingState(TAB_IS_LOADING);
-  tab_manager.RecordSwitchToTab(tab);
-  tab_manager.RecordSwitchToTab(tab);
-
-  // Nothing should happen until we're in a session restore
-  histograms.ExpectTotalCount(kHistogramName, 0);
-
-  tab_manager.OnSessionRestoreStartedLoadingTabs();
-
-  data->SetTabLoadingState(TAB_IS_NOT_LOADING);
-  tab_manager.RecordSwitchToTab(tab);
-  tab_manager.RecordSwitchToTab(tab);
-  histograms.ExpectTotalCount(kHistogramName, 2);
-  histograms.ExpectBucketCount(kHistogramName, TAB_IS_NOT_LOADING, 2);
-
-  data->SetTabLoadingState(TAB_IS_LOADING);
-  tab_manager.RecordSwitchToTab(tab);
-  tab_manager.RecordSwitchToTab(tab);
-  tab_manager.RecordSwitchToTab(tab);
-
-  histograms.ExpectTotalCount(kHistogramName, 5);
-  histograms.ExpectBucketCount(kHistogramName, TAB_IS_NOT_LOADING, 2);
-  histograms.ExpectBucketCount(kHistogramName, TAB_IS_LOADING, 3);
-
-  data->SetTabLoadingState(TAB_IS_LOADED);
-  tab_manager.RecordSwitchToTab(tab);
-  tab_manager.RecordSwitchToTab(tab);
-  tab_manager.RecordSwitchToTab(tab);
-  tab_manager.RecordSwitchToTab(tab);
-
-  histograms.ExpectTotalCount(kHistogramName, 9);
-  histograms.ExpectBucketCount(kHistogramName, TAB_IS_NOT_LOADING, 2);
-  histograms.ExpectBucketCount(kHistogramName, TAB_IS_LOADING, 3);
-  histograms.ExpectBucketCount(kHistogramName, TAB_IS_LOADED, 4);
-
-  // Tabs with a committed URL must be closed explicitly to avoid DCHECK errors.
-  tab_strip.CloseAllTabs();
-}
-
 TEST_F(TabManagerTest, MaybeThrottleNavigation) {
   TabManager* tab_manager = g_browser_process->GetTabManager();
   MaybeThrottleNavigations(tab_manager);

chrome/browser/resource_coordinator/tab_manager_web_contents_data.h

@@ -141,7 +141,6 @@ class TabManager::WebContentsData
   // Needed to access tab_data_.
   FRIEND_TEST_ALL_PREFIXES(TabManagerWebContentsDataTest, CopyState);
   FRIEND_TEST_ALL_PREFIXES(TabManagerWebContentsDataTest, TabLoadingState);
-  FRIEND_TEST_ALL_PREFIXES(TabManagerTest, HistogramsSessionRestoreSwitchToTab);
 
   struct Data {
     Data();

chrome/test/BUILD.gn

@@ -3623,6 +3623,7 @@ test("unit_tests") {
       "../browser/media_galleries/win/mtp_device_object_enumerator_unittest.cc",
       "../browser/resource_coordinator/background_tab_navigation_throttle_unittest.cc",
       "../browser/resource_coordinator/tab_manager_delegate_chromeos_unittest.cc",
+      "../browser/resource_coordinator/tab_manager_stats_collector_unittest.cc",
       "../browser/resource_coordinator/tab_manager_unittest.cc",
       "../browser/resource_coordinator/tab_manager_web_contents_data_unittest.cc",