[Sessions] Add detailed logging of SessionRestore events.

chrome/browser/sessions/session_restore_stats_collector.h

Index: chrome/browser/sessions/session_restore_stats_collector.h
diff --git a/chrome/browser/sessions/session_restore_stats_collector.h b/chrome/browser/sessions/session_restore_stats_collector.h
index 668e2c01b77a52e1a1ed57ea4017da9d870f44fd..61ece634eec24b72c478ef48a79fc22cace722a7 100644
--- a/chrome/browser/sessions/session_restore_stats_collector.h
+++ b/chrome/browser/sessions/session_restore_stats_collector.h
@@ -5,7 +5,7 @@
 #ifndef CHROME_BROWSER_SESSIONS_SESSION_RESTORE_STATS_COLLECTOR_H_
 #define CHROME_BROWSER_SESSIONS_SESSION_RESTORE_STATS_COLLECTOR_H_
 
-#include <set>
+#include <map>
 
 #include "base/callback_list.h"
 #include "chrome/browser/sessions/session_restore.h"
@@ -20,83 +20,248 @@ class NavigationController;
 
 // SessionRestoreStatsCollector observes SessionRestore events ands records UMA
 // accordingly.
+//
+// A SessionRestoreStatsCollector is tied to an instance of a session restore,
+// currently being instantianted and owned by the TabLoader. It has two main
+// phases to its life:
+//
+// 1. The session restore is active and ongoing (the TabLoader is still
+//    scheduling tabs for loading). This phases ends when there are no
+//    non-deferred tabs left to be loaded. During this phases statistics are
+//    gathered in a structure before being emitted as UMA metrics at the end of
+//    this phase. At this point the TabLoader ceases to exist and destroys it's
+//    reference to the SessionRestoreStatsCollector.
+// 2. If any tabs have been deferred the SessionRestoreStatsCollector continues
+//    tracking deferred tabs. This continues to observe the tabs to see which
+//    (if any) of the deferred tabs are subsequently forced to be loaded by the
+//    user. Since such tabs may exist until the end of the browsers life the
+//    statistics are emitted immediately, or risk being lost entirely. When
+//    there are no longer deferred tabs to track the
+//    SessionRestoreStatsCollector will destroy itself.
+//
+// TODO(chrisha): Many of these metrics don't make sense to collect in the
+// presence of an unavailable network, or when tabs are closed during loading.
+// Rethink the collection in these cases.
 class SessionRestoreStatsCollector
     : public content::NotificationObserver,
       public base::RefCounted<SessionRestoreStatsCollector> {
  public:
-  // Called to start tracking tabs. If a restore is already occuring, the tabs
-  // are added to the existing list of tracked tabs.
-  static void TrackTabs(
-      const std::vector<SessionRestoreDelegate::RestoredTab>& tabs,
-      const base::TimeTicks& restore_started);
-
-  // Called to start tracking only active tabs. If a restore is already
-  // occuring, the tabs are added to the existing list of tracked tabs.
-  static void TrackActiveTabs(
-      const std::vector<SessionRestoreDelegate::RestoredTab>& tabs,
-      const base::TimeTicks& restore_started);
+  // Houses all of the statistics gathered by the SessionRestoreStatsCollector
+  // while the underlying TabLoader is active. These statistics are all reported
+  // at once via the reporting delegate.
+  struct TabLoaderStats {
+    // Constructor that initializes everything to zero.
+    TabLoaderStats();
+
+    // The number of tabs involved all overlapping session restores being

[sky, 2015/06/03 22:00:08]

'in' all?

[chrisha, 2015/06/04 14:04:47]

Done.

+    // tracked by this SessionRestoreStatsCollector. This corresponds to the
+    // "SessionRestore.TabCount" metric and one bucket of the
+    // "SessionRestore.TabActions" histogram.
+    size_t tab_count;
+
+    // The number of tabs loaded automatically because they are active, and
+    // explicitly caused to be loaded by the TabLoader. This corresponds to one
+    // bucket of the "SessionRestore.TabActions" histogram.
+    size_t tabs_loaded;
+
+    // The time elapsed between |restore_started| and reception of the first
+    // NOTIFICATION_LOAD_STOP event for any of the active tabs involved in the
+    // session restore. If this is zero it is because it has not been
+    // recorded (all visible tabs were closed before they finished loading, or
+    // the user switched to an already loaded tab before a visible session
+    // restore tab finished loading). Corresponds to
+    // "SessionRestore.ForegroundTabFirstLoaded" and its _XX variants.
+    base::TimeDelta foreground_tab_first_loaded;
+
+    // The time elapsed between |restore_started| and reception of the first
+    // NOTIFICATION_RENDER_WIDGET_HOST_DID_UPDATE_BACKING_STORE event for any of
+    // the tabs involved in the session restore. If this is zero it is because
+    // it has not been recorded (all visible tabs were closed or switched away
+    // from before they were painted). Corresponds to
+    // "SessionRestore.ForegroundTabFirstPaint3" and its _XX variants.
+    base::TimeDelta foreground_tab_first_paint;
+
+    // The time taken for all non-deferred tabs to be loaded. This corresponds
+    // to the "SessionRestore.AllTabsLoaded" metric and its _XX variants
+    // (vaguely named for historical reasons, as it predates the concept of
+    // deferred tabs).
+    base::TimeDelta non_deferred_tabs_loaded;
+
+    // The maximum number of tabs loading in parallel. This corresponds to the
+    // "SessionRestore.ParallelTabLoads" metric.
+    size_t parallel_tab_loads;
+  };
+
+  // The StatsReportingDelegate is responsible for delivering statistics
+  // reported by the SessionRestoreStatsCollector.
+  class StatsReportingDelegate;
+
+  // An implementation of StatsReportingDelegate for reporting via UMA.
+  class UmaStatsReportingDelegate;
+
+  // Constructs a SessionRestoreStatsCollector.
+  SessionRestoreStatsCollector(
+      const base::TimeTicks& restore_started,
+      scoped_refptr<StatsReportingDelegate> reporting_delegate);
+
+  // Adds new tabs to the list of tracked tabs.
+  void TrackTabs(const std::vector<SessionRestoreDelegate::RestoredTab>& tabs);
+
+  // Called to indicate that the loading of a tab has been deferred by session
+  // restore.
+  void DeferTab(content::NavigationController* tab);
+
+  // Exposed for unittesting.
+  const TabLoaderStats& tab_loader_stats() const { return tab_loader_stats_; }
 
  private:
   friend class base::RefCounted<SessionRestoreStatsCollector>;
 
-  using RenderWidgetHostSet = std::set<content::RenderWidgetHost*>;
+  // Indicates whether the loading state of a tab.
+  enum TabLoadingState {
+    TAB_IS_NOT_LOADING,
+    TAB_IS_LOADING,
+    TAB_IS_LOADED
+  };
+
+  // State that is tracked for a tab while it is being observed.
+  struct TabState {
+    explicit TabState(content::NavigationController* controller);
+
+    // The NavigationController associated with the tab. This is the primary
+    // index for it and is never null.
+    content::NavigationController* controller;
+
+    // The RenderWidgetHost associated with the tab. This is the secondary
+    // index and starts out being null. If it is not null it is because the tab
+    // is actively loading or waiting to be painted.
+    content::RenderWidgetHost* render_widget_host;
+
+    // Set to true if the tab has been deferred by the TabLoader.
+    bool is_deferred;
+
+    // The current loading state of the tab.
+    TabLoadingState loading_state;
+  };
+
+  // Maps a NavigationController to its state. This is the primary map and
+  // physically houses the state.
+  using NavigationControllerMap = std::map<content::NavigationController*,
+                                           TabState>;
 
-  explicit SessionRestoreStatsCollector(const base::TimeTicks& restore_started);
   ~SessionRestoreStatsCollector() override;
 
-  // NotificationObserver method.
+  // NotificationObserver method. This is the workhorse of the class and drives
+  // all state transitions.
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
-  // Adds new tabs to the list of tracked tabs.
-  void AddTabs(const std::vector<SessionRestoreDelegate::RestoredTab>& tabs);
-
-  // Called when a tab is no longer tracked.
+  // Called when a tab is no longer tracked. This is called by the 'Observe'
+  // notification callback. Takes care of unregistering all observers and
+  // removing the tab from all internal data structures.
   void RemoveTab(content::NavigationController* tab);
 
-  // Registers for relevant notifications for a tab.
-  void RegisterForNotifications(content::NavigationController* tab);
+  // Registers for relevant notifications for a tab and inserts the tab into
+  // to tabs_tracked_ map. Return a pointer to the newly created TabState.
+  TabState* RegisterForNotifications(content::NavigationController* tab);
 
   // Returns the RenderWidgetHost of a tab.
   content::RenderWidgetHost* GetRenderWidgetHost(
       content::NavigationController* tab);
 
-  // Have we recorded the times for a foreground tab load?
+  // Returns the tab state, nullptr if not found.
+  TabState* GetTabState(content::NavigationController* tab);
+  TabState* GetTabState(content::RenderWidgetHost* tab);
+
+  // Marks a tab as loading.
+  void MarkTabAsLoading(TabState* tab_state);
+
+  // Returns true if done tracking non-deferred tabs. When this returns true
+  // the TabLoader will have finished its work and aggregate statistics will be
+  // emitted via Record
+  bool DoneTrackingNonDeferredTabs() const;
+
+  // Returns true when no longer tracking any tabs. When this returns true the
+  // collector will destory itself. Called from Observe.
+  bool DoneTracking() const;
+
+  // Has DoneTrackingNonDeferredTabs returned true?
+  bool got_done_tracking_non_deferred_tabs_;
+
+  // Has the time for foreground tab load been recorded?
   bool got_first_foreground_load_;
 
-  // Have we recorded the times for a foreground tab paint?
+  // Has the time for foreground tab paint been recorded?
   bool got_first_paint_;
 
   // The time the restore process started.
-  base::TimeTicks restore_started_;
+  const base::TimeTicks restore_started_;
 
-  // The renderers we have started loading into.
-  RenderWidgetHostSet render_widget_hosts_loading_;
+  // List of tracked tabs, mapped to their TabState.
+  NavigationControllerMap tabs_tracked_;
 
-  // The renderers we have loaded and are waiting on to paint.
-  RenderWidgetHostSet render_widget_hosts_to_paint_;
+  // Counts the number of non-deferred tabs that the
+  // SessionRestoreStatsCollector is waiting to see load.
+  size_t waiting_for_load_tab_count_;
 
-  // List of tracked tabs.
-  std::set<content::NavigationController*> tabs_tracked_;
-
-  // The number of tabs that have been restored.
-  int tab_count_;
-
-  // Max number of tabs that were loaded in parallel (for metrics).
-  size_t max_parallel_tab_loads_;
+  // Counts the current number of actively loading tabs.
+  size_t loading_tab_count_;
 
   // Notification registrar.
   content::NotificationRegistrar registrar_;
 
-  // To keep the collector alive as long as needed.
-  scoped_refptr<SessionRestoreStatsCollector> this_retainer_;
+  // Statistics gathered regarding the TabLoader.
+  TabLoaderStats tab_loader_stats_;
 
-  // The shared SessionRestoreNotifier instance for all SessionRestores running
-  // at this time.
-  static SessionRestoreStatsCollector* shared_collector_;
+  // The reporting delegate used to report gathered statistics.
+  scoped_refptr<StatsReportingDelegate> reporting_delegate_;
+
+  // For keeping SessionRestoreStatsCollector alive while it is still working
+  // even if no TabLoader references it. The object only lives on if it still
+  // has deferred tabs remaining from an interrupted session restore.
+  scoped_refptr<SessionRestoreStatsCollector> this_retainer_;
 
   DISALLOW_COPY_AND_ASSIGN(SessionRestoreStatsCollector);
 };
 
