[Sessions] Add detailed logging of SessionRestore events.

chrome/browser/sessions/session_restore_stats_collector.h

 // Copyright 2015 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_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 "base/time/tick_clock.h"
 #include "chrome/browser/sessions/session_restore.h"
 #include "chrome/browser/sessions/session_restore_delegate.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/render_widget_host.h"
 
 namespace content {
 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);
-
- private:
+  // 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 in all overlapping session restores being
+    // 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. Ownership of

[sky, 2015/06/17 15:33:54]

I would nuke the second sentence. It is implied by virtue of reporting_delegate
being passed via a scoped_ptr.

[chrisha, 2015/06/17 17:44:34]

Done.

+  // |reporting_delegate| is passed to this object.
+  SessionRestoreStatsCollector(
+      const base::TimeTicks& restore_started,
+      scoped_ptr<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_; }
+
+ protected:

[sky, 2015/06/17 15:33:54]

Style guide says no protected members.

[chrisha, 2015/06/17 17:44:34]

Was just to make them unittest accessible. Will use a friend declaration
instead.

   friend class base::RefCounted<SessionRestoreStatsCollector>;
 
-  using RenderWidgetHostSet = std::set<content::RenderWidgetHost*>;
-
-  explicit SessionRestoreStatsCollector(const base::TimeTicks& restore_started);
-  ~SessionRestoreStatsCollector() override;
-
-  // NotificationObserver method.
+  // 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>;
+
+  virtual ~SessionRestoreStatsCollector();
+
+  // 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);
+
+  // Checks to see if the SessionRestoreStatsCollector has finished collecting,
+  // and cleans itself up if it has.
+  void CleanupIfDoneTracking();
+
+  // Testing seam for configuring the tick clock in use. Ownership of the clock
+  // is passed to this object.
+  void set_tick_clock(scoped_ptr<base::TickClock> tick_clock);

[sky, 2015/06/17 15:33:54]

Generally we inline functions in unix_hacker_style.

[chrisha, 2015/06/17 17:44:34]

Done.

+
+  // Has CleanIfDoneTracking determined that there are no non-deferred tabs to
+  // track?
+  bool 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_;
-
-  // The renderers we have started loading into.
-  RenderWidgetHostSet render_widget_hosts_loading_;
-
-  // The renderers we have loaded and are waiting on to paint.
-  RenderWidgetHostSet render_widget_hosts_to_paint_;
-
-  // 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_;
+  const base::TimeTicks restore_started_;
+
+  // List of tracked tabs, mapped to their TabState.
+  NavigationControllerMap tabs_tracked_;
+
+  // Counts the number of non-deferred tabs that the
+  // SessionRestoreStatsCollector is waiting to see load.
+  size_t waiting_for_load_tab_count_;
+
+  // 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.
+  // Statistics gathered regarding the TabLoader.
+  TabLoaderStats tab_loader_stats_;
+
+  // The source of ticks used for taking timing information. This is
+  // configurable as a testing seam. Defaults to using base::DefaultTickClock,
+  // which in turn uses base::TimeTicks.
+  scoped_ptr<base::TickClock> tick_clock_;
+
+  // The reporting delegate used to report gathered statistics.
+  scoped_ptr<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_;
 
-  // The shared SessionRestoreNotifier instance for all SessionRestores running
-  // at this time.
-  static SessionRestoreStatsCollector* shared_collector_;
-
   DISALLOW_COPY_AND_ASSIGN(SessionRestoreStatsCollector);
 };
 
+// An abstract reporting delegate is used as a testing seam.
+class SessionRestoreStatsCollector::StatsReportingDelegate {
+ public:
+  StatsReportingDelegate() { }

[sky, 2015/06/17 15:33:54]

nit: { } -> {} here, next line and 258. Make sure you run git cl format. I'm
pretty sure it would have fixed these up.

[chrisha, 2015/06/17 17:44:34]

Yeah, neglected to run format. Done now.

+  virtual ~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:
+  DISALLOW_COPY_AND_ASSIGN(StatsReportingDelegate);
+};
+
+// The default reporting delegate, which reports statistics via UMA.
+class SessionRestoreStatsCollector::UmaStatsReportingDelegate
+    : public StatsReportingDelegate {
+ public:
+  UmaStatsReportingDelegate();
+  virtual ~UmaStatsReportingDelegate() { }

[sky, 2015/06/17 15:33:54]

This should be override, no virtual.

[chrisha, 2015/06/17 17:44:34]

Done.

+
+  // 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_