[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..0974ba62578ae7baf4daf4c3d8a4f0f34ca0c705 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,81 +20,203 @@ 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 ReportTabLoaderStats.
+  struct TabLoaderStats {
+    // Constructor that initializes everything to zero.
+    TabLoaderStats();
+
+    // The number of tabs involved in the session restore. This corresponds to

[sky, 2015/05/27 15:18:39]

Using 'the session restore' is confusing here as this may actual span multiple
session restores.

[chrisha, 2015/06/03 21:16:36]

Done.

+    // the "SessionRestore.TabCount" metric and one bucket of the
+    // "SessionRestore.TabActions" histogram.
+    size_t tab_count;
+
+    // The number of tabs automatically loaded by the TabLoader. This

[sky, 2015/05/27 15:18:40]

It's not clear what automatically loaded means here. Please make it clearer.

[chrisha, 2015/06/03 21:16:36]

Done.

+    // corresponds to one bucket of the "SessionRestore.TabActions" histogram.
+    size_t tabs_loaded;
+
+    // The time taken to load the first foreground tab. If this is zero it is

[sky, 2015/05/27 15:18:40]

Clarify what this is relative to and what 'load' constitutes in this context.

[chrisha, 2015/06/03 21:16:36]

Done.

+    // because it has not been recorded (tab was closed before loading).
+    // Corresponds to "SessionRestore.ForegroundTabFirstLoaded" and its _XX
+    // variants.
+    base::TimeDelta foreground_tab_first_loaded;
+
+    // The time taken to paint the first foreground tab. If this is zero it is

[sky, 2015/05/27 15:18:40]

Same comment about what this is relative to. It's probably worth describing that
all times are relative to XXX.

[chrisha, 2015/06/03 21:16:36]

Done.

+    // because it has not been recorded (tab was closed before painting or user
+    // switched to another tab). Corresponds to
+    // "SessionRestore.ForegroundTabFirstPaint" and
+    // "SessionRestore.ForegroundTabFirstPaint2" metrics and their _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.
+    base::TimeDelta all_tabs_loaded;

[sky, 2015/05/27 15:18:40]

all is too overloaded here. Maybe time_to_non_deferred_tabs_loaded.

[chrisha, 2015/06/03 21:16:36]

Done.

+
+    // The maximum number of tabs loading in parallel. This corresponds to the
+    // "SessionRestore.ParallelTabLoads" metric.
+    size_t parallel_tab_loads;
+  };
+
+  explicit SessionRestoreStatsCollector(const base::TimeTicks& restore_started);
+
+  // 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_; }
+
+ protected:
+  // Reporting functions that cause actual UMA metrics to be emitted. Exposed as
+  // a unittesting seams.
+  virtual void ReportTabLoaderStats();

[sky, 2015/05/27 15:18:40]

I would prefer a delegate rather than subclassing.

[chrisha, 2015/06/03 21:16:36]

Done.

+  virtual void ReportTabDeferred();
+  virtual void ReportDeferredTabLoaded();
 
  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 {
+    TabState(content::NavigationController* controller);

[sky, 2015/05/27 15:18:40]

explicit

[chrisha, 2015/06/03 21:16:37]

Done.

+
+    // 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>;
+
+  // Maps a RenderWidgetHost to its state. This is a secondary index and maps
+  // the tab to a pointer to its state housed in the primary map.
+  using RenderWidgetHostMap = std::map<content::RenderWidgetHost*, 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_;
 
+  // Has ReportTabDeferred been called?
+  bool got_report_tab_deferred_;
+
   // The time the restore process started.
   base::TimeTicks restore_started_;

[sky, 2015/05/27 15:18:39]

const

[chrisha, 2015/06/03 21:16:36]

Done.

 
-  // 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_;
+  // A secondary index of TabState by their RenderWidgetHosts.
+  RenderWidgetHostMap render_widget_map_;
 
-  // List of tracked tabs.
-  std::set<content::NavigationController*> tabs_tracked_;
+  // Counts the number of non-deferred tabs that the
+  // SessionRestoreStatsCollector is waiting to see load.
+  size_t waiting_for_load_tab_count_;
 
-  // 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_;
+  // 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);
 };