+// An abstract reporting delegate is used as a testing seam.
+class SessionRestoreStatsCollector::StatsReportingDelegate
+    : public base::RefCounted<StatsReportingDelegate> {

[sky, 2015/06/03 22:00:08]

Do you really need this ref counted?

[chrisha, 2015/06/04 14:04:47]

There's a lifetime management issue. Three options:

(1) The delegate must outlive the stats collector. That places the burden on the
user (the SessionRestoreDelegate or the TabLoader), and since their lifetimes
are decoupled from that of the SessionRestoreStatsCollector, this is not easy to
do.

(2) Ownership is transferred to the SessionRestoreStatsCollector. This works
fine in the general case, but makes unittesting slightly harder, as the delegate
will be reaped with the stats collector. This means the delegate needs to
communicate to another class with a longer lifetime, or the unittest has to
arrange for the collector not to be reaped. Both of these approaches are
relatively simple.

(3) Reference counted. Maybe a little overkill.

Does (2) sound reasonable to you?

[sky, 2015/06/04 19:38:02]

2 sgtm

+ public:
+  StatsReportingDelegate() { }
+
+  // Called when TabLoader has completed its work.
+  virtual void ReportTabLoaderStats(
+      const TabLoaderStats& tab_loader_stats) = 0;
+  // Called when a tab has been deferred.
+  virtual void ReportTabDeferred() = 0;
+  // Called when a deferred tab has been loaded.
+  virtual void ReportDeferredTabLoaded() = 0;
+
+ private:
+  friend class base::RefCounted<StatsReportingDelegate>;
+
+  DISALLOW_COPY_AND_ASSIGN(StatsReportingDelegate);
+};
+
+// The default reporting delegate, which reports statistics via UMA.
+class SessionRestoreStatsCollector::UmaStatsReportingDelegate
+    : public StatsReportingDelegate {
+ public:
+  UmaStatsReportingDelegate();
+
+  // StatsReportingDelegate implementation.
+  void ReportTabLoaderStats(
+      const TabLoaderStats& tab_loader_stats) override;
+  void ReportTabDeferred() override;
+  void ReportDeferredTabLoaded() override;
+
+ private:
+  // Has ReportTabDeferred been called?
+  bool got_report_tab_deferred_;
+
+  DISALLOW_COPY_AND_ASSIGN(UmaStatsReportingDelegate);
+};
+
 #endif  // CHROME_BROWSER_SESSIONS_SESSION_RESTORE_STATS_COLLECTOR_H